Skip to content

[2.x] fix: distinguish a missing announcement excerpt from an empty one - #4902

Merged
imorland merged 1 commit into
2.xfrom
im/announcements-missing-include
Aug 5, 2026
Merged

[2.x] fix: distinguish a missing announcement excerpt from an empty one#4902
imorland merged 1 commit into
2.xfrom
im/announcements-missing-include

Conversation

@imorland

@imorland imorland commented Aug 5, 2026

Copy link
Copy Markdown
Member

The announcements fetcher read each excerpt from the firstPost include and ran it through makeExcerpt() whether or not that post arrived:

'excerpt' => $this->makeExcerpt(Arr::get($firstPost, 'attributes.contentHtml', '')),

With no post, Arr::get(null, …) yields '', so the excerpt became an empty string — indistinguishable from a post that genuinely has no text.

That is how the admin announcements widget came to render every card blank without anyone noticing. The response looked well-formed and said, in effect, "these announcements have no content". The real fault was upstream: fof/gamification had narrowed the firstPost eager load on the discussion index, so discuss.flarum.org stopped serializing the include at all (fixed in FriendsOfFlarum/gamification#169).

Since every Flarum forum pulls this endpoint, that single upstream change blanked the widget everywhere.

The change

excerpt is now null where the post never arrived, and '' only where the post is genuinely empty.

Nothing renders differently — AnnouncementItem already guards with {a.excerpt && …} — but a recurrence becomes legible rather than silent. The AnnouncementData type is updated to match, since it can now be null.

Tests

Three added. The existing ten all hand-build their own included array, so none of them could notice the request losing its include, nor the include coming back empty — which is exactly the gap this fell through.

  • the request asks for the relationships the excerpt and author need — asserts include contains firstPost and user, via a Guzzle history middleware. Passes today; would have caught a lost include.
  • a missing include yields null — fails without this change (Failed asserting that '' is null). I checked by reverting it.
  • an empty post still yields an empty string — so the distinction holds in both directions and this isn't just "null everywhere".

Core unit suite green: 405 tests. Also drops a deprecated ReflectionProperty::setAccessible() call flagged on PHP 8.5, taking the suite's deprecation count from 2 to 1.

The announcements fetcher read each excerpt from the `firstPost` include and
ran it through `makeExcerpt()` regardless of whether that post arrived. With no
post, `Arr::get(null, ...)` yields `''` and the excerpt became an empty string —
the same value a post with no text would produce.

That is how the admin announcements widget came to render every card blank
without anyone noticing: the response looked well-formed and said, in effect,
"these announcements have no content". The actual fault was upstream, where
fof/gamification had narrowed the `firstPost` eager load and stopped the include
being serialized at all.

The excerpt is now null where the post never arrived, and an empty string only
where the post is genuinely empty. The widget already guards on a falsy excerpt,
so nothing renders differently — but a future recurrence is legible instead of
silent.

Tests: that the request asks for the relationships the excerpt and author are
read from — nothing asserted that before, so a lost include would not have been
noticed; that a missing include yields null; and that an empty post still yields
an empty string, so the distinction holds in both directions.
@imorland
imorland requested a review from a team as a code owner August 5, 2026 17:55
@imorland imorland added this to the 2.0.0-rc.6 milestone Aug 5, 2026
@imorland
imorland merged commit dfba8ad into 2.x Aug 5, 2026
25 checks passed
@imorland
imorland deleted the im/announcements-missing-include branch August 5, 2026 18:10
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.

1 participant