[2.x] Fix notifications endpoint 400 on invalid subject.discussion default include#4816
Open
karl-bullock wants to merge 1 commit into
Open
Conversation
NotificationResource added `subject.discussion` to the default include whenever more than one notification subject type was registered. That include is only valid when a subject type actually has a `discussion` relationship; with, for example, a User-subject notification type and no Post-subject type, the include validator rejects the endpoint's own default include with `400 Invalid include [subject.discussion]`, breaking the notifications endpoint for every user on the forum. Gate the include on whether any subject type genuinely exposes an includable `discussion` relationship instead of on the subject-type count. Adds regression tests covering both the no-discussion-subject case (no longer 400s) and that the discussion is still eager-loaded when a discussion-bearing subject type is present.
This was referenced Jul 14, 2026
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.
Fixes #4815
Changes proposed in this pull request
NotificationResourceaddedsubject.discussionto the notifications endpoints' default include whenever more than one notification subject type was registered:That nested include is only valid when at least one subject type actually exposes an includable
discussionrelationship. The include validator validates default includes too, so when the subject collection is, for example,{discussions, users}(neither of which has adiscussionrelationship), it rejects the endpoint's own default include:{"errors":[{"status":"400","code":"validation_error","title":"Bad Request","detail":"Invalid include [subject.discussion]","source":{"parameter":"include"}}]}Because
NotificationListStatesends no explicitincludeand relies on the server default, this returns400for every user and breaks the notifications dropdown and page.Usersubjects are not exotic:flarum/mentionsshipsUserMentionedBlueprint; it only stays safe because it also shipsPostMentionedBlueprint, andPosthas adiscussionrelationship. Any forum with aUser-subject (orGroup-subject) notification type and noPost-subject type present hits this.This PR gates the include on whether a subject type genuinely exposes an includable
discussionrelationship, instead of on the subject-type count. The include is preserved exactly where it matters (Post subjects) and dropped only when it would be invalid.Reviewers should focus on
subjectsHaveDiscussionRelationship()resolves each subject type's resource and checks its fields for an includablediscussionrelationship. This usesresolveFields()(which applies field extenders), guarded by aninstanceof AbstractResourcecheck.Verified
Reproduced and fixed on a fresh
2.0.0-rc.5install (MariaDB):GET /api/notificationsbefore{discussions}(core only){discussions, users}{discussions, users, posts}Two regression tests are added to
notifications/ListTest: one asserting the endpoint no longer 400s when a subject type has no discussion relationship, and one asserting the discussion is still eager-loaded when a discussion-bearing subject type is present. The existing notification tests still pass.