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 forward-compat of NativeSessionStorage with PHP 7.2 #24531

Merged
merged 1 commit into from Nov 5, 2017

Conversation

Projects
None yet
6 participants
@sroze
Member

sroze commented Oct 12, 2017

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.

sroze added a commit to sroze/symfony that referenced this pull request Oct 12, 2017

@Simperfit

Please look at the appveyor build the constant seems undefined

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Oct 12, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 13, 2017

Member

Status: needs work

Member

nicolas-grekas commented Oct 13, 2017

Status: needs work

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 14, 2017

Member

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:

  1. Instead of ignoring, we will send a deprecation on 3.4
  2. Instead of the deprecation, we will throw an exception on 4.0
Member

sroze commented Oct 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:

  1. Instead of ignoring, we will send a deprecation on 3.4
  2. Instead of the deprecation, we will throw an exception on 4.0
@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 14, 2017

Member

Status: Needs review

ping @nicolas-grekas

Member

sroze commented Oct 14, 2017

Status: Needs review

ping @nicolas-grekas

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 14, 2017

Member

And when merged we should finally be able to get green tests with PHP 7.2 in #24516.

Member

sroze commented Oct 14, 2017

And when merged we should finally be able to get green tests with PHP 7.2 in #24516.

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 23, 2017

Member

👋 @symfony/mergers should we go ahead with this first PR for PHP 7.2 support (i.e. green tests) ?

Member

sroze commented Oct 23, 2017

👋 @symfony/mergers should we go ahead with this first PR for PHP 7.2 support (i.e. green tests) ?

@nicolas-grekas nicolas-grekas self-requested a review Oct 28, 2017

@fabpot

fabpot approved these changes Nov 5, 2017

@nicolas-grekas nicolas-grekas changed the title from [HttpFundation] Create the native session storage with PHP 7.2 to [HttpFoundation] Fix forward-compat of NativeSessionStorage with PHP 7.2 Nov 5, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 5, 2017

Member

Thank you @sroze.

Member

nicolas-grekas commented Nov 5, 2017

Thank you @sroze.

@nicolas-grekas nicolas-grekas merged commit 00a1357 into symfony:2.7 Nov 5, 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 5, 2017

bug #24531 [HttpFoundation] Fix forward-compat of NativeSessionStorag…
…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

This was referenced Nov 5, 2017

@sroze sroze deleted the sroze:native-storage-and-php72 branch Nov 6, 2017

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Nov 6, 2017

Member

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

Member

sroze commented Nov 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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 6, 2017

Member

@sroze not high priority, but we should try for 4.1 or up

Member

nicolas-grekas commented Nov 6, 2017

@sroze not high priority, but we should try for 4.1 or up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment