Fix support for PHP 7.2 #23711

Simperfit · 2017-07-29T09:16:44Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#23671
License	MIT
Doc PR	-

There are still the deprecation problem with phpunit since it use each().

There are 3 tests linked to session that I don't know how to fix / what to do, do you have any idea @nicolas-grekas ?

nicolas-grekas · 2017-07-29T09:54:48Z

.travis.yml

@@ -31,6 +31,8 @@ matrix:
          env: deps=high
        - php: 7.1
          env: deps=low
+        - php: 7.2
+          env: deps=high


no need to add a new section, 7.1 should be changed to 7.2 just above instead, deps=low

Simperfit · 2017-08-01T16:09:34Z

@nicolas-grekas
I've got some ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
on the \Symfony\Component\HttpFoundation\Tests\Session\Storage\Handler\PdoSessionHandlerTest::testSessionGC

Could you give me insight on how to fix this ?

I need to look for the seg fault too.

nicolas-grekas · 2017-08-01T16:44:27Z

Dunno sorry...

Simperfit · 2017-08-09T14:23:26Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php

        $storage->write('gc_id', 'data');
        $storage->close();
        $this->assertEquals(2, $pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'No session pruned because gc not called');
+        // @TODO: Really not sure about this at all....
+        true === headers_sent() && $storage->destroy('gc_id');


I guess @stof or @ogizanagi could know if there is something better then this dirty hack ?

Taluu · 2017-08-25T20:04:04Z

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

@@ -348,7 +352,7 @@ public function setOptions(array $options)
        ));

        foreach ($options as $key => $value) {
-            if (isset($validOptions[$key])) {
+            if (isset($validOptions[$key]) && false === headers_sent()) {


imo you should test for (PHP_VERSION_ID < 70200 || false=== headers_sent()), in order to keep the original (even flawed) behavior...

But that may be dirty actually.

I think it's actually a little dirty. Then we should do that in every modified component

stof · 2017-08-31T13:42:36Z

.github/build-packages.php

 $packages = array();
 $flags = \PHP_VERSION_ID >= 50400 ? JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE : 0;
-


please revert this. These lines were here for readability

stof · 2017-08-31T13:43:28Z

.travis.yml

@@ -141,7 +141,7 @@ install:
          export SYMFONY_DEPRECATIONS_HELPER=weak &&
          cp composer.json composer.json.orig &&
          echo -e '{\n"require":{'"$(grep phpunit-bridge composer.json)"'"php":"*"},"minimum-stability":"dev"}' > composer.json &&
-          php .github/build-packages.php HEAD^ $COMPONENTS &&
+          (php .github/build-packages.php HEAD^ $COMPONENTS || true) &&


why ? If this fails, the build will be garbage

Because it does trigger a fatal on git and because of that trigger a fatal on php

If we have a better way for this, maybe by fixing github/build-packager.php:11

$mergeBase = trim(shell_exec(sprintf('git merge-base %s HEAD', array_shift($dirs))));

that triggers a git fatal then a seg fault than a exit 139 which lead to a build failure

I'm trying to have the test running and we should be discussing this

/home/travis/.travis/job_stages: line 57: 14926 Segmentation fault (core dumped) $COMPOSER_UP The command "if [[ ! $skip ]]; then $COMPOSER_UP; fi" failed and exited with 139 during .

stof · 2017-08-31T13:44:08Z

src/Symfony/Component/Form/Tests/Extension/Csrf/CsrfProvider/LegacyDefaultCsrfProviderTest.php

-        ini_set('session.save_handler', 'files');
-        ini_set('session.save_path', sys_get_temp_dir());
+        //ini_set('session.save_handler', 'files');
+        //ini_set('session.save_path', sys_get_temp_dir());


code should not be commented. If it is not needed anymore, it should be removed

stof · 2017-08-31T13:45:30Z

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

@@ -348,7 +352,7 @@ public function setOptions(array $options)
        ));

        foreach ($options as $key => $value) {
-            if (isset($validOptions[$key])) {
+            if (isset($validOptions[$key]) && false === headers_sent()) {


this should not be checked for each option, but only once before the loop.
And it should use !header_sent()

stof · 2017-08-31T13:46:52Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php

@@ -282,18 +282,20 @@ public function testSessionGC()

        $storage->open('', 'sid');
        $storage->read('gc_id');
-        ini_set('session.gc_maxlifetime', -1); // test that you can set lifetime of a session after it has been read
+        // IN 7.2 this does not work


if this test does not work in PPH 7.2, this case should be extracted to a separate test and be skipped when unapplicable instead of skipping one step (and ensuring other things still work)

stof · 2017-08-31T13:48:06Z

.travis.yml

+      #  - php: 5.3
+       # - php: 5.4
+        #- php: 5.5
+        #- php: 5.6


Must be reverted

This will be reverted

stof · 2017-08-31T13:48:33Z

.travis.yml

        - php: 7.0
-          env: deps=high


must be reverted

stof · 2017-08-31T13:49:11Z

.travis.yml

        - php: 7.0
-          env: deps=high
-        - php: 7.1


We need to keep a PHP 7.1 job (even though it is not one doing a special handling of deps). Otherwise, Symfony would be untested on 7.1, which is not acceptable.

stof · 2017-08-31T13:49:30Z

src/Symfony/Component/HttpFoundation/composer.json

@@ -35,4 +35,4 @@
            "dev-master": "2.7-dev"
        }
    }
-}
+}


