[2.x] fix(sticky): look up the excerpt relationship on the discussions resource - #4883
Merged
Merged
Conversation
…urce The excerpt field resolved the firstPost relationship from $context->collection — the REQUEST's primary resource. On the discussions index that is the discussions resource and everything works, but when a sticky discussion is serialized as an INCLUDED resource, the primary resource is something else entirely. The posts index default-includes each post's discussion, so any posts request for a sticky discussion looked firstPost up on the posts resource, found nothing, and handed the relationship buffer a null relationship — which sends it down its aggregate path and crashes with a TypeError. The lookup now targets the discussions resource explicitly via $context->api->getResource(). Regression test: a posts request for a sticky discussion returns 200 with the excerpt on the included discussion — it fails with a 500 against the previous code.
imorland
changed the title
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
1 participant
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.
Regression fix for #4882, found by clicking around a live install within hours of merge — a posts request for any sticky discussion 500s:
Root cause
The excerpt field resolved the
firstPostrelationship from$context->collection— the request's primary resource. On the discussions index that'sDiscussionResourceand everything works (which is why #4882's tests all passed). But the posts index default-includes each post'sdiscussion, and when a sticky discussion is serialized as an included resource, the primary resource isPostResource: the field lookup misses,EloquentBuffer::load()receives a null relationship, and its!$aggregate && $relationshipbranch condition sends it intoloadAggregate(null)→ TypeError.The testing gap in #4882: every payload test hit
/api/discussions; none exercised a discussion serialized as an included resource of another endpoint.Fix
Look the relationship up on the discussions resource explicitly:
Testing
New regression test:
GET /api/posts?filter[discussion]=<sticky>returns 200 withfirstPostExcerpton the included discussion — verified failing (500) against current 2.x HEAD and green with the fix. Sticky suite 27/27, PHPStan clean, and the reporting URL's document + posts API both return 200 on a live install.Worth a thought for later (not this PR):
EloquentBuffer::load()treating "no aggregate + no relationship" as an aggregate call is a footgun — a loud guard there would have turned this into an obvious error instead of a TypeError two frames deep.