From 02c70390e03ba1bff4c4225363009c99408128f1 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 1 Dec 2017 09:48:24 -0600 Subject: [PATCH 1/4] Ensure setting save_path considers save_handler The fix in #99, while correct, did not address setting the session.save_path for non-files save handlers when the save handler is set via `setOption('save_handler', $value)`. The reason for this was due to the fact that `setOption()` delegates directly to `setStorageOption()` instead of the relevant class method; the changes in #99 make that method a no-op in the case of a save handler option. What this patch does is two-fold: - It overrides `setOption()` and has it call `setPhpSaveHandler()` if `save_handler` is provided as the `$option` argument. It then returns on completion. Otherwise, it delegates to the parent. - It modifies `setPhpSaveHandler()` to extract the bulk of the logic to a new method, `performSaveHandlerUpdate()`. This new method now returns the save handler _name_ to store. `setPhpSaveHandler()` then sets that name as the `save_handler` value in the `$options` property before returning. I have added a test to verify this behavior based on an example provided in #98. --- src/Config/SessionConfig.php | 153 ++++++++++++++++++++---------- test/Config/SessionConfigTest.php | 22 +++++ 2 files changed, 124 insertions(+), 51 deletions(-) diff --git a/src/Config/SessionConfig.php b/src/Config/SessionConfig.php index 3b74a202..dc5d3ff3 100644 --- a/src/Config/SessionConfig.php +++ b/src/Config/SessionConfig.php @@ -42,7 +42,11 @@ class SessionConfig extends StandardConfig protected $rememberMeSeconds = 1209600; // 2 weeks /** - * @var string + * Name of the save handler currently in use. This will either be a PHP + * built-in save handler name, or the name of a SessionHandlerInterface + * class being used as a save handler. + * + * @var null|string */ protected $saveHandler; @@ -85,6 +89,24 @@ class SessionConfig extends StandardConfig */ protected $validHashFunctions; + /** + * Override standard option setting. + * + * Provides an overload for setting the save handler. + * + * {@inheritDoc} + */ + public function setOption($option, $value) + { + switch (strtolower($option)) { + case 'save_handler': + $this->setPhpSaveHandler($value); + return $this; + default: + return parent::setOption($option, $value); + } + } + /** * Set storage option in backend configuration store * @@ -176,56 +198,8 @@ public function setSaveHandler($phpSaveHandler) */ public function setPhpSaveHandler($phpSaveHandler) { - $knownHandlers = $this->locateRegisteredSaveHandlers(); - - if (in_array($phpSaveHandler, $knownHandlers, true)) { - set_error_handler([$this, 'handleError']); - session_module_name($phpSaveHandler); - restore_error_handler(); - if ($this->phpErrorCode >= E_WARNING) { - throw new Exception\InvalidArgumentException(sprintf( - 'Error setting session save handler module "%s": %s', - $phpSaveHandler, - $this->phpErrorMessage - )); - } - - $this->saveHandler = $phpSaveHandler; - $this->setOption('save_handler', $phpSaveHandler); - return $this; - } - - if (is_string($phpSaveHandler) - && (! class_exists($phpSaveHandler) - || ! (in_array(SessionHandlerInterface::class, class_implements($phpSaveHandler))) - ) - ) { - throw new Exception\InvalidArgumentException(sprintf( - 'Invalid save handler specified ("%s"); must be one of [%s]' - . ' or a class implementing %s', - $phpSaveHandler, - implode(', ', $knownHandlers), - SessionHandlerInterface::class, - SessionHandlerInterface::class - )); - } - - if (is_string($phpSaveHandler)) { - $phpSaveHandler = new $phpSaveHandler(); - } - - if (! $phpSaveHandler instanceof SessionHandlerInterface) { - throw new Exception\InvalidArgumentException(sprintf( - 'Invalid save handler specified ("%s"); must implement %s', - get_class($phpSaveHandler), - SessionHandlerInterface::class - )); - } - - session_set_save_handler($phpSaveHandler); - - $this->saveHandler = get_class($phpSaveHandler); - $this->setOption('save_handler', $this->saveHandler); + $this->saveHandler = $this->performSaveHandlerUpdate($phpSaveHandler); + $this->options['save_handler'] = $this->saveHandler; return $this; } @@ -429,6 +403,83 @@ private function locateRegisteredSaveHandlers() return $this->knownSaveHandlers; } + /** + * Perform a session.save_handler update. + * + * Determines if the save handler represents a PHP built-in + * save handler, and, if so, passes that value to session_module_name + * in order to activate it. The save handler name is then returned. + * + * If it is not, it tests to see if it is a SessionHandlerInterface + * implementation. If the string is a class implementing that interface, + * it creates an instance of it. In such cases, it then calls + * session_set_save_handler to activate it. The class name of the + * handler is returned. + * + * In all other cases, an exception is raised. + * + * @param string|SessionHandlerInterface $phpSaveHandler + * @return string + * @throws Exception\InvalidArgumentException if an error occurs when + * setting a PHP session save handler module. + * @throws Exception\InvalidArgumentException if the $phpSaveHandler + * is a string that does not represent a class implementing + * SessionHandlerInterface. + * @throws Exception\InvalidArgumentException if $phpSaveHandler is + * a non-string value that does not implement SessionHandlerInterface. + */ + private function performSaveHandlerUpdate($phpSaveHandler) + { + $knownHandlers = $this->locateRegisteredSaveHandlers(); + + if (in_array($phpSaveHandler, $knownHandlers, true)) { + $phpSaveHandler = strtolower($phpSaveHandler); + set_error_handler([$this, 'handleError']); + session_module_name($phpSaveHandler); + restore_error_handler(); + if ($this->phpErrorCode >= E_WARNING) { + throw new Exception\InvalidArgumentException(sprintf( + 'Error setting session save handler module "%s": %s', + $phpSaveHandler, + $this->phpErrorMessage + )); + } + + return $phpSaveHandler; + } + + if (is_string($phpSaveHandler) + && (! class_exists($phpSaveHandler) + || ! (in_array(SessionHandlerInterface::class, class_implements($phpSaveHandler))) + ) + ) { + throw new Exception\InvalidArgumentException(sprintf( + 'Invalid save handler specified ("%s"); must be one of [%s]' + . ' or a class implementing %s', + $phpSaveHandler, + implode(', ', $knownHandlers), + SessionHandlerInterface::class, + SessionHandlerInterface::class + )); + } + + if (is_string($phpSaveHandler)) { + $phpSaveHandler = new $phpSaveHandler(); + } + + if (! $phpSaveHandler instanceof SessionHandlerInterface) { + throw new Exception\InvalidArgumentException(sprintf( + 'Invalid save handler specified ("%s"); must implement %s', + get_class($phpSaveHandler), + SessionHandlerInterface::class + )); + } + + session_set_save_handler($phpSaveHandler); + + return get_class($phpSaveHandler); + } + /** * Grab module information from phpinfo. * diff --git a/test/Config/SessionConfigTest.php b/test/Config/SessionConfigTest.php index 85697e46..5486238c 100644 --- a/test/Config/SessionConfigTest.php +++ b/test/Config/SessionConfigTest.php @@ -1206,4 +1206,26 @@ public function testProvidingValidKnownSessionHandlerToSetPhpSaveHandlerResultsI $this->assertSame($this->config, $this->config->setPhpSaveHandler('unittest')); $this->assertEquals('unittest', $this->config->getOption('save_handler')); } + + public function testCanProvidePathWhenUsingRedisSaveHandler() + { + $phpinfo = $this->getFunctionMock('Zend\Session\Config', 'phpinfo'); + $phpinfo + ->expects($this->once()) + ->will($this->returnCallback(function () { + echo "Registered save handlers => user files redis"; + })); + + $sessionModuleName = $this->getFunctionMock('Zend\Session\Config', 'session_module_name'); + $sessionModuleName + ->expects($this->once()) + ->with($this->equalTo('redis')); + + $url = 'tcp://localhost:6379?auth=foobar&database=1'; + + $this->config->setOption('save_handler', 'redis'); + $this->config->setOption('save_path', $url); + + $this->assertSame($url, $this->config->getOption('save_path')); + } } From c323a94c0546d1bb33706c5d063180d048204bce Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 1 Dec 2017 10:29:06 -0600 Subject: [PATCH 2/4] Mark as conflicting with phpunit 6.5+ PHPUnit 6.5+ updates to use php-mock-objects ^5.0. Unfortunately, phpunit still typehints against interfaces defined in the v4 release, causing breakage when using the invocation builder to setup your mock expectations. --- composer.json | 5 ++++- composer.lock | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 41700bfb..2ebd0e64 100644 --- a/composer.json +++ b/composer.json @@ -24,7 +24,7 @@ "container-interop/container-interop": "^1.1", "mongodb/mongodb": "^1.0.1", "php-mock/php-mock-phpunit": "^1.1.2 || ^2.0", - "phpunit/phpunit": "^5.7.15 || ^6.0.8", + "phpunit/phpunit": "^5.7.5 || ^6.0.8", "zendframework/zend-cache": "^2.6.1", "zendframework/zend-coding-standard": "~1.0.0", "zendframework/zend-db": "^2.7", @@ -32,6 +32,9 @@ "zendframework/zend-servicemanager": "^2.7.5 || ^3.0.3", "zendframework/zend-validator": "^2.6" }, + "conflict": { + "phpunit/phpunit": ">=6.5.0" + }, "suggest": { "mongodb/mongodb": "If you want to use the MongoDB session save handler", "zendframework/zend-cache": "Zend\\Cache component", diff --git a/composer.lock b/composer.lock index 7904849e..7024422b 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file", "This file is @generated automatically" ], - "content-hash": "097d88139a812af12fd8e367091f80d1", + "content-hash": "67202c7721a11f3806f51d19696e44a7", "packages": [ { "name": "zendframework/zend-eventmanager", From acff5d9dbab1a67cb97920a74064d43c8f99b540 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 1 Dec 2017 10:40:20 -0600 Subject: [PATCH 3/4] Updates minimum supported PHPUnit 6.X version 6.0.13 has fixes necessary to work with PHP 7.2. Also sets that as the minimum version PHP 7.2 will install when running the 7.2-lowest target. --- .travis.yml | 1 + composer.json | 2 +- composer.lock | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 7f063528..2b02cdce 100644 --- a/.travis.yml +++ b/.travis.yml @@ -65,6 +65,7 @@ before_install: install: - travis_retry composer install $COMPOSER_ARGS --ignore-platform-reqs - if [[ $LEGACY_DEPS != '' ]]; then travis_retry composer update --with-dependencies $COMPOSER_ARGS $LEGACY_DEPS ; fi + - if [[ $DEPS == 'lowest' && $TRAVIS_PHP_VERSION =~ ^7.2 ]]; then travis_retry composer require --dev --no-update phpunit/phpunit:^6.0.13 php-mock/php-mock-phpunit:^2.0; fi - if [[ $DEPS == 'lowest' ]]; then travis_retry composer update --prefer-lowest --prefer-stable $COMPOSER_ARGS ; fi - if [[ $DEPS == 'latest' ]]; then travis_retry composer update $COMPOSER_ARGS ; fi - if [[ $TEST_COVERAGE == 'true' ]]; then travis_retry composer require --dev $COMPOSER_ARGS $COVERAGE_DEPS ; fi diff --git a/composer.json b/composer.json index 2ebd0e64..1653006a 100644 --- a/composer.json +++ b/composer.json @@ -24,7 +24,7 @@ "container-interop/container-interop": "^1.1", "mongodb/mongodb": "^1.0.1", "php-mock/php-mock-phpunit": "^1.1.2 || ^2.0", - "phpunit/phpunit": "^5.7.5 || ^6.0.8", + "phpunit/phpunit": "^5.7.5 || ^6.0.13", "zendframework/zend-cache": "^2.6.1", "zendframework/zend-coding-standard": "~1.0.0", "zendframework/zend-db": "^2.7", diff --git a/composer.lock b/composer.lock index 7024422b..3fd3a4c3 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file", "This file is @generated automatically" ], - "content-hash": "67202c7721a11f3806f51d19696e44a7", + "content-hash": "d00e1bc4c6f2029ba82cfd344c09c999", "packages": [ { "name": "zendframework/zend-eventmanager", From 660a48cd4d73e8da1ef8c3e2b258960789ec4756 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 1 Dec 2017 11:34:56 -0600 Subject: [PATCH 4/4] Adds CHANGELOG entry for #101 --- CHANGELOG.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18d482dc..4b591a7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All notable changes to this project will be documented in this file, in reverse chronological order by release. -## 2.8.3 - TBD +## 2.8.3 - 2017-12-01 ### Added @@ -22,7 +22,10 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#101](https://github.com/zendframework/zend-session/pull/101) fixes an issue + created with the 2.8.2 release with regards to setting a session save path for + non-files save handlers; prior to this patch, incorrect validations were run + on the path provided, leading to unexpected exceptions being raised. ## 2.8.2 - 2017-11-29