[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 #24531

sroze · 2017-10-12T10:51:37Z

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

PHP 7.2 disallow setting session options when the session was already started. This PR will not set any option if the session is already started and throw an exception if trying to do so with custom options.

stloyd · 2017-10-12T11:01:33Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php

+        $this->getStorage();
+
+        // Assert no exception has been thrown by `getStorage()`
+        $this->assertTrue(true);


$this->addToAssertionCount(1);

What's the difference?

semantic: more explicit

stloyd · 2017-10-12T11:03:13Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php

+     */
+    public function testCannotSetSessionOptionsOnceSessionStarted()
+    {
+        if (PHP_VERSION_ID < 70200) {


Annotation @requires PHP 7.2.0

Would that allow 7.2 and above or lock at 7.2.* or 7.(2+) ?

@requires PHP 7.2 works

Simperfit

Please look at the appveyor build the constant seems undefined

nicolas-grekas · 2017-10-12T15:33:33Z

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

-            session_register_shutdown();
+        $this->setMetadataBag($metaBag);
+
+        if (PHP_VERSION_ID >= 70200 && \PHP_SESSION_ACTIVE === session_status()) {


I think we should try as much as possible to NOT have a different behavior on 7.2 than before.

This could be a BC break if we don't add this version condition as it's a behaviour changed in PHP's runtime.

upgrading to 7.2 will hit the BC break, so this doesn't prevent it, it just hides it :)

Fair enough. So you'd like me to remove the PHP version condition in here? Note that we'll have to add a check to know if the constant exists for old Windows builds as apparently it doesn't exists. (dunno what's the limit but OK for master but not 2.7 on AppVeyor)

should be a no-op then, which matches the behavior pre-7.2
on 3.4, we could trigger a deprecation, and throw on 4.0

Alright, let's do this in three PRs instead of directly throwing on 2.7:

On 2.7, just ignore and don't set the options/handler so it won't break with the INI settings issue

On 3.4, trigger a deprecation

On master, throw an exception

nicolas-grekas · 2017-10-13T13:28:34Z

Status: needs work

sroze · 2017-10-14T13:11:10Z

PR changed to be the first one of three. This one, on 2.7, simply ignores the extra options if the session is already started.

Once merged, I'll issue two other ones:

Instead of ignoring, we will send a deprecation on 3.4
Instead of the deprecation, we will throw an exception on 4.0

sroze · 2017-10-14T13:11:30Z

Status: Needs review

ping @nicolas-grekas

sroze · 2017-10-14T13:13:01Z

And when merged we should finally be able to get green tests with PHP 7.2 in #24516.

sroze · 2017-10-23T08:43:34Z

👋 @symfony/mergers should we go ahead with this first PR for PHP 7.2 support (i.e. green tests) ?

nicolas-grekas · 2017-11-05T16:36:34Z

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

@@ -102,6 +102,12 @@ class NativeSessionStorage implements SessionStorageInterface
     */
    public function __construct(array $options = array(), $handler = null, MetadataBag $metaBag = null)
    {
+        $this->setMetadataBag($metaBag);
+
+        if (PHP_VERSION_ID >= 70200 && \PHP_SESSION_ACTIVE === session_status()) {


I'd suggest to remove the PHP_VERSION_ID >= 70200 part: the behavior should be consistent across versions.

nicolas-grekas · 2017-11-05T16:36:59Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php

+    }
+
+    /**
+     * @requires PHP 7.2


see previous comment, might be removed

nicolas-grekas · 2017-11-05T19:04:56Z

Thank you @sroze.

…e with PHP 7.2 (sroze) This PR was merged into the 2.7 branch. Discussion ---------- [HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24524 | License | MIT | Doc PR | ø PHP 7.2 disallow setting session options when the session was already started. This PR will not set any option if the session is already started and throw an exception if trying to do so with custom options. Commits ------- 00a1357 [HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2

sroze · 2017-11-06T08:22:23Z

@nicolas-grekas thanks for updating. Based on your changes... do you still think that we should deprecate and throw when options are tried to be changed?

nicolas-grekas · 2017-11-06T08:49:04Z

@sroze not high priority, but we should try for 4.1 or up

carsonbot added Status: Needs Review Bug labels Oct 12, 2017

sroze force-pushed the native-storage-and-php72 branch 2 times, most recently from 35d4024 to 426a0cb Compare October 12, 2017 10:53

sroze added a commit to sroze/symfony that referenced this pull request Oct 12, 2017

Remove NativeSessionStorage changes (moved to symfony#24531)

04164e2

sroze mentioned this pull request Oct 12, 2017

[Process] Fix broken tests for PHP 7.2 #24516

Closed

stloyd reviewed Oct 12, 2017

View reviewed changes

Simperfit approved these changes Oct 12, 2017

View reviewed changes

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

Simperfit suggested changes Oct 12, 2017

View reviewed changes

carsonbot added Status: Needs Work Status: Reviewed and removed Status: Reviewed Status: Needs Work labels Oct 12, 2017

Simperfit approved these changes Oct 12, 2017

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Oct 12, 2017

nicolas-grekas reviewed Oct 12, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 13, 2017

sroze force-pushed the native-storage-and-php72 branch from 2409c5a to a930a67 Compare October 14, 2017 13:06

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 14, 2017

nicolas-grekas self-requested a review October 28, 2017 16:35

fabpot approved these changes Nov 5, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 5, 2017

nicolas-grekas reviewed Nov 5, 2017

View reviewed changes

nicolas-grekas force-pushed the native-storage-and-php72 branch from a930a67 to e54c292 Compare November 5, 2017 18:30

nicolas-grekas changed the title ~~[HttpFundation] Create the native session storage with PHP 7.2~~ [HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 Nov 5, 2017

nicolas-grekas force-pushed the native-storage-and-php72 branch 2 times, most recently from 23d917f to 34faff7 Compare November 5, 2017 18:33

nicolas-grekas approved these changes Nov 5, 2017

View reviewed changes

[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2

00a1357

nicolas-grekas force-pushed the native-storage-and-php72 branch from 34faff7 to 00a1357 Compare November 5, 2017 18:48

nicolas-grekas merged commit 00a1357 into symfony:2.7 Nov 5, 2017

This was referenced Nov 5, 2017

Release v3.4.0-BETA3 #24831

Merged

Release v4.0.0-BETA3 #24832

Merged

sroze deleted the native-storage-and-php72 branch November 6, 2017 08:16

garak mentioned this pull request Nov 6, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 #24531

[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 #24531

sroze commented Oct 12, 2017

stloyd Oct 12, 2017

sroze Oct 12, 2017

nicolas-grekas Nov 5, 2017

stloyd Oct 12, 2017

sroze Oct 12, 2017

fabpot Oct 12, 2017

Simperfit left a comment

nicolas-grekas Oct 12, 2017

sroze Oct 12, 2017

nicolas-grekas Oct 12, 2017

sroze Oct 12, 2017 •

edited

Loading

nicolas-grekas Oct 12, 2017

sroze Oct 12, 2017

nicolas-grekas commented Oct 13, 2017

sroze commented Oct 14, 2017

sroze commented Oct 14, 2017

sroze commented Oct 14, 2017

sroze commented Oct 23, 2017 •

edited

Loading

nicolas-grekas Nov 5, 2017

nicolas-grekas Nov 5, 2017

nicolas-grekas commented Nov 5, 2017

sroze commented Nov 6, 2017

nicolas-grekas commented Nov 6, 2017

[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 #24531

[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 #24531

Conversation

sroze commented Oct 12, 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

Simperfit left a 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

sroze Oct 12, 2017 • edited Loading

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 13, 2017

sroze commented Oct 14, 2017

sroze commented Oct 14, 2017

sroze commented Oct 14, 2017

sroze commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 5, 2017

sroze commented Nov 6, 2017

nicolas-grekas commented Nov 6, 2017

sroze Oct 12, 2017 •

edited

Loading

sroze commented Oct 23, 2017 •

edited

Loading