NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 2 commits intosymfony:2.7fromnicolas-grekas:session-fix

Nov 13, 2017

Merged

[HttpFoundation] Fix session-related BC break#24952

nicolas-grekas merged 2 commits intosymfony:2.7fromnicolas-grekas:session-fix

Nov 13, 2017

Conversation

Copy link

Member

nicolas-grekas commentedNov 13, 2017•
edited
Loading

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.

[HttpFoundation] Fix session-related BC break

3eaa188

nicolas-grekas added this to the2.7 milestone

Nov 13, 2017

carsonbot added Status: Needs Review HttpFoundation Bug labels

Nov 13, 2017

Copy link

Contributor

sroze commentedNov 13, 2017

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

Copy link

MemberAuthor

nicolas-grekas commentedNov 13, 2017•
edited
Loading

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

[HttpFoundation] Add test

38186aa

sroze reviewed

Nov 13, 2017

View reviewed changes

Copy link

Contributor

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.

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

		publicfunctionsetOptions(array$options)
		{
		if (headers_sent()) {
		if (headers_sent() \|\| (\PHP_VERSION_ID >=50400 && \PHP_SESSION_ACTIVE ===session_status())) {

Copy link

Contributor

srozeNov 13, 2017

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?

Copy link

Contributor

sroze commentedNov 13, 2017

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

Copy link

MemberAuthor

nicolas-grekas commentedNov 13, 2017•
edited
Loading

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.

Copy link

Contributor

sroze commentedNov 13, 2017

will be reduced to one instruction on master.

Could you clarify this one?

Copy link

MemberAuthor

nicolas-grekas commentedNov 13, 2017•
edited
Loading

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

Copy link

Contributor

sroze commentedNov 13, 2017

Hehe, true 👍

Copy link

MemberAuthor

nicolas-grekas commentedNov 13, 2017

Thank you@sroze.

nicolas-grekas merged commit38186aa intosymfony:2.7

Nov 13, 2017

nicolas-grekas added a commit that referenced this pull request

Nov 13, 2017

bug#24952[HttpFoundation] Fix session-related BC break (nicolas-gre…

70dd46b

…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 test3eaa188 [HttpFoundation] Fix session-related BC break