Skip to content
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

[9.x] Eloquent Strict Loading #37334

Closed
wants to merge 8 commits into from

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented May 10, 2021

Update: Moved to the 8.x branch: #37363


This PR introduces a strict_load database connection configuration that when set to true throws an exception on lazy loading eloquent relations.

When the mode is turned on, the following will throw an exception:

$users = User::get();

$users[0]->posts
StrictLoadingViolationException: Trying to lazy load [posts] in model [User] is restricted

However, this will work:

$user = User::find(1);

$user->posts

@mfn
Copy link
Contributor

mfn commented May 10, 2021

Nice, reminds me a bit about #29089

@lancepioch
Copy link
Contributor

Food for thought, it might be better to have this as a default Laravel config setting instead with the ability to override this. Otherwise, it's a simple and clean solution.

{
if ($this->getConnection() &&
$this->getConnection()->getConfig('strict_load')) {
foreach ($models as $model) {
Copy link
Contributor

@cerbero90 cerbero90 May 11, 2021

Choose a reason for hiding this comment

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

To avoid adding a new loop, would it be possible to set the strictLoading property when hydrating models? Something like:

public function hydrate(array $items)
{
    $instance = $this->newModelInstance();

    return $instance->newCollection(array_map(function ($item) use ($instance) {
        return tap($instance->newFromBuilder($item), function ($model) {
            $model->strictLoading = $model->getConnection() && $model->getConnection()->getConfig('strict_load');
        })
    }, $items));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

but array map is not a loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, but it is already present in the Laravel codebase

@peterfox
Copy link
Contributor

peterfox commented May 11, 2021

I like the idea, one thing of note is personally I'd rather this be on a per model basis. I could see it being more accessible to configure it in a service provider like User::StrictLoading() than having to set it across the connection.

*
* @var bool
*/
public $strictLoading;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public $strictLoading;
public $strictLoading = false;

@@ -426,6 +427,10 @@ protected function getAttributeFromArray($key)
*/
public function getRelationValue($key)
{
if ($this->strictLoading && ! $this->relationLoaded($key)) {

This comment was marked as resolved.

@hkan
Copy link

hkan commented May 11, 2021

It might be beneficial to have a configuration option to make it not throw an error but trigger an event in the app. So I can log the occurrences or send notifications to myself about it. Wouldn't be as strict, but still... I wouldn't want end users to see error pages just because my code wasn't optimized enough.

@negoziator
Copy link

I really like this idea.

Would it be possible to add something like Model::enableStrictLoading()
and Model::disableStrictLoading()

Just like with foreignkey checks?

@peterfox
Copy link
Contributor

peterfox commented May 11, 2021

It might be beneficial to have a configuration option to make it not throw an error but trigger an event in the app. So I can log the occurrences or send notifications to myself about it. Wouldn't be as strict, but still... I wouldn't want end users to see error pages just because my code wasn't optimized enough.

I like this idea also.

If it was configured per model maybe there could be a callable parameter.

User::enableStrictLoading(function (string $relationship) {
    return true; // throws exception, return false or anything else, it'll continue as normal
});

Might be a bit clunky but then people can assign their own events etc.

@mathiasgrimm
Copy link
Contributor

I think in the first version it could log a deprecation so that people can gradually fix it then on the next version you could throw an exception.

@mathiasgrimm
Copy link
Contributor

That would also realize all places that needed to be changed. You might have forgotten some place it dont want to break production

@themsaid themsaid changed the base branch from master to 8.x May 12, 2021 16:03
@themsaid themsaid changed the base branch from 8.x to master May 12, 2021 16:03
@themsaid themsaid closed this May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants