8000 [12.x] Support nested relations on `relationLoaded` method by tmsperera · Pull Request #55595 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

[12.x] Support nested relations on relationLoaded method #55595

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8000 3 commits into from
Closed

[12.x] Support nested relations on relationLoaded method #55595

wants to merge 3 commits into from

Conversation

tmsperera
Copy link
Contributor
@tmsperera tmsperera commented Apr 29, 2025 •
edited
Loading

Added support to check nested relations when using relationLoaded() method of Eloquent Model

This is the 2nd attempt of previously reverted #55471

Why?

Current relationLoaded() method only checks single level relation of the Model. Now users can check nested relations.

$user->load('posts.comments');

// Previously
$user->relationLoaded('posts'); // true
$user->relationLoaded('posts.comments'); // false

// Now
$user->relationLoaded('posts'); // true
$user->relationLoaded('posts.comments'); // true

Benefits

End users can check whether the nested relation loaded once and load the desired relation.

About break existing features...

Checked and updated all usage inside the framework and added regressions tests to ensure previous functionality.

Usage of the method outside framework will not break because the feature preserves single level relation check as previously worked.

This is the 2nd attempt of previously reverted #55471

Has added test cases to verify the issues identified previously by @rodrigopedra @danharrin @tabuna.

Related

#55471
#55518
#55519
#55535
dc5b445

@tmsperera
Copy link
Contributor Author

@rodrigopedra @danharrin @tabuna Glad to hear from you on this.

danharrin
danharrin reviewed Apr 29, 2025
View reviewed changes
Copy link
Contributor
@danharrin danharrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that parts of the codebase that were previously using relationLoaded() now have to use an alternative method indicates that this is a serious enough breaking change that it should not be introduced into the framework, in my opinion.

@tmsperera
Copy link
Contributor Author
tmsperera commented Apr 29, 2025 •
edited
Loading

The fact that parts of the codebase that were previously using relationLoaded() now have to use an alternative method indicates that this is a serious enough breaking change that it should not be introduced into the framework, in my opinion.

@danharrin In that point of view you are correct but previously relationLoaded() just did check if matching key exists on $relation array of a model. I just replaced with the exact logic of previous relationLoaded() method. Technically it will not break anything.

I believe relationLoaded() method should be more convenient than just checking immediate relation on $relations array. It will be convenient to check if any given nested relations are loaded and now I think the method does serves it's name. Let's see what is the point of view of the maintainers.

@danharrin
Copy link
Contributor

I know it doesn't break the places where it is not being called anymore (because you replaced it), but anyone calling this method in a similar way is then going to encounter the new behaviour.

@tmsperera
Copy link
Contributor Author
tmsperera commented Apr 29, 2025 •
edited
Loading

@danharrin new relationLoaded method also behaves same because it first checks for single level relations, it will switch to new behavior only if $key contains nested relations ('user.posts'). New behavior is also mached 100% to old behavior. I have regression test cases as well to verify the behavior.

The places I have replaced the relationLoaded inside framework are confirmed not for nested relations. It is purely for checking inside $relations array for single level relations. There are some scenarios nested relations are passes to those areas as parameters so it is a breaking change e.g #55535. I have eliminated this kind of breaking by replacing relationLoaded with 'array_key_exists' on those areas.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danharrin danharrin danharrin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tmsperera @danharrin @taylorotwell
0