From aa667ce91d5c4d16805391ddf35030b3a6993f41 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Tue, 23 Apr 2019 17:44:51 +0100 Subject: [PATCH 1/3] Fix regression of delegation of Session::save() to Storage::Save() --- src/Symfony/Component/HttpFoundation/Session/Session.php | 4 +--- .../Session/Storage/NativeSessionStorage.php | 6 ++++++ .../HttpFoundation/Tests/Session/SessionTest.php | 9 --------- .../Tests/Session/Storage/NativeSessionStorageTest.php | 7 +++++++ 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Session.php b/src/Symfony/Component/HttpFoundation/Session/Session.php index 62ce948b6828..867ceba97f8d 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Session.php +++ b/src/Symfony/Component/HttpFoundation/Session/Session.php @@ -193,9 +193,7 @@ public function migrate($destroy = false, $lifetime = null) */ public function save() { - if ($this->isStarted()) { - $this->storage->save(); - } + $this->storage->save(); } /** diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php index c4dbe7586822..8d22267669c1 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php @@ -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 + // not started. + if (!$this->started) { + return; + } + $session = $_SESSION; foreach ($this->bags as $bag) { diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php index e75b3321b035..da21213bb24c 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php @@ -261,13 +261,4 @@ public function testIsEmpty() $this->assertTrue($this->session->isEmpty()); } - 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(); - } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php index 7cc2eb79c8db..25a2d8c9c17a 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php @@ -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(); From ab7868547a4596d8301c2731f74a3cce377a377e Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Tue, 23 Apr 2019 19:16:33 +0100 Subject: [PATCH 2/3] Update for @nicolas-grekas's code review --- .../HttpFoundation/Session/Storage/NativeSessionStorage.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php index 8d22267669c1..13d0a2034d5f 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php @@ -219,12 +219,11 @@ public function regenerate($destroy = false, $lifetime = null) */ public function save() { - // In PHP <7.2 session_write_close() does not error if the session is - // not started. + // 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) { return; } - $session = $_SESSION; foreach ($this->bags as $bag) { From 27f26159b973a4487e02f4dcdeae11feb97b4faf Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Wed, 24 Apr 2019 12:55:36 +0100 Subject: [PATCH 3/3] Update coding standards --- .../Component/HttpFoundation/Tests/Session/SessionTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php index da21213bb24c..afa00fc7c304 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/SessionTest.php @@ -260,5 +260,4 @@ public function testIsEmpty() $flash->get('hello'); $this->assertTrue($this->session->isEmpty()); } - }