[HttpFoundation] Fix regression of delegation of Session::save() to Storage::Save() #31215
Conversation
nicolas-grekas
changed the title
| @@ -219,6 +219,12 @@ public function regenerate($destroy = false, $lifetime = null) | |||
| */ | |||
| public function save() | |||
| { | |||
| // In PHP <7.2 session_write_close() does not error if the session is | |||
There was a problem hiding this comment.
maybe this should be written as In PHP >=7.2 session_write_close() errors if the session is not started? (single line please)
There was a problem hiding this comment.
I was trying to say that this is following the logic of session_write_close in PHP <7.2 - in PHP >=7.2 it returns FALSE.
However I've just read the docs on the interface which say
/**
* Force the session to be saved and closed.
*
* This method must invoke session_write_close() unless this interface is
* used for a storage object design for unit or functional testing where
* a real PHP session would interfere with testing, in which case
* it should actually persist the session data if required.
*
* @throws \RuntimeException if the session is saved without being started, or if the session
* is already closed
*/
public function save();
So now I'm not sure what's the right thing to do. What's clear is that is not what \Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::save() does in 3.4 HEAD.
| @@ -261,13 +261,4 @@ public function testIsEmpty() | |||
| $this->assertTrue($this->session->isEmpty()); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
extra line should be removed
|
@dmaicher when you have some time I'd be happy to have your go here. |
| @@ -219,6 +219,11 @@ public function regenerate($destroy = false, $lifetime = null) | |||
| */ | |||
| public function save() | |||
| { | |||
| // In PHP <7.2 session_write_close() returns void if the session is not started. | |||
| // @see https://www.php.net/manual/function.session-write-close.php | |||
| if (!$this->started) { | |||
There was a problem hiding this comment.
I thought about this approach as well. I hesitated to go this way because of the phpdoc on the interface:
Not sure if this would not also be a BC break as someone is relying on this? 😕
There was a problem hiding this comment.
Actually it seems this RuntimeException is only really valid for the mock storages?
see #6362
Edit: but anyway this means if a storage is used that throws an exception on save without being started then this would fail now if the kernel is reset on a non-started (but instantiated) session.
There was a problem hiding this comment.
Can we catch that exception in Session::save() and convert it to a warning or deprecation error saying that the storage needs to handle an attempt to save a reset session on its own? The behavior that you describe is what existed prior to this BC break in 3.4.24 for custom storage, prior to that they would have had to handle a reset session on their own anyway.
|
We've decided to revert the original PR instead. Thank you for the report and very sorry for the inconvenience. |
…session service resettable (dmaicher)" (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- Revert "bug #30620 [FrameworkBundle][HttpFoundation] make session service resettable (dmaicher)" | 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 | - This reverts commit 029fb2e, reversing changes made to 9dad29d. Reverts #30620 Replaces #31215 We don't need to solve this in 3.4 Making the session resettable should be done on master, by implementing `ResetInterface`. On 3.4 apps, one should write a dedicated `SessionResetter` that would implement the reverted logic. Commits ------- 4177331 Revert "bug #30620 [FrameworkBundle][HttpFoundation] make session service resettable (dmaicher)"
#30620 caused a regression in Drupal because we have the concept of lazy sessions. We tried to address this by patching Drupal core but we're now finding affects on contributed modules and the wider ecosystem. As per #30620 (comment) this PR implements a slightly different fix which still delegates saving to the storage but allows for the NativeStorage as implemented by Symfony to be resettable.