[HttpFoundation] Fixes a possible regression in SessionStorage top classes #9530
Conversation
This regression was introduced by symfony#9246 ; if this PR was to set the session to "not started" was it was saved (and thus closed), for an unknown reason, the PHPSESSID cookie was never created for some apps. But if we modify the isStarted method, and take into account the $this->closed state, then it should enact the sought behaviour without introducing this (possible) regression
|
@Taluu Since we've got it wrong first time. Would it be possible to add a test case to avoid this problem in the future? |
|
I was wondering what tests I should add but I'll try to make something then. Something on the isStarted maybe ? |
|
Hum, adding tests, and I see that on the |
| @@ -162,6 +162,7 @@ public function save() | |||
| if (!$this->started || $this->closed) { | |||
| throw new \RuntimeException("Trying to save a session that was not started yet or was already closed"); | |||
| } | |||
|
|
|||
| // nothing to do since we don't persist the session data | |||
| $this->closed = false; | |||
| $this->started = false; | |||
There was a problem hiding this comment.
As i said in my second commit, I'm not sure if I should take that as a "regression"... As the data is not persisted, there is no other way to know (AFAIK) if the session was "stopped", or if the closed boolean should be turned to true here.
And also revert a part of the first commit on the MockArray storage, due to a comment that could make more sense (never actually "closing" the session). As this is used in unit tests (and functional with its extension) should this point be discussed ?
| @@ -239,7 +239,6 @@ public function save() | |||
| } | |||
|
|
|||
| $this->closed = true; | |||
| $this->started = false; | |||
There was a problem hiding this comment.
Reverting the other PR is not an option as it definitely fixed a bug.
There was a problem hiding this comment.
This is still a regression. With this addition, somehow, the PHPSESSID (default session name) is not sent anymore. To do what the other was trying to do, I changed the condition in the isStarted method, which should do the work that was intended.
Or maybe did I miss something on the usage of this property ?
There was a problem hiding this comment.
ping @fabpot Let me be more clearer ; this "revert" is done to fix the mentionned bug (which was #7936), but for an unknown reason, for some app, the PHPSESSID cookie is never sent (which makes us stuck at 2.2.9).
That is why I tried another approach to solve this bug, without causing the cookie problem : if yiou check if the session is started, instead of just checking the started property, it also tests the closed property of the SessionStorage.
so this "revert" is still fixing the bug... or it would seems, as I don't have any use case to reproduce it.
Maybe @Drak could check up if it seems legit ?
|
@Taluu - If I remember correctly, there is no need to "close" a MockArrayStorage session because there is no session open. Your test should probably be a functional tests using real sessions (with the tests in process isolation). I think there needs to be tests: one showing what was broken was fixed (in the previous PR), one proving the fix has broken something else. Then you need another test to show that has been fixed. The |
|
Hum as I dug a little deeper, maybe the bug fix show another bug. Indeed, as the bug fix #9246 is trying to set that if the session is closed, then it should not be "started". That's fine, but the storage's So I think that the fix I'm proposing here may be not totally correct, and should more focus on the getId. I'll open up a pr (and henceforth close this one), but I don't see where I should add functional tests... ? |
…ession was closed (Taluu) This PR was merged into the 2.2 branch. Discussion ---------- [HttpFoundation] Do not return an empty session id if the session was closed | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7936 | License | MIT | Doc PR | ~ A regression from the bug fix #9246 was introduced due to an incomplete fix ; As the `started` flag on the NativeSessionStorage was not `true` anymore when saving the session, the session id was always empty when saving it, and thus when sending the `PHPSESSID` cookie. The previous PR I made (#9530) was another approach to solve this regression, but this one should keep the fix made by @tecbot on #9246, and finish it with another verification. I may miss another place where `started` is used, but I don't really see which other conditions depending on this property should be altered... **Note : this is #9611 rebased on 2.2** This should be mergeable. Commits ------- 5b9a727 When getting the session's id, check if the session is not closed
This should fix a regression which was introduced by #9246 ; if this PR
was to set the session to "not started" was it was saved (and thus
closed), for an unknown reason, the PHPSESSID cookie was never
created for some apps (aka not the recent ones installed with the latest
versions of symfony).
But if we modify the
isStartedmethod, and take into account the$this->closedstate, then it should enact the sought behaviourwithout introducing this (possible) regression
I pointed this fix to master, but this regression (if this is one) is present since
2.2.10, 2.3.7, and 2.4.0-beta2.