-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpFoundation] Fix session-related BC break #24952
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@sroze yes but I prefer this approach, as it's more conservative: the diff with pre-7.2-concerns is much lighter, thus easier to validate. |
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.
Also, it would be nice to add a test for the fixed issue with the null session handler. You've got one you can copy/paste from my other PR.
@@ -340,7 +335,7 @@ public function isStarted() | |||
*/ | |||
public function setOptions(array $options) | |||
{ | |||
if (headers_sent()) { | |||
if (headers_sent() || (\PHP_VERSION_ID >= 50400 && \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.
Could you move that in a private method?
@nicolas-grekas I don't mind either tbh so... 👍 |
I borrowed your test case. |
Could you clarify this one? |
from |
Hehe, true 👍 |
Thank you @sroze. |
…kas, sroze) This PR was merged into the 2.7 branch. Discussion ---------- [HttpFoundation] Fix session-related BC break | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24941, #24934, #24947 and #24946 | License | MIT | Doc PR | - Conservative fix. Commits ------- 38186aa [HttpFoundation] Add test 3eaa188 [HttpFoundation] Fix session-related BC break
Conservative fix.