apply php 7.2 to travis #92

samsonasik · 2017-10-25T02:06:12Z

No description provided.

samsonasik · 2017-10-25T05:34:33Z

Got following errrors under php 7.2:

There were 6 errors:

1) ZendTest\Session\Config\SessionConfigTest::testSavePathCanBeNonDirectoryWhenSaveHandlerNotFiles

Zend\Session\Exception\InvalidArgumentException: Invalid save handler specified: ini_set(): Cannot set 'user' save handler by ini_set() or session_module_name()

/home/travis/build/zendframework/zend-session/src/Config/SessionConfig.php:148

/home/travis/build/zendframework/zend-session/test/Config/SessionConfigTest.php:68

2) ZendTest\Session\Config\SessionConfigTest::testSaveHandlerIsMutable

ini_set(): Cannot set 'user' save handler by ini_set() or session_module_name()

/home/travis/build/zendframework/zend-session/src/Config/SessionConfig.php:97

/home/travis/build/zendframework/zend-session/src/Config/StandardConfig.php:147

/home/travis/build/zendframework/zend-session/src/Config/StandardConfig.php:912

/home/travis/build/zendframework/zend-session/test/Config/SessionConfigTest.php:105

3) ZendTest\Session\Config\SessionConfigTest::testSaveHandlerAltersIniSetting

ini_set(): Cannot set 'user' save handler by ini_set() or session_module_name()

/home/travis/build/zendframework/zend-session/src/Config/SessionConfig.php:97

/home/travis/build/zendframework/zend-session/src/Config/StandardConfig.php:147

/home/travis/build/zendframework/zend-session/src/Config/StandardConfig.php:912

/home/travis/build/zendframework/zend-session/test/Config/SessionConfigTest.php:111

4) ZendTest\Session\Config\SessionConfigTest::testSetOptionSetsIniSetting with data set #2 ('save_handler', 'getOption', 'user')

ini_set(): Cannot set 'user' save handler by ini_set() or session_module_name()

/home/travis/build/zendframework/zend-session/src/Config/SessionConfig.php:97

/home/travis/build/zendframework/zend-session/test/Config/SessionConfigTest.php:972

5) ZendTest\Session\Config\SessionConfigTest::testSetOptionsTranslatesUnderscoreSeparatedKeys with data set #2 ('save_handler', 'getOption', 'user')

ini_set(): Cannot set 'user' save handler by ini_set() or session_module_name()

/home/travis/build/zendframework/zend-session/src/Config/SessionConfig.php:97

/home/travis/build/zendframework/zend-session/src/Config/StandardConfig.php:147

/home/travis/build/zendframework/zend-session/src/Config/StandardConfig.php:116

/home/travis/build/zendframework/zend-session/test/Config/SessionConfigTest.php:1005

6) ZendTest\Session\SaveHandler\MongoDBTest::testReadDestroysExpiredSession

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time

/home/travis/build/zendframework/zend-session/test/SaveHandler/MongoDBTest.php:97

michalbundyra · 2017-10-25T06:04:56Z

.travis.yml

      env:
        - DEPS=latest
  allow_failures:
-    - php: nightly
+    - php: 7.2


Not sure if I am right, but somewhere, someone secretly decided to not allow failures on PHP 7.2 any more ;-)
@froschdesign Is it right? 😄

@samsonasik and @webimpress
November 30, 2017 is around the corner therefore it make no sense! 😃

@webimpress @froschdesign I've removed allow_failures->php 7.2, do you think we need to keep the "nightly"?

@samsonasik I wouldn't do that as long as we don't have PHP 7.3-alpha yet.

samsonasik · 2017-10-25T06:23:24Z

So, any suggestion to get the error gone in php 7.2 ?

michalbundyra · 2017-10-25T06:34:06Z

@samsonasik I'd suggest to try session_set_save_handler() method, because error message says:

Cannot set 'user' save handler by ini_set() or session_module_name()

michalbundyra · 2017-10-25T06:35:31Z

@samsonasik Hm... oh, no... sorry. No idea for now. This is not gonna work.

remicollet · 2017-11-08T14:51:11Z

Same issue from Fedora QA
https://apps.fedoraproject.org/koschei/package/php-zendframework-zend-session?collection=f28

apply php 7.2 to travis

weierophinney · 2017-11-28T16:30:30Z

This is a case where the tests are incorrect; the value "user" was never correct, but previous versions of PHP did not raise an error.

I'll update to provide a mock SessionHandlerInterface implementation.

This patch takes the following approach: - If "files" is provided in _any_ PHP version, it continues to use `ini_set`. - If "user" is provided in PHP versions prior to 7.2, it continues to use `ini_set`. - Otherwise, it checks to see if the handler provided is a class or instance that implements PHP's `SessionHandlerInterface`; if so, it uses `session_set_save_handler()` with that instance (instantiating it if it is a class name). - It no longer uses `ini_get()` to retrieve the save handler configuration _unless_ it has not been memoized in the instance. This approach works cross-platform.

michalbundyra · 2017-11-28T18:00:02Z

src/Config/SessionConfig.php

