Skip to content
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

Jump to bottom

[HttpFoundation] Fix regression of delegation of Session::save() to Storage::Save() #31215

Closed
wants to merge 3 commits into from
Closed

[HttpFoundation] Fix regression of delegation of Session::save() to Storage::Save() #31215

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa667ce
Fix regression of delegation of Session::save() to Storage::Save()
alexpott Apr 23, 2019
ab78685
Update for @nicolas-grekas's code review
alexpott Apr 23, 2019
27f2615
Update coding standards
alexpott Apr 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/Symfony/Component/HttpFoundation/Session/Session.php
View file
Open in desktop
Expand Up @@ -193,9 +193,7 @@ public function migrate($destroy = false, $lifetime = null)
*/
public function save()
{
if ($this->isStarted()) {
$this->storage->save();
}
$this->storage->save();
}

/**
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
View file
Open in desktop
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about this approach as well. I hesitated to go this way because of the phpdoc on the interface:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/SessionStorageInterface.php#L107

Not sure if this would not also be a BC break as someone is relying on this? 😕

Copy link
Contributor

@dmaicher dmaicher Apr 23, 2019 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return;
}
$session = $_SESSION;

foreach ($this->bags as $bag) {
Expand Down
9 changes: 0 additions & 9 deletions src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php
View file
Open in desktop
Expand Up @@ -261,13 +261,4 @@ public function testIsEmpty()
$this->assertTrue($this->session->isEmpty());
}

Copy link
Member

Choose a reason for hiding this comment

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

extra line should be removed

public function testSaveIfNotStarted()
{
$storage = $this->getMockBuilder('Symfony\Component\HttpFoundation\Session\Storage\SessionStorageInterface')->getMock();
$session = new Session($storage);

$storage->expects($this->once())->method('isStarted')->willReturn(false);
$storage->expects($this->never())->method('save');
$session->save();
}
}
7 changes: 7 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php
View file
Open in desktop
Expand Up @@ -105,6 +105,13 @@ public function testGetId()
$this->assertSame($id, $storage->getId(), 'ID stays after saving session');
}

public function testSaveIfNotStarted()
{
$storage = $this->getStorage();
$this->assertNull($storage->save(), 'Saving unstarted sessions does not error');
$this->assertEmpty(glob($this->savePath.'/*'));
}

public function testRegenerate()
{
$storage = $this->getStorage();
Expand Down