Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[HttpFoundation] Do not return an empty session id if the session was closed #9611

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[HttpFoundation] Do not return an empty session id if the session was closed #9611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion 2 src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public function start()
*/
public function getId()
{
if (!$this->started) {
if (!$this->started && !$this->closed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's protected specifically to allow inheritance, like you can see with the PhpBridgeSessionStorage.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the additional problem with refactoring closed is it would also mean any child classes relying on it will break.

return ''; // returning empty is consistent with session_id() behaviour
}

Expand Down
4 changes: 4 additions & 0 deletions 4 src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,12 @@ public function testGetId()
{
$storage = $this->getStorage();
$this->assertEquals('', $storage->getId());

$storage->start();
$this->assertNotEquals('', $storage->getId());

$storage->save();
$this->assertNotEquals('', $storage->getId());
}

public function testRegenerate()
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.