Skip to content

[1.x] fix(mentions): prevent deleted fallback for unsynced posts during formatting#4826

Merged
imorland merged 5 commits into
flarum:1.xfrom
huoxin233:huoxin/1.x-fix-mentions-unparse-saving
Jul 23, 2026
Merged

[1.x] fix(mentions): prevent deleted fallback for unsynced posts during formatting#4826
imorland merged 5 commits into
flarum:1.xfrom
huoxin233:huoxin/1.x-fix-mentions-unparse-saving

Conversation

@huoxin233

@huoxin233 huoxin233 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #4823 & #3454
Backport fix from #4824 to 1.x

Changes proposed in this pull request:

This PR modifies all Format*Mentions and Unparse*Mentions classes to:

  • Use the relation's loaded collection if it has been eager loaded (via relationLoaded), executing 0 queries.
  • Fallback directly to the base model query (e.g., Post::query()->find()) rather than querying the pivot table if the relation isn't loaded.

This completely bypasses the pivot table requirement during text evaluation, allowing the formatters to accurately retrieve display names for edited posts.

This bug also caused false positives in the Flarum test suite:

  • editing_a_post_with_deleted_author_that_has_a_mention_works: The test edited a post by sending the exact same text that the post already had. Because the broken unparse() method previously evaluated the old post content as [deleted], Flarum thought the content had changed and dispatched the Revised event. With unparse() fixed, Flarum correctly recognizes the text did not change and skips the Revised event, causing the test's pivot table assertion to fail. This PR updates the test payloads to actually mutate the content so that the Revised event fires properly.
  • deleted_tag_mentions_relation_unparse_and_render_as_expected: The test verified that a post mentioning a Tag would render as [deleted] if the pivot row was missing, despite the Tag actually existing in the database fixtures. Before our format fixes, Flarum wrongly rendered this as deleted because it strictly trusted the empty pivot table. With our format logic fixed, Flarum rightfully discovers the Tag still exists and refuses to render it as deleted, causing the test to fail. This PR updates the test to target a genuinely non-existent Tag ID (999) to accurately test the deleted fallback.

Output of $post->content

Content use to test:

@"huoxin"#p34 abc
@"huoxin"#1 abc

Output:

Before:
[2026-07-22 19:05:01] flarum.INFO: [Test] Content observed during Saving event: {"content":"@\"[unknown]\"#p34 abc
@\"[deleted]\"#1 abc"} 

After:
[2026-07-22 19:07:39] flarum.INFO: [Test] Content observed during Saving event: {"content":"@\"huoxin\"#p34 abc
@\"huoxin\"#1 abc"} 

This seems to fix issue related to email notification #3454 also:

Before:

[2026-07-22 19:05:01] flarum.INFO: Message-ID: <95f9f5663f4c9b91ec6f56f6588ee9c4@0.0.0.0>
Date: Wed, 22 Jul 2026 19:05:01 +0000
Subject: [Follow User] New post in asdsa
From: test <admin@xxxxxx>
To: huoxin <a@a.com>
MIME-Version: 1.0
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable
.....
    <div class="content">
        <p>Hey huoxin,</p>

<p>huoxin233 posted in a discussion: asdsa</p>

<p>To view the new activity, check out the following link:</p>

<p><a href="https://xxxxxxxx/d/9/9">https://xxxxxxxx/d/9/9</a></p>

<p>—</p>

<p>@“[unknown]”#p34 abc
@“[deleted]”#1 abc</p>

<p>—</p>

<p>You won’t receive any more notifications about this discussion until you’re up-to-date.</p>


    </div>
    <div class="footer">
        <div class="content">
            <p>Sent from test using FoF Pretty Mail</p>
        </div>
    </div>
    </body>
</html>

After:

[2026-07-22 19:07:39] flarum.INFO: Message-ID: <c976d05041b1ec05578ddf134a5da263@0.0.0.0>
Date: Wed, 22 Jul 2026 19:07:39 +0000
Subject: [Follow User] New post in asdsa
From: test <admin@xxxx>
To: huoxin <a@a.com>
MIME-Version: 1.0
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

