[1.x] fix(core): render only the requested posts on the no-JS discussion page (#4852)

The server-rendered discussion page chose what to print by scanning the
whole API response for anything post-shaped:

    foreach ($apiDocument->included as $resource) {
        if ($resource->type === 'posts'
            && isset($resource->relationships->discussion)
            && isset($resource->attributes->contentHtml)) {

Extensions register their own post relationships on that endpoint.
flarum/mentions adds posts.mentionedBy together with
posts.mentionedBy.discussion, so every post that quoted a post on the page
arrives in `included` as a full post, carrying a discussion relationship
and rendered content. That makes it indistinguishable from the page's own
posts, and the quoted post gets printed on a page it does not belong to,
then printed again on the page it does.

On a forum with mentions enabled the effect is easy to see. Fetching the
first eight pages of a busy discussion returned 29, 20, 23, 24, 21, 23, 20
and 24 posts rather than 20 each, with 21 posts appearing on two different
pages.

The page also disagreed with itself about which page it was. A numbered URL
computes $page as 1 + intdiv($near, 20) and uses it for the previous and
next links and for the canonical URL, while the window it rendered was
centred on the linked post. So /d/1/25 declared page 2 as canonical and
then printed posts 15 to 30, putting six of page 1's posts on a page that
claims to be page 2.

Ask the posts endpoint for the page instead, and render its primary data.
Extensions cannot add posts to another endpoint's primary data, so the
render is correct by construction rather than by filtering more cleverly,
and the rendered window now always matches the canonical URL.

The discussion document, and therefore the payload the JS app boots from,
is deliberately left alone. It still carries the posts surrounding the one
being linked to, which is what lets the app scroll to it.

Verified in a dev forum (1.8, headless): with the fix, pages of a 30 post
discussion render 20 and 10 with no post on two pages, against 23 and 10
before. Reverting the change reproduces the old counts.

Author: karl-bullock

extensions/mentions/tests/integration/frontend/DiscussionPageMentionsTest.php

@@ -0,0 +1,88 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Mentions\Tests\integration\frontend;
+
+use Carbon\Carbon;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Illuminate\Support\Str;
+
+class DiscussionPageMentionsTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('flarum-mentions');
+
+        $posts = [];
+
+        for ($i = 1; $i <= 25; $i++) {
+            $posts[] = [
+                'id' => $i,
+                'number' => $i,
+                'discussion_id' => 1,
+                'created_at' => Carbon::parse('2024-01-01 00:00:00')->addMinutes($i),
+                'user_id' => 2,
+                'type' => 'comment',
+                'content' => '<t><p>POST-MARKER-'.str_pad((string) $i, 2, '0', STR_PAD_LEFT).'</p></t>',
+            ];
+        }
+
+        $this->prepareDatabase([
+            'discussions' => [
+                ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'user_id' => 2, 'first_post_id' => 1, 'last_post_id' => 25, 'comment_count' => 25, 'is_private' => 0],
+            ],
+            'posts' => $posts,
+            // Post 21 sits on page 2 and quotes post 2, which sits on page 1.
+            'post_mentions_post' => [
+                ['post_id' => 21, 'mentions_post_id' => 2],
+            ],
+            'users' => [
+                $this->normalUser(),
+            ],
+        ]);
+    }
+
+    private function html(string $path): string
+    {
+        $body = $this->send($this->request('GET', $path))->getBody()->getContents();
+
+        return Str::before($body, '<script id="flarum-json-payload"');
+    }
+
+    private function marker(int $number): string
+    {
+        return 'POST-MARKER-'.str_pad((string) $number, 2, '0', STR_PAD_LEFT);
+    }
+
+    /**
+     * This extension asks the discussion endpoint to include, for every post on
+     * the page, the posts that quoted it. Those arrive as full posts, with a
+     * discussion relationship and rendered content, so a page must not decide
+     * what to render by looking for post-shaped resources in the response.
+     *
+     * @test
+     */
+    public function page_does_not_render_a_later_post_that_quotes_one_of_its_posts()
+    {
+        $body = $this->html('/d/1');
+
+        // Page 1 is posts 1 to 20.
+        $this->assertStringContainsString($this->marker(2), $body);
+        $this->assertStringContainsString($this->marker(20), $body);
+
+        // Post 21 belongs to page 2. It quotes post 2, and that must not drag
+        // it onto page 1.
+        $this->assertStringNotContainsString($this->marker(21), $body);
+    }
+}

framework/core/src/Forum/Content/Discussion.php

@@ -85,9 +85,27 @@ public function __invoke(Document $document, Request $request)
                 ($queryString ? '?'.$queryString : '');
         };
 
+        // Ask for the posts this page is supposed to show, instead of sifting the
+        // included resources for anything post-shaped. Extensions register their
+        // own post relationships on this endpoint: flarum/mentions includes
+        // posts.mentionedBy along with posts.mentionedBy.discussion, so every post
+        // that quoted a post on this page arrives as a full post, with a discussion
+        // relationship and rendered content, and is indistinguishable from the
+        // page's own posts once it is in `included`. That is how quoted posts ended
+        // up printed on pages they do not belong to, and printed again on the page
+        // they do.
+        $postsApiDocument = $this->getPostsApiDocument($request, [
+            'filter' => ['discussion' => $apiDocument->data->id],
+            'sort' => 'number',
+            'page' => [
+                'offset' => ($page - 1) * 20,
+                'limit' => 20,
+            ],
+        ]);
+
         $posts = [];
 
