zendframework / zend-expressive-session-ext

PHP ext-session persistence adapter for zend-expressive-session.
BSD 3-Clause "New" or "Revised" License
12 stars 9 forks source link

Regenerate required to initiate a session #19

Closed rtek closed 6 years ago

rtek commented 6 years ago

I am unable to get the session cookie to attach to the response from an initial client request without a regeneration. I would expect the first response to contain a Set-Cookie since there is not yet a session to regenerate.

   public function testCookiesNotSetWithoutRegenerate(): void
   {
        $persistence = new PhpSessionPersistence();
        $request = new ServerRequest();
        $session = $persistence->initializeSessionFromRequest($request);

        //remove to work around
        //$session = $session->regenerate();

        $response = new Response();
        $response = $persistence->persistSession($session, $response);

        $this->assertNotEmpty($response->getHeaderLine('Set-Cookie'));
   }
$ vendor/bin/phpunit test/BugTest.php
PHPUnit 7.0.2 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 93 ms, Memory: 4.00MB

There was 1 failure:

1) ZendTest\Expressive\Session\Ext\BugTest::testCookiesNotSetOnFreshRequest
Failed asserting that a string is not empty.

C:\server\root\oss\zend-expressive-session-ext\test\BugTest.php:33
rtek commented 6 years ago

Proposed fix resulted in lots of failed tests. Reviewing the tests:

These seem like they are reversed. Wouldn't a session cookie imply existing session, so no need to Set-Cookie unless there is a regeneration?

geerteltink commented 6 years ago

If there isn't any data set, it doesn't need to generate a new session id or add the cookie to the response. So your test is wrong. The assert should be like this: $this->assertEmpty($response->getHeaderLine('Set-Cookie'));

However it also fails this test:

    public function testCookiesSetWithoutRegenerate(): void
    {
        $persistence = new PhpSessionPersistence();
        $request = new ServerRequest();
        $session = $persistence->initializeSessionFromRequest($request);

        $session->set('foo', 'bar');

        $response = new Response();
        $response = $persistence->persistSession($session, $response);

        $this->assertNotEmpty($response->getHeaderLine('Set-Cookie'));
    }

In case you have a new session and add data to it, it should be persisted. The problem is that nowhere a check is performed for changed data. This is solved in #21:

    public function persistSession(SessionInterface $session, ResponseInterface $response) : ResponseInterface
    {
        // Regenerate if the session is marked as regenerated
        // Regenerate if there is no cookie id set but the session has changed (new session with data)
        if ($session->isRegenerated()
            || ($session->hasChanged() && ! $this->cookie)) {
            $this->regenerateSession();
        }

        // ...
    }
rtek commented 6 years ago

Yes this works. I misunderstood the underlying issue.