must be reverted

stof · 2017-08-31T13:49:57Z

src/Symfony/Component/HttpKernel/Tests/EventListener/LocaleListenerTest.php

@@ -38,7 +38,7 @@ public function testDefaultLocaleWithoutSession()
    public function testLocaleFromRequestAttribute()
    {
        $request = Request::create('/');
-        session_name('foo');
+        //        session_name('foo');


commenting is a no-go. If we don't want this code anymore, it should be removed.

I know that, I think it's like you said, all of this should be skipped on 7.2

Simperfit · 2017-08-31T14:14:32Z

Thanks @stof for the review ill do the changes

Simperfit · 2017-09-01T07:54:50Z

Ok, I couldn't add || true on the phpunit running that would be wrong. But everything is triggering a segfault.

@remicollet maybe you have an idea ?

stof · 2017-09-01T10:48:36Z

@Simperfit please avoid mentioning me in your commit message when fixing the review. This sends me a notification when pushing the commit to Github. And anytime you amend or rebase the commit, this creates a new commit, and so pushing it sends me another notification.

stof · 2017-09-01T10:50:52Z

.travis.yml

-    - if [[ ! $skip ]]; then $COMPOSER_UP; fi
-    - if [[ ! $skip ]]; then ./phpunit install; fi
+    - if [[ ! $skip ]]; then $COMPOSER_UP || true; fi
+    - if [[ ! $skip ]]; then ./phpunit install || true; fi


Adding || true is totally a no-go. If the setup of the testsuite fails, we don't want to ignore the failure and try the next step. This would just lead to an output impossible to understand due to weird failures later (trying to use the packages which were not installed properly before)

Do you know how we could debug this segmentation fault ? I think something needs to be done globally in the script since it does not work even adding OR true

stof · 2017-09-01T10:51:25Z

.travis.yml

    - |
      # phpinfo
-      if [[ ! $PHP = hhvm* ]]; then php -i; else hhvm --php -r 'print_r($_SERVER);print_r(ini_get_all());'; fi
+      if [[ ! $PHP = hhvm* ]]; then php -i || true; else hhvm --php -r 'print_r($_SERVER);print_r(ini_get_all());'; fi


does php -i really fail ? If yes, it means that the PHP shipped in Travis is totally broken, and there is no hope.

it does not fail since it shows the phpinfo(); but it seems that it triggers a segfault

stof · 2017-09-01T10:52:12Z

src/Symfony/Component/Form/Tests/Extension/Csrf/CsrfProvider/LegacyDefaultCsrfProviderTest.php

@@ -25,8 +25,10 @@ class LegacyDefaultCsrfProviderTest extends TestCase

    public static function setUpBeforeClass()
    {
-        ini_set('session.save_handler', 'files');
-        ini_set('session.save_path', sys_get_temp_dir());
+        if (PHP_VERSION_ID < 70200) {


PHP_VERSION_ID should be fully-qualified instead of relying on constant fallback, to allow resolving the version check at compilation time.

What do you mean ? (PHP_VERSION maybe ?)

whoops, understood

stof · 2017-09-01T10:52:48Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php

@@ -271,6 +271,10 @@ public function testSessionDestroy()

    public function testSessionGC()
    {
+        if (PHP_VERSION_ID === 70200) {


this would make the test run again on 7.2.1. Are you sure about it ?

stof · 2017-09-01T10:53:32Z

src/Symfony/Component/Security/Csrf/Tests/TokenStorage/NativeSessionTokenStorageTest.php

-        ini_set('session.save_handler', 'files');
-        ini_set('session.save_path', sys_get_temp_dir());
+        //ini_set('session.save_handler', 'files');
+        //ini_set('session.save_path', sys_get_temp_dir());


commenting code is still a no-go

will remove

@paul-m

…ethod (nicolas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [Bridge\PhpUnit] Fix infinite loop when running isolated method | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - May unlock #23003 (ping @paul-m) And required to make #23711 pass. Commits ------- e07c2f1 [Bridge\PhpUnit] Fix infinite loop when running isolated method

nicolas-grekas · 2017-10-09T18:17:24Z

Tests are now green and PR is ready.

Simperfit · 2017-10-09T19:03:41Z

thanks for the help !

fabpot · 2017-10-10T04:00:44Z

Thank you @Simperfit.

@nicolas-grekas

This PR was merged into the 2.7 branch. Discussion ---------- Fix support for PHP 7.2 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #23671 | License | MIT | Doc PR | - There are still the deprecation problem with phpunit since it use `each()`. There are 3 tests linked to session that I don't know how to fix / what to do, do you have any idea @nicolas-grekas ? Commits ------- fdf285b Fix 7.2 compat layer e229dd0 Fix PHP 7.2 support

sroze · 2017-11-13T13:24:19Z

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

@@ -383,6 +394,10 @@ public function setSaveHandler($saveHandler = null)
            throw new \InvalidArgumentException('Must be instance of AbstractProxy or NativeSessionHandler; implement \SessionHandlerInterface; or be null.');
        }

+        if (headers_sent($file, $line)) {
+            throw new \RuntimeException(sprintf('Failed to set the session handler because headers have already been sent by "%s" at line %d.', $file, $line));


@Simperfit @nicolas-grekas why do we throw in here while we return in the other cases? This definitely smells BC break 🤔

carsonbot added Status: Needs Review Deprecation labels Jul 29, 2017

Simperfit changed the base branch from master to 2.7 July 29, 2017 09:16

Simperfit force-pushed the feature/fix-test-for-7-2 branch from 57d52ed to e041175 Compare July 29, 2017 09:18

nicolas-grekas changed the title ~~feature: fix symfony test for php 7.2~~ Fix support for PHP 7.2 Jul 29, 2017

nicolas-grekas added Bug and removed Deprecation labels Jul 29, 2017

nicolas-grekas reviewed Jul 29, 2017

View reviewed changes

Simperfit force-pushed the feature/fix-test-for-7-2 branch from 9c44ac7 to 28ae236 Compare July 29, 2017 10:08

Simperfit changed the title ~~Fix support for PHP 7.2~~ Fix support for PHP 7.2 on 2.7 Jul 29, 2017

xabbuh added this to the 2.7 milestone Aug 1, 2017

Simperfit force-pushed the feature/fix-test-for-7-2 branch from 28ae236 to fe1e9f2 Compare August 1, 2017 16:10

Simperfit force-pushed the feature/fix-test-for-7-2 branch from fe1e9f2 to 5dfd95a Compare August 9, 2017 14:21

Simperfit commented Aug 9, 2017

View reviewed changes

Simperfit force-pushed the feature/fix-test-for-7-2 branch from 5dfd95a to 05017ef Compare August 24, 2017 14:37

Taluu reviewed Aug 25, 2017

View reviewed changes

Simperfit force-pushed the feature/fix-test-for-7-2 branch 2 times, most recently from d3d321a to 49302f5 Compare August 31, 2017 12:08

stof requested changes Aug 31, 2017

View reviewed changes

Simperfit force-pushed the feature/fix-test-for-7-2 branch 3 times, most recently from eaa77c6 to 5d4838b Compare September 1, 2017 07:53

Simperfit force-pushed the feature/fix-test-for-7-2 branch from 5d4838b to e16c012 Compare September 1, 2017 08:08

stof requested changes Sep 1, 2017

View reviewed changes

Simperfit force-pushed the feature/fix-test-for-7-2 branch from e16c012 to 40a1c7d Compare September 7, 2017 09:50

nicolas-grekas force-pushed the feature/fix-test-for-7-2 branch 3 times, most recently from 5f48ccd to 25f9519 Compare October 9, 2017 11:24

nicolas-grekas mentioned this pull request Oct 9, 2017

[Bridge\PhpUnit] Fix infinite loop when running isolated method #24498

Merged

nicolas-grekas force-pushed the feature/fix-test-for-7-2 branch from 25f9519 to b0e92fc Compare October 9, 2017 14:49

nicolas-grekas force-pushed the feature/fix-test-for-7-2 branch 8 times, most recently from cf43c55 to 306cd02 Compare October 9, 2017 17:55

Fix 7.2 compat layer

fdf285b

nicolas-grekas force-pushed the feature/fix-test-for-7-2 branch from 306cd02 to fdf285b Compare October 9, 2017 18:05

fabpot approved these changes Oct 10, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 10, 2017

fabpot merged commit fdf285b into symfony:2.7 Oct 10, 2017

anlutro mentioned this pull request Oct 11, 2017

[HttpFoundation] Failed to set session handler, headers already sent by phpunit #24524

Closed

This was referenced Nov 10, 2017

Release v2.7.36 #24914

Merged

Release v2.8.29 #24915

Merged

Release v3.3.11 #24916

Merged

sroze reviewed Nov 13, 2017

View reviewed changes

sroze mentioned this pull request Nov 13, 2017

[HttpFoundation] Do not throw when setSaveHandler is called and headers have already been sent #24947

Closed

		$packages = array();
		$flags = \PHP_VERSION_ID >= 50400 ? JSON_PRETTY_PRINT \| JSON_UNESCAPED_SLASHES \| JSON_UNESCAPED_UNICODE : 0;

Fix support for PHP 7.2 #23711

Fix support for PHP 7.2 #23711

Conversation

Simperfit commented Jul 29, 2017 • edited by nicolas-grekas Loading

Choose a reason for hiding this comment

Simperfit commented Aug 1, 2017 • edited Loading

nicolas-grekas commented Aug 1, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Aug 31, 2017

Simperfit commented Sep 1, 2017

stof commented Sep 1, 2017

Choose a reason for hiding this comment

Simperfit Sep 7, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 9, 2017

Simperfit commented Oct 9, 2017

fabpot commented Oct 10, 2017

sroze Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Simperfit commented Jul 29, 2017 •

edited by nicolas-grekas

Loading

Simperfit commented Aug 1, 2017 •

edited

Loading

Simperfit Sep 7, 2017 •

edited

Loading

sroze Nov 13, 2017 •

edited

Loading