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

[HttpFoundation] Fix session-related BC break #24952

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@nicolas-grekas
Member

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

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Nov 13, 2017

Member

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

Member

sroze commented Nov 13, 2017

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 13, 2017

Member

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

Member

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.

@sroze

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())) {

This comment has been minimized.

@sroze

sroze Nov 13, 2017

Member

Could you move that in a private method?

@sroze

sroze Nov 13, 2017

Member

Could you move that in a private method?

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Nov 13, 2017

Member

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

Member

sroze commented Nov 13, 2017

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 13, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Nov 13, 2017

Member

will be reduced to one instruction on master.

Could you clarify this one?

Member

sroze commented Nov 13, 2017

will be reduced to one instruction on master.

Could you clarify this one?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 13, 2017

Member

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

Member

nicolas-grekas commented Nov 13, 2017

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

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Nov 13, 2017

Member

Hehe, true 👍

Member

sroze commented Nov 13, 2017

Hehe, true 👍

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 13, 2017

Member

Thank you @sroze.

Member

nicolas-grekas commented Nov 13, 2017

Thank you @sroze.

@nicolas-grekas nicolas-grekas merged commit 38186aa into symfony:2.7 Nov 13, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Nov 13, 2017

bug #24952 [HttpFoundation] Fix session-related BC break (nicolas-gre…
…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 nicolas-grekas:session-fix branch Nov 13, 2017

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