[1.x] fix(core): render only the requested posts on the no-JS discussion page - #4852
Open
karl-bullock wants to merge 1 commit into
Open
[1.x] fix(core): render only the requested posts on the no-JS discussion page#4852karl-bullock wants to merge 1 commit into
karl-bullock wants to merge 1 commit into
Conversation
…ion page
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The server-rendered discussion page decides what to print by scanning the whole API response for anything post-shaped:
Extensions register their own post relationships on that endpoint.
flarum/mentionsaddsposts.mentionedBytogether withposts.mentionedBy.discussion, so every post that quoted a post on the page arrives inincludedas a full post, carrying both a discussion relationship and rendered content. That makes it indistinguishable from the page's own posts, so the quoting post is printed on a page it does not belong to, and printed again on the page it does.On a live forum with mentions enabled this 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. Checking page 1 against the API, 10 of the 29 rendered posts were
mentionedBytargets of posts on that page.The page also disagrees with itself about which page it is. A numbered URL computes
$pageas1 + intdiv($near, 20)and uses it for the previous/next links and the canonical URL, while the window it renders is centred on the linked post. So/d/1/25declares page 2 as canonical and then prints posts 15 to 30, putting six of page 1's posts on a page that claims to be page 2.Both are the same class of problem as #3370, where the reporter found Google indexing a discussion page using content from other pages. That issue is discussed in terms of frontend auto-loading, and this is a separate, server-side mechanism producing the same symptom, so I would not call it a fix for #3370, but it removes one concrete way the rendered HTML ends up holding another page's posts. I have added the details there.
What
Ask the posts endpoint for the page 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 always matches the canonical URL.
The discussion document, and therefore the payload the JS app boots from, is deliberately left untouched. It still carries the posts surrounding the one being linked to, which is what lets the app scroll to it. This is narrower than the equivalent code on
2.x, which also rewritesincluded.The one behaviour change is that a numbered URL now renders the page it declares as canonical, rather than a window centred on the post. For the no-JS render I think that is the point: it is what the pagination links and the canonical URL already claim.
Tests
framework/core/tests/integration/frontend/DiscussionPageContentTest.php, 3 tests, covering the first page, the second page, and a numbered URL rendering the page it declares as canonical. The last one fails on current1.x.extensions/mentions/tests/integration/frontend/DiscussionPageMentionsTest.php, 1 test, pinning the reported case: a post that merely quotes a post on page 1 must not be rendered on page 1. This fails on current1.x.Verification