Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2#24531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
35d4024 to426a0cbCompare| $this->getStorage(); | ||
| // Assert no exception has been thrown by `getStorage()` | ||
| $this->assertTrue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$this->addToAssertionCount(1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
semantic: more explicit
| */ | ||
| publicfunctiontestCannotSetSessionOptionsOnceSessionStarted() | ||
| { | ||
| if (PHP_VERSION_ID <70200) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Annotation@requires PHP 7.2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would that allow 7.2 and above or lock at 7.2.* or 7.(2+) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@requires PHP 7.2 works
Simperfit left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please look at the appveyor build the constant seems undefined
| session_register_shutdown(); | ||
| $this->setMetadataBag($metaBag); | ||
| if (PHP_VERSION_ID >=70200 && \PHP_SESSION_ACTIVE ===session_status()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should try as much as possible to NOT have a different behavior on 7.2 than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This could be a BC break if we don't add this version condition as it's a behaviour changed in PHP's runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
upgrading to 7.2 will hit the BC break, so this doesn't prevent it, it just hides it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedOct 13, 2017
Status: needs work |
2409c5a toa930a67Comparesroze commentedOct 14, 2017
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:
|
sroze commentedOct 14, 2017
Status: Needs review ping@nicolas-grekas |
sroze commentedOct 14, 2017
And when merged we should finally be able to get green tests with PHP 7.2 in#24516. |
sroze commentedOct 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
👋 @symfony/mergers should we go ahead with this first PR for PHP 7.2 support (i.e. green tests) ? |
| { | ||
| $this->setMetadataBag($metaBag); | ||
| if (PHP_VERSION_ID >=70200 && \PHP_SESSION_ACTIVE ===session_status()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest to remove thePHP_VERSION_ID >= 70200 part: the behavior should be consistent across versions.
| } | ||
| /** | ||
| * @requires PHP 7.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see previous comment, might be removed
a930a67 toe54c292Compare23d917f to34faff7Compare34faff7 to00a1357Comparenicolas-grekas commentedNov 5, 2017
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 commentedNov 6, 2017
@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 commentedNov 6, 2017
@sroze not high priority, but we should try for 4.1 or up |
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.