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] Fixes a possible regression in SessionStorage top classes #9530

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 2 commits into from
Closed

[HttpFoundation] Fixes a possible regression in SessionStorage top classes #9530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc87e65
Fixes regression in SessionStorage top classes
Taluu Nov 19, 2013
fa08c13
Add tests for closing sessions
Taluu Nov 19, 2013
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
1 change: 1 addition & 0 deletions 1 src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Expand Down
3 changes: 1 addition & 2 deletions 3 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 @@ -239,7 +239,6 @@ public function save()
}

$this->closed = true;
$this->started = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Reverting the other PR is not an option as it definitely fixed a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?


/**
Expand Down Expand Up @@ -314,7 +313,7 @@ public function getMetadataBag()
*/
public function isStarted()
{
return $this->started;
return $this->started && !$this->closed;
}

/**
Expand Down
17 changes: 17 additions & 0 deletions 17 src/Symfony/Component/HttpFoundation/Tests/Session/Storage/MockArraySessionStorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,21 @@ public function testUnstartedSave()
{
$this->storage->save();
}

public function testClosedIsStarted()
{
$this->storage->start();

$refl = new \ReflectionProperty($this->storage, 'started');
$refl->setAccessible(true);

$this->assertTrue($this->storage->isStarted());
$this->assertTrue($refl->getValue($this->storage));

$this->storage->save();

// isStarted should return false once the storage saves the session
$this->assertFalse($this->storage->isStarted());
$this->assertFalse($refl->getValue($this->storage));
}
}
18 changes: 18 additions & 0 deletions 18 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 @@ -249,6 +249,24 @@ public function testCanStartOutside54()
$this->assertFalse(isset($_SESSION[$key]));
$storage->start();
}

public function testClosedIsStarted()
{
$storage = $this->getStorage();
$storage->start();

$refl = new \ReflectionProperty($storage, 'started');
$refl->setAccessible(true);

$this->assertTrue($storage->isStarted());
$this->assertTrue($refl->getValue($storage));

$storage->save();

// isStarted should return false once the storage saves the session
$this->assertFalse($storage->isStarted());
$this->assertTrue($refl->getValue($storage));
}
}

class SessionHandler implements \SessionHandlerInterface
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.