8000 [HttpFoundation] Fix regression of delegation of Session::save() to Storage::Save() by alexpott · Pull Request #31215 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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 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 1 commit
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix regression of delegation of Session::save() to Storage::Save()
  • Loading branch information
@alexpott
alexpott committed Apr 23, 2019
commit aa667ce91d5c4d16805391ddf35030b3a6993f41
4 changes: 1 addition & 3 deletions src/Symfony/Component/HttpFoundation/Session/Session.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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
6 changes: 6 additions & 0 deletions 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 @@ -219,6 +219,12 @@ public function regenerate($destroy = false, $lifetime = null)
*/
public function save()
{
// In PHP <7.2 session_write_close() does not error if the session is
Copy link
Member
@nicolas-grekas nicolas-grekas Apr 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

maybe this should be written as In PHP >=7.2 session_write_close() errors if the session is not started? (single line please)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to say that this is following the logic of session_write_close in PHP <7.2 - in PHP >=7.2 it returns FALSE.

However I've just read the docs on the interface which say

    /**
     * Force the session to be saved and closed.
     *
     * This method must invoke session_write_close() unless this interface is
     * used for a storage object design for unit or functional testing where
     * a real PHP session would interfere with testing, in which case
     * it should actually persist the session data if required.
     *
     * @throws \RuntimeException if the session is saved without being started, or if the session
     *                           is already closed
     */
    public function save();

So now I'm not sure what's the right thing to do. What's clear is that is not what \Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::save() does in 3.4 HEAD.

// not started.
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
Loading

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
Original file line number Diff line number Diff line change
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();
}
}
99EC
7 changes: 7 additions & 0 deletions 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 @@ -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
0