-        foreach ($apiDocument->included as $resource) {
+        foreach ($postsApiDocument->data as $resource) {
             if ($resource->type === 'posts' && isset($resource->relationships->discussion) && isset($resource->attributes->contentHtml)) {
                 $posts[] = $resource;
             }
@@ -132,4 +150,21 @@ protected function getApiDocument(Request $request, string $id, array $params)
 
         return json_decode($response->getBody());
     }
+
+    /**
+     * Get the result of an API request to list one page of a discussion's posts.
+     *
+     * The primary data of this response is exactly the posts being asked for, so
+     * unlike the discussion endpoint's `included`, extensions cannot add posts of
+     * their own to it.
+     */
+    protected function getPostsApiDocument(Request $request, array $params)
+    {
+        $response = $this->api
+            ->withParentRequest($request)
+            ->withQueryParams($params)
+            ->get('/posts');
+
+        return json_decode($response->getBody());
+    }
 }

framework/core/tests/integration/frontend/DiscussionPageContentTest.php

@@ -0,0 +1,127 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tests\integration\frontend;
+
+use Carbon\Carbon;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Illuminate\Support\Str;
+
+class DiscussionPageContentTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $posts = [];
+
+        // Markers are zero padded so that no marker is a prefix of another one,
+        // which keeps assertStringNotContainsString honest.
+        for ($i = 1; $i <= 30; $i++) {
+            $posts[] = [
+                'id' => $i,
+                'number' => $i,
+                'discussion_id' => 1,
+                'created_at' => Carbon::parse('2024-01-01 00:00:00')->addMinutes($i)->toDateTimeString(),
+                'user_id' => 2,
+                'type' => 'comment',
+                'content' => '<t><p>POST-MARKER-'.str_pad((string) $i, 2, '0', STR_PAD_LEFT).'</p></t>',
+            ];
+        }
+
+        $this->prepareDatabase([
+            'discussions' => [
+                ['id' => 1, 'title' => 'Paginated discussion', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 1, 'last_post_id' => 30, 'comment_count' => 30, 'is_private' => 0],
+            ],
+            'posts' => $posts,
+            'users' => [
+                $this->normalUser(),
+            ],
+        ]);
+    }
+
+    /**
+     * The rendered markup, without the JSON payload that follows it.
+     *
+     * The markup is what a client with no JavaScript, and a crawler, reads as
+     * the page. The payload after it is the data the JS app boots from, which
+     * still carries the posts surrounding the one being linked to so that the
+     * app can scroll to it, and is not page text.
+     */
+    private function html(string $path, array $query = []): string
+    {
+        $request = $this->request('GET', $path);
+
+        if ($query) {
+            $request = $request->withQueryParams($query);
+        }
+
+        $body = $this->send($request)->getBody()->getContents();
+
+        return Str::before($body, '<script id="flarum-json-payload"');
+    }
+
+    private function marker(int $number): string
+    {
+        return 'POST-MARKER-'.str_pad((string) $number, 2, '0', STR_PAD_LEFT);
+    }
+
+    /**
+     * @test
+     */
+    public function first_page_renders_only_the_first_twenty_posts()
+    {
+        $body = $this->html('/d/1');
+
+        $this->assertStringContainsString($this->marker(1), $body);
+        $this->assertStringContainsString($this->marker(20), $body);
+        $this->assertStringNotContainsString($this->marker(21), $body);
+        $this->assertStringNotContainsString($this->marker(30), $body);
+    }
+
+    /**
+     * @test
+     */
+    public function second_page_renders_only_the_second_page_of_posts()
+    {
+        $body = $this->html('/d/1', ['page' => 2]);
+
+        $this->assertStringNotContainsString($this->marker(1), $body);
+        $this->assertStringNotContainsString($this->marker(20), $body);
+        $this->assertStringContainsString($this->marker(21), $body);
+        $this->assertStringContainsString($this->marker(30), $body);
+    }
+
+    /**
+     * A discussion page must not serve content that belongs to another page.
+     * A numbered URL declares a canonical page, so the page it renders has to
+     * be that one, otherwise a crawler indexes one page's posts under another
+     * page's URL.
+     *
+     * @test
+     */
+    public function numbered_url_renders_the_page_it_declares_as_canonical()
+    {
+        $body = $this->html('/d/1/25');
+
+        $this->assertStringContainsString('?page=2', $body, 'Expected the numbered URL to declare page 2 as its canonical URL.');
+
+        // Post 25 sits on page 2, so page 2 is what should be rendered.
+        $this->assertStringContainsString($this->marker(21), $body);
+        $this->assertStringContainsString($this->marker(30), $body);
+
+        // Posts 15 to 20 belong to page 1, and must not appear on a page whose
+        // canonical URL says it is page 2.
+        $this->assertStringNotContainsString($this->marker(15), $body);
+        $this->assertStringNotContainsString($this->marker(20), $body);
+    }
+}