From dc87e652f59c489f87cbd25771705ac20e9eb2ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baptiste=20=22Talus=22=20Clavi=C3=A9?= Date: Tue, 19 Nov 2013 11:25:30 +0100 Subject: [PATCH 1/2] Fixes regression in SessionStorage top classes This regression was introduced by symfony/symfony#9246 ; if this PR was to set the session to "not started" was it was saved (and thus closed), for an unknown reason, the PHPSESSID cookie was never created for some apps. But if we modify the isStarted method, and take into account the $this->closed state, then it should enact the sought behaviour without introducing this (possible) regression --- .../HttpFoundation/Session/Storage/MockArraySessionStorage.php | 3 +-- .../HttpFoundation/Session/Storage/NativeSessionStorage.php | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php index 6f643cc0160bf..1779c1733dc65 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php @@ -164,7 +164,6 @@ public function save() } // nothing to do since we don't persist the session data $this->closed = false; - $this->started = false; } /** @@ -213,7 +212,7 @@ public function getBag($name) */ public function isStarted() { - return $this->started; + return $this->started && !$this->closed; } /** diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php index e9d2e176dd47d..3616a33853e76 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php @@ -239,7 +239,6 @@ public function save() } $this->closed = true; - $this->started = false; } /** @@ -314,7 +313,7 @@ public function getMetadataBag() */ public function isStarted() { - return $this->started; + return $this->started && !$this->closed; } /** From fa08c13f6fb82be6066d50a1fdf7a6188b530b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baptiste=20=22Talus=22=20Clavi=C3=A9?= Date: Tue, 19 Nov 2013 15:09:06 +0100 Subject: [PATCH 2/2] Add tests for closing sessions And also revert a part of the first commit on the MockArray storage, due to a comment that could make more sense (never actually "closing" the session). As this is used in unit tests (and functional with its extension) should this point be discussed ? --- .../Storage/MockArraySessionStorage.php | 4 +++- .../Storage/MockArraySessionStorageTest.php | 17 +++++++++++++++++ .../Storage/NativeSessionStorageTest.php | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php b/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php index 1779c1733dc65..e540d229e0228 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/MockArraySessionStorage.php @@ -162,8 +162,10 @@ 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; } /** @@ -212,7 +214,7 @@ public function getBag($name) */ public function isStarted() { - return $this->started && !$this->closed; + return $this->started; } /** diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/MockArraySessionStorageTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/MockArraySessionStorageTest.php index 2a6f6bb7e8fe5..87986a3e60140 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/MockArraySessionStorageTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/MockArraySessionStorageTest.php @@ -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)); + } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php index 31dd41ab00f27..898cc41035c0d 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php @@ -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