[HttpFoundation] Add Session::isEmpty(), fix MockFileSessionStorage to behave like the native one #25220
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c54978f to
f635ca2
Compare
nicolas-grekas
commented
Nov 30, 2017
| @@ -29,7 +29,7 @@ | |||
| protected $options = array( | |||
| 'check_path' => '/login_check', | |||
| 'use_forward' => false, | |||
| 'require_previous_session' => true, | |||
| 'require_previous_session' => false, | |||
There was a problem hiding this comment.
The Request::hasPreviousSession() check that this triggers is a micro-optimization that doesn't make sense, and in fact doesn't work anymore now that we destroy any empty session.
The only case were it can make sense is for remember-me strategy, and this is still enabled there as RememberMeFactory replaces this $options array, and the default is true.
stof
approved these changes
Nov 30, 2017
f635ca2 to
bc3593a
Compare
the method is now |
xabbuh
approved these changes
Nov 30, 2017
…o behave like the native one
bc3593a to
56846ac
Compare
nicolas-grekas
added a commit
that referenced
this pull request
Nov 30, 2017
…onStorage to behave like the native one (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [HttpFoundation] Add Session::isEmpty(), fix MockFileSessionStorage to behave like the native one | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - MockFileSessionStorage should not create any file when the session is empty. Like the native session storage, it should ignore the metadataBag to decide if the session is empty. And to prevent AbstractTestSessionListener from registered a wrong cookie, it must have access to this empty state, which is now possible thanks to a new `Session::isEmpty()` method. Implementing is requires access to the internal storage of bags, which is possible via an internal proxy. Commits ------- 56846ac [HttpFoundation] Add Session::isEmpty(), fix MockFileSessionStorage to behave like the native one
jpuck
added a commit
to razorbacks/laravel-shibboleth
that referenced
this pull request
Nov 30, 2017
jpuck
added a commit
to razorbacks/laravel-shibboleth
that referenced
this pull request
Nov 30, 2017
symfony/symfony#25220 solution was to stop testing in processIsolation https://stackoverflow.com/a/38615131/4233593
This was referenced Mar 12, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MockFileSessionStorage should not create any file when the session is empty. Like the native session storage, it should ignore the metadataBag to decide if the session is empty.
And to prevent AbstractTestSessionListener from registering a wrong cookie, it must have access to this empty state, which is now possible thanks to a new
Session::isEmpty()method. Implementing it requires access to the internal storage of bags, which is possible via an internal proxy.