....
    <div class="content">
        <p>Hey huoxin,</p>

<p>huoxin233 posted in a discussion: asdsa</p>

<p>To view the new activity, check out the following link:</p>

<p><a href="https://xxxxxxxx/d/9/10">https://xxxxxxxx/d/9/10</a></p>

<p>—</p>

<p>@“huoxin”#p34 abc
@“huoxin”#1 abc</p>

<p>—</p>

<p>You won’t receive any more notifications about this discussion until you’re up-to-date.</p>


    </div>
    <div class="footer">
        <div class="content">
            <p>Sent from test using FoF Pretty Mail</p>
        </div>
    </div>
    </body>
</html>

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)

huoxin233 and others added 5 commits July 23, 2026 05:03
During the Saving event, the mentions pivot tables are not yet populated.
When unparse() is called (e.g. by an extension reading $post->content),
the fallback logic in updatePostMentionTags (and User/Tag variants) was
attempting to execute a relationship query ($context->mentionsPosts()->find()).
Because the pivot table is empty for a new post, this query fails, causing
the unparser to incorrectly resolve the mention to the deleted_text fallback.

This fix removes the pivot table constraint when the relationship is not
eagerly loaded, directly querying the underlying model instead. This also
resolves an N+1 query bottleneck when unparsing posts without eager loaded
mentions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…matting

When a post containing mentions is edited, Flarum generates a new `contentHtml`
payload to send back in the immediate API response. During this save lifecycle,
the mentions pivot tables (`post_mentions_users`, `post_mentions_posts`, etc.)
are not fully synced for the current transaction.

Previously, the formatters (`FormatPostMentions`, `FormatUserMentions`, etc.)
contained a fallback check (`|| $context instanceof Post`) that forced a
query against the pivot table if the relationship wasn't eager-loaded in memory.
Because the pivot table is unsynced for edited posts, this query returned null,
causing the formatter to mistakenly assume the mentioned entity was deleted.
This resulted in the frontend UI temporarily displaying `[deleted]` mentions
until the user manually refreshed the page.

This fix removes the forced pivot table query. If the relation is not
already loaded via `$context->getRelations()`, it falls back directly
to the base model query (e.g., `Post::find()`), which accurately retrieves
the display names for unsaved edits without hitting the empty pivot tables
or triggering an N+1 lazy load.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The `editing_a_post_with_deleted_author_that_has_a_mention_works` tests
were incorrectly submitting a `PATCH` payload with the exact same content
as the unparsed original post text. Because the text was identical, Flarum
correctly optimized away the `Revised` event, which meant the database
pivot tables were never synced and the test failed on the database assertion.

This fixes the test by actually mutating the content string during the edit,
ensuring the `Revised` event is fired and the pivot tables are populated
so they can be properly asserted.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The `deleted_tag_mentions_relation_unparse_and_render_as_expected` test verified that a post mentioning a Tag (ID 3) would render as `[deleted]`
if the relationship pivot row was missing, despite Tag 3 actually existing in the
database fixtures. Before our format fixes, Flarum wrongly rendered this as deleted
because it strictly trusted the empty pivot table.

With our format logic fixed to correctly query the base table when the pivot
is empty, Flarum rightfully discovered that Tag 3 still exists and refused to
render it as deleted, causing the test to fail.

This commit fix the test by changing the mention to target Tag ID 999 (which truly
does not exist in the database fixtures), ensuring the deleted rendering logic is
accurately tested against an actually missing tag.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: StyleCI Bot <bot@styleci.io>
@huoxin233
huoxin233 requested a review from a team as a code owner July 23, 2026 10:33
@imorland imorland added this to the 1.8.18 milestone Jul 23, 2026
@imorland
imorland merged commit 03d594b into flarum:1.x Jul 23, 2026
398 checks passed
@huoxin233
huoxin233 deleted the huoxin/1.x-fix-mentions-unparse-saving branch July 24, 2026 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants