Fix/eager load one of many subquery constraints#59337
Draft
sadique-cws wants to merge 4 commits intolaravel:13.xfrom
Draft
Fix/eager load one of many subquery constraints#59337sadique-cws wants to merge 4 commits intolaravel:13.xfrom
sadique-cws wants to merge 4 commits intolaravel:13.xfrom
Conversation
When eager loading a latestOfMany/ofMany relationship with constraints
(e.g. with(['lastPayment' => fn($q) => $q->where('store_id', 1)])),
the constraints were only applied to the outer query, not the inner
subquery that determines which record is the 'latest'.
This caused the inner subquery to pick the wrong aggregate row (e.g.
the latest payment across ALL stores), and then the outer filter would
discard it — silently returning null.
The fix snapshots the outer query's wheres before and after the user's
constraint closure runs, then copies only the new wheres into the
oneOfMany subquery. This ensures the inner subquery determines 'latest'
within the user's filtered dataset.
Fixes laravel#59318
Member
|
Don't change any other tests or give explanation why they are changed. |
37577bc to
a3d63c8
Compare
Contributor
Author
|
Apologies for that, Taylor! My editor accidentally wiped out two assertion lines from the neighboring I was not intending to modify any existing tests! I have completely restored them and forcefully updated the branch so the test suite history remains clean without any unrelated changes. It should be good to merge now! |
db57637 to
68f4676
Compare
This PR addresses the issue where constraints applied to eager-loaded or lazy-loaded oneOfMany relationships were erroneously applied as post-filters. By implementing __call in the CanBeOneOfMany trait, we now route where* and orWhere* constraints directly to the aggregation subquery, ensuring they correctly influence record selection.
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.
Issue #59318 — PR Resubmission Description
Description
This PR fixes a silent data loss bug when developers attempt to eager load latestOfMany, oldestOfMany, or ofMany relationships with dynamic closure constraints (Issue #59318).
🚨 The Bug
Currently, if a developer applies a closure constraint to a one-of-many eager load—for example:
...the
'status' = 'successful'constraint is only applied to the outer query. It completely misses the inner subquery that determines which record is actually the "latest".As a result, the inner subquery fetches the global latest login for that user (which might have a
failedstatus). Then, the outer query executes with thesuccessfulfilter and discards that row. The relationship silently returnsnull, even if the user has thousands of prior successful logins.The Solution
Within
Eloquent\Builder::eagerLoadRelation(), we now snapshot the outer query'swheresandbindings['where']before executing the developer's$constraints($relation)closure.After the closure is executed, we diff the query and extract any newly appended where constraints. If the relationship is a one-of-many variant (by checking getOneOfManySubQuery()), we securely forward those specific dynamic differences down into the inner
oneOfManySubQuery.Why is this safe? Does it break anything?
This introduces zero breaking changes.
The outer query diffing mechanism ensures that we only forward constraints explicitly passed by the developer in that exact with() closure. It does not tamper with default Eloquent scopes, relation mappings, or touch base constraints. It behaves completely identically to how constraints naturally append to
HasManyqueries. All pre-existingoneOfManyintegration tests continue to pass without modifications.How does it make building web applications easier?
This eliminates a highly unintuitive "gotcha" for end users. Developers naturally expect that attaching a closure to standard Eloquent relations limits the scope of the entities returned. When users use eloquent eager loading constraints inside a latestOfMany(), they naturally assume the application will intelligently fetch the "latest record that matches my sub-filter." By ensuring the aggregate queries obey developer constraints properly, it removes a silent data failure and restores expected framework behavior.
Tests Added
I've drafted and included an explicit integration test within DatabaseEloquentHasOneOfManyTest named testEagerLoadingWithConstraintsAppliesToSubQuery that builds a small relationship simulation, intercepts the eager-loaded queries with closure constraints, and definitively proves the inner-join resolves the valid restricted aggregate row instead of silently passing null.