+     */
+    public function setSaveHandler($phpSaveHandler)
+    {
+        // error_log(sprintf('In %s with %s', __METHOD__, var_export($phpSaveHandler, true)));


Do we need this comment above?

I'll remove; thanks!

michalbundyra · 2017-11-28T18:00:25Z

src/SessionManager.php

+            // ArrayUtils::merge parameter
+            if ($oldSessionData instanceof Storage\StorageInterface) {
+                $oldSessionData = $oldSessionData->toArray();
+            } elseif ($oldSessionData instanceof \Traversable) {


Traversable is imported

michalbundyra · 2017-11-28T18:00:49Z

src/SessionManager.php

+            if ($oldSessionData instanceof Storage\StorageInterface) {
+                $oldSessionData = $oldSessionData->toArray();
+            } elseif ($oldSessionData instanceof \Traversable) {
+                $oldSessionData = iterator_to_array($oldSessionData);


I'm not sure if it is sufficient - because it is not recursive.

michalbundyra · 2017-11-28T18:01:09Z

src/SessionManager.php

-        if ($oldSessionData instanceof \Traversable
-            || (! empty($oldSessionData) && is_array($oldSessionData))
-        ) {
+        if ($oldSessionData instanceof Traversable) {


As noted by @Xerkus in separate PR this is no longer needed

michalbundyra · 2017-11-28T18:01:36Z

src/SessionManager.php

+            $oldSessionData = $oldSessionData->toArray();
+        }
+
+        if (! empty($oldSessionData) && is_array($oldSessionData)) {


! empty is useless

- PHP 7+ environments have it by default, but it's out-of-date - PHP 5 environments have ext/mongo but not ext/mongodb, so we need to install it. `pecl upgrade` will install if it's not present, and upgrade otherwise. Once installed, we need to enable it if it is not already.

Test case fails on PHP 7.2 due to headers being sent; run in separate process.

weierophinney · 2017-11-28T19:48:56Z

Single test failure is on 5.6-locked, where the MongoDb SaveHandler implementation's garbage collection test reliably fails; passes on all other environments, however, so I'm going to take it as a pass.

Forward port #92

weierophinney · 2017-11-28T19:50:58Z

Thanks, @samsonasik, for getting this started!

Per zendframework#97, the fix in 2.8.1 (provided by zendframework#92) for having `SessionConfig::setPhpSaveHandler()` work with PHP 7.2 was too aggressive, and prevented the ability to set the value to a known extension, such as redis. This patch fixes that approach by grabbing a list of known save handlers from `phpinfo()` (after first stripping `user` from the list), and comparing the incoming value to that list; if it is in that list, it now uses `session_module_name()` to set the value. Tests for this are next to impossible to write, unfortunately, as we would need to have multiple extensions installed on developer machines and in Travis. I have instead added tests verifying the various exceptions thrown, including one thrown when a value is in the known PHP save handlers list, but fails to be set properly.

apply php 7.2 to travis

8e41582

michalbundyra reviewed Oct 25, 2017

View reviewed changes

froschdesign added the enhancement label Oct 25, 2017

removed allow_failures -> php 7.2

e4022e3

Merge pull request zendframework#92 from samsonasik/patch-1

1a32ff7

apply php 7.2 to travis

michalbundyra reviewed Nov 28, 2017

View reviewed changes

weierophinney force-pushed the patch-1 branch 5 times, most recently from 37571c5 to 76d30d7 Compare November 28, 2017 18:32

weierophinney force-pushed the patch-1 branch from 76d30d7 to 69eb479 Compare November 28, 2017 18:39

Ensure headers are not sent for test case

e3f9460

Test case fails on PHP 7.2 due to headers being sent; run in separate process.

weierophinney added a commit that referenced this pull request Nov 28, 2017

Adds CHANGELOG entry for #92

335537e

weierophinney closed this in 440a4f7 Nov 28, 2017

weierophinney added a commit that referenced this pull request Nov 28, 2017

Merge branch 'hotfix/92' into develop

6fac22e

Forward port #92

weierophinney mentioned this pull request Nov 29, 2017

Allow using any configured save handler extension #99

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apply php 7.2 to travis #92

apply php 7.2 to travis #92

samsonasik commented Oct 25, 2017

samsonasik commented Oct 25, 2017

michalbundyra Oct 25, 2017

froschdesign Oct 25, 2017

samsonasik Oct 25, 2017

michalbundyra Oct 25, 2017

samsonasik commented Oct 25, 2017

michalbundyra commented Oct 25, 2017

michalbundyra commented Oct 25, 2017

remicollet commented Nov 8, 2017

weierophinney commented Nov 28, 2017

michalbundyra Nov 28, 2017

weierophinney Nov 28, 2017

michalbundyra Nov 28, 2017

michalbundyra Nov 28, 2017

michalbundyra Nov 28, 2017

michalbundyra Nov 28, 2017

weierophinney commented Nov 28, 2017

weierophinney commented Nov 28, 2017

apply php 7.2 to travis #92

apply php 7.2 to travis #92

Conversation

samsonasik commented Oct 25, 2017

samsonasik commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik commented Oct 25, 2017

michalbundyra commented Oct 25, 2017

michalbundyra commented Oct 25, 2017

remicollet commented Nov 8, 2017

weierophinney commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Nov 28, 2017

weierophinney commented Nov 28, 2017