fix(session): age flash values at the start of the request (#2198)

Author: osbre

packages/http/src/Session/ManageSessionMiddleware.php

@@ -24,10 +24,14 @@ public function __construct(
 
     public function __invoke(Request $request, HttpMiddlewareCallable $next): Response
     {
+        // Flash values are aged at the start of the request, before the response
+        // body is rendered, so a value flashed on the previous request stays
+        // readable this request and is expired on the next one.
+        $this->session->cleanup();
+
         try {
             return $next($request);
         } finally {
-            $this->session->cleanup();
             $this->sessionManager->save($this->session);
 
             match ($this->config->cleanupStrategy) {

packages/http/src/Session/Session.php

@@ -65,11 +65,9 @@ public function reflash(): void
      */
     public function get(string|UnitEnum $key, mixed $default = null): mixed
     {
-        $key = Str\parse($key);
-        $value = $this->data[$key] ?? $default;
+        $value = $this->data[Str\parse($key)] ?? $default;
 
         if ($value instanceof FlashValue) {
-            $this->expiredKeys[$key] = $key;
             $value = $value->value;
         }
 
@@ -110,13 +108,28 @@ public function remove(string|UnitEnum $key): void
     }
 
     /**
-     * Cleans up expired session values.
+     * Expires flash values by one request. Values flashed on the previous request
+     * are removed; values flashed since are marked to expire on the next request.
+     *
+     * This runs once, at the start of the request, so that flash values remain
+     * readable while the request is handled - including while the response body
+     * (e.g. a view) is being rendered - regardless of whether they are read.
      */
     public function cleanup(): void
     {
         foreach ($this->expiredKeys as $key) {
             $this->remove($key);
         }
+
+        $this->expiredKeys = [];
+
+        foreach ($this->data as $key => $value) {
+            if (! $value instanceof FlashValue) {
+                continue;
+            }
+
+            $this->expiredKeys[$key] = $key;
+        }
     }
 
     /**

tests/Fixtures/Controllers/FlashViewController.php

@@ -0,0 +1,34 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tests\Tempest\Fixtures\Controllers;
+
+use Tempest\Http\Responses\Redirect;
+use Tempest\Http\Session\Session;
+use Tempest\Router\Get;
+use Tempest\Router\Post;
+use Tempest\View\View;
+
+use function Tempest\View\view;
+
+final readonly class FlashViewController
+{
+    public function __construct(
+        private Session $session,
+    ) {}
+
+    #[Post('/flash-view')]
+    public function flash(): Redirect
+    {
+        $this->session->flash('message', 'flashed-message');
+
+        return new Redirect('/flash-view');
+    }
+
+    #[Get('/flash-view')]
+    public function show(): View
+    {
+        return view('./flash-view.view.php');
+    }
+}

tests/Fixtures/Controllers/flash-view.view.php

@@ -0,0 +1,12 @@
+<?php
+
+use Tempest\Http\Session\Session;
+
+use function Tempest\Container\get;
+
+// The flash value is consumed while the view renders, i.e. after the session
+// middleware has run. It must still expire for the following request.
+$message = get(Session::class)->get('message');
+?>
+
+<div id="flash">{{ $message }}</div>

tests/Integration/Http/FlashInViewTest.php

@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tests\Tempest\Integration\Http;
+
+use PHPUnit\Framework\Attributes\Test;
+use Tests\Tempest\Integration\FrameworkIntegrationTestCase;
+
+/**
+ * @internal
+ */
+final class FlashInViewTest extends FrameworkIntegrationTestCase
+{
+    #[Test]
+    public function flash_value_consumed_while_rendering_a_view_is_shown_once(): void
+    {
+        // A request flashes a value and redirects.
+        $this->http
+            ->post('/flash-view')
+            ->assertRedirect('/flash-view');
+
+        // The redirected request reads the flash value while rendering the view.
+        $this->http
+            ->get('/flash-view')
+            ->assertOk()
+            ->assertSee('flashed-message');
+
+        // The following request no longer sees it: it was aged out at the start of
+        // this request, even though it was only ever read while rendering the view.
+        $this->http
+            ->get('/flash-view')
+            ->assertOk()
+            ->assertNotSee('flashed-message');
+    }
+}

tests/Integration/Http/FormSessionTest.php

@@ -166,10 +166,12 @@ public function errors_are_flashed_and_cleared_after_next_request(): void
         // First access - errors exist
         $this->assertTrue($this->formSession->hasErrors());
 
-        // Simulate cleanup after request
+        // Start of the next request: still available to display.
         $this->session->cleanup();
+        $this->assertTrue($this->formSession->hasErrors());
 
-        // Second access - errors cleared
+        // Start of the request after that: cleared.
+        $this->session->cleanup();
         $this->assertFalse($this->formSession->hasErrors());
     }
 
@@ -183,10 +185,12 @@ public function values_are_flashed_and_cleared_after_next_request(): void
         // First access - values exist
         $this->assertEquals('John Doe', $this->formSession->getOriginalValueFor('name'));
 
-        // Simulate cleanup after request
+        // Start of the next request: still available to display.
         $this->session->cleanup();
+        $this->assertEquals('John Doe', $this->formSession->getOriginalValueFor('name'));
 
-        // Second access - values cleared
+        // Start of the request after that: cleared.
+        $this->session->cleanup();
         $this->assertEquals('', $this->formSession->getOriginalValueFor('name'));
     }
 }

tests/Integration/Http/SessionTest.php

@@ -74,7 +74,25 @@ public function flash(): void
         $this->session->flash('message', 'success');
         $this->assertEquals('success', $this->session->get('message'));
 
+        // Start of the next request: the value is still available (e.g. to render).
         $this->session->cleanup();
+        $this->assertEquals('success', $this->session->get('message'));
+
+        // Start of the request after that: it has expired.
+        $this->session->cleanup();
+        $this->assertNull($this->session->get('message'));
+    }
+
+    #[Test]
+    public function flash_value_expires_even_when_never_read(): void
+    {
+        $this->session->flash('message', 'success');
+
+        // Aging is independent of reads: two request boundaries expire the value
+        // without it ever being retrieved.
+        $this->session->cleanup();
+        $this->session->cleanup();
+
         $this->assertNull($this->session->get('message'));
     }