-
Notifications
You must be signed in to change notification settings - Fork 11.3k
[12.x] Support nested relations on relationLoaded
method
#55471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
relationLoaded
methodrelationLoaded
method
@@ -1070,9 +1070,26 @@ public function getRelation($relation) | |||
* @param string $key | |||
* @return bool | |||
*/ | |||
public function relationLoaded($key) | |||
public function relationLoaded(string $key): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding type hinting to a method's argument, and adding a return type to a method, are breaking changes to anyone overriding this method in a subclass.
As all models extend the Model
class, there is a non-zero chance some project might be overriding this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigopedra Yes having return type causes issues. Modified the code.
I would keep the current check on top of the method body to avoid any performance impact. Something like this: public function relationLoaded($key)
{
if (array_key_exists($key, $this->relations)) {
return true;
}
// augmented logic
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve it, but looks good to me
Hmm this does seem to have caused a problem for me, here's an open source test failure if you are interested: https://github.com/filamentphp/filament/actions/runs/14589554182/job/40966543141 |
The issue is that the
In this case, the issue is when attempting to access an attribute with a |
@tmsperera you might want to add an framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php Lines 1088 to 1094 in 3faeb07
Something like this: if ($nestedRelation !== null) {
foreach ($this->$relation as $related) {
if (! $related instanceof Model) {
return false;
}
if (! $related->relationLoaded($nestedRelation)) {
return false;
}
}
} |
Issue opened with reproduction repository: #55518 |
@danharrin Great you found the bug. @rodrigopedra great job on the fix. I will add more test cases to avoid such unexpected bugs happens in future as well. |
Reverted in: dc5b445 |
Added support to check nested relations when using
relationLoaded()
method of Eloquent ModelWhy?
Current
relationLoaded()
method only checks single level relation of the Model. Now users can check nested relations.Benefits
End users can check whether the nested relation loaded once and load the desired relation.
It will not break existing features
Technically this new enhancement should not break any existing usage of the method because the code is designed to support single level relation check as well.