[HttpFoundation] Do not return an empty session id if the session was closed #9611
Conversation
This introduced a regression from symfony#9246, with 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
|
👍 |
| @@ -163,7 +163,7 @@ public function start() | ||
| */ | ||
| public function getId() | ||
| { | ||
| if (!$this->started) { | ||
| if (!$this->started && !$this->closed) { |
There was a problem hiding this comment.
I wonder what is the purpose of the closed flag? Under which circumstance can a session be started === true and closed === true at the same time?
There was a problem hiding this comment.
The state you're describing (started === true and closed === true), was effective when saving the session before applying #9246. this behavour was solved, but this method would then return an empty id, thus deleting the session cookie... Which is why I extended this condition to return an empty session id only if the session was never started, not even once.
So that's the point of having the closed flag : to tell the difference for the started flag whether it means the session was already at least started once or never started. If it is true, then that means that the session was started at least once.
There was a problem hiding this comment.
Yes. The only time when you have an empty session ID is when no session has been started, Once it's been started, it can be closed and started again. But in this case you will have a session ID. It's just representing PHP's behaviour.
There was a problem hiding this comment.
Ok. So having two boolean variables suggest there can be 4 different states. But you only described 3.
If there are only 3, then why not remove the closed property and make started = true|false|null (null means never started).
There was a problem hiding this comment.
I think it's really hard to read !$this->started && !$this->closed and all these combinations. Also there does not seem to be a getter for "closed" anyway.
There was a problem hiding this comment.
Point taken ; but I think it could be confusing to have a started property which could be null, false or true... How about changing closed to running and setting it to true when the session is started for the first time ? And leave the started flag as it is, a flag ?
There was a problem hiding this comment.
@Taluu I think it's out of scope of this bug fix. A PR should focus on one task - which in this case is fixing a bug. Refactoring internals is something separate.
@Tobion closed represents private internal state so should not have a getter. To the outside world, the session is either started or not. But internally, it needs to know more in order to be able to replicate PHP's behaviour.
There was a problem hiding this comment.
yes that is why i said it can be implemented differently because its only internal. But why is it protected at all and not private?
There was a problem hiding this comment.
It's protected specifically to allow inheritance, like you can see with the PhpBridgeSessionStorage.
There was a problem hiding this comment.
I guess the additional problem with refactoring closed is it would also mean any child classes relying on it will break.
|
I think this should be mergeable (ping @fabpot) ; As this concern the 2.2, 2.3, 2.4 and master branch, should I leave this PR on master or should I point it to 2.2 ? |
|
This is a bugfix so it should be applied to all versions since the previous "fix". Means you have to rebase to the lowest version branch. |
|
Which is the 2.2, hence my question. Should I open 3 other PR or just open
|
|
Just on |
|
Closing and reopening on 2.2 then |
…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 introduced a regression from #9246, with an incomplete fix ;
As the
startedflag on the NativeSessionStorage was nottrueanymore when saving the session, the session id was always empty when saving it, and thus when sending thePHPSESSIDcookie.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
startedis used, but I don't really see which other conditions depending on this property should be altered...I tried to alter the getId tests on the
NativeSessionStorage, but I think it is missing a functionnal test to show it indeed broke before and is now fixed... Any idea ?ping @Drak, @jakzal
Note : The test are failing due to the StopWatch component (something date related I guess ?)