Skip to content

[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

Merged
merged 2 commits into from
Nov 13, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 13, 2017

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.

@sroze
Copy link
Contributor

sroze commented Nov 13, 2017

See #24947 and #24946. They've been given a 👍 in issues on top of that.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Nov 13, 2017

@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.

Copy link
Contributor

@sroze sroze left a 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())) {
Copy link
Contributor

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?

@sroze
Copy link
Contributor

sroze commented Nov 13, 2017

@nicolas-grekas I don't mind either tbh so... 👍

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Nov 13, 2017

I borrowed your test case.
No need for the private method, that's diff boilerplate to me, and will be reduced to one instruction on 3.3.

@sroze
Copy link
Contributor

sroze commented Nov 13, 2017

will be reduced to one instruction on master.

Could you clarify this one?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Nov 13, 2017

from \PHP_VERSION_ID >= 50400 && \PHP_SESSION_ACTIVE === session_status()
to \PHP_SESSION_ACTIVE === session_status()

@sroze
Copy link
Contributor

sroze commented Nov 13, 2017

Hehe, true 👍

@nicolas-grekas
Copy link
Member Author

Thank you @sroze.

@nicolas-grekas nicolas-grekas merged commit 38186aa into symfony:2.7 Nov 13, 2017
nicolas-grekas added a commit that referenced this pull request Nov 13, 2017
…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
@nicolas-grekas nicolas-grekas deleted the session-fix branch November 13, 2017 18:05
This was referenced Nov 13, 2017
This was referenced Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants