Member

This PR introduces a Model::enableStrictLoading() configuration method that when used 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

Contributor

Nice @themsaid

Is there a reason why you omitted a Model::disableStrictLoading() ?

I guess you would argue that you would only enable when needed and only for that php process.

For me its just convinient to have a corresponding disable function.

Contributor

This PR has the same? similar issue as #29089 (comment) has

Try to access a property of a non-persisted model which is actually not a relation -> it will go through \Illuminate\Database\Eloquent\Concerns\HasAttributes::getAttribute but not find the attribute and go end up \Illuminate\Database\Eloquent\Concerns\HasAttributes::getRelationValue and the bombs:

• the expected behaviour is that null is returned
• the same applies if you try to access an attribute whose model has not been fully loaded (=hydrated), e.g. via custom selected columns

    public function testStrictModeThrowsOnAccessUnknownAttribute()
    {
        $model = new EloquentStrictLoadingTestModel1();
        $model->withStrictLoading = true;
        $model->some_attribute;
    }

The test may sound artificial, but just imagine you want a certain model to have this property enabled by default, then this creates the issue described.

Member

@mfn yeah - I could see that being a fairly significant issue.

I think at the end of the day it may not be possible to implement this "strict" loading feature without the ability to explicitly mark relationship methods as such in Laravel. That is of course fairly easy to do now that PHP 8 has attributes but it introduces some additional overhead by needing to reflect the class and determine the relationship methods, etc.

Though maybe you could check if a method exists and assume if a method exists with that name it is likely a relationship?

Member Author

@mfn can your write a failing test? “ $model->withStrictLoading = true;” is not the intented way to use this feature.

Contributor

What's the intended way to set this flag?

Another one:

    public function testStrictModeThrowsOnAccessUnknownAttribute()
    {
        $model = new EloquentStrictLoadingTestModel4();
        $model->some_attribute;
    }
…
class EloquentStrictLoadingTestModel4 extends Model
{
    public $withStrictLoading = true;
}

Member Author

You don’t set this flag. Check the Pr description, enabling this feature is via model::enableStrictLoading()

I agree with @taylorotwell.
My suggestion is we can refactor the method getRelationValue, we can extract the condition check if a method is a relation, something like isRelation($key).
So the code can be like so

   public function getRelationValue($key)
    {
        // If the key already exists in the relationships array, it just means the
        // relationship has already been loaded, so we'll just return it out of
        // here because there is no need to query within the relations twice.
        if ($this->relationLoaded($key)) {
            return $this->relations[$key];
        }
        if (! $this->isRelation($key)) {
            return;
        }
        if ($this->withStrictLoading) {
            throw new StrictLoadingViolationException($this, $key);
        }
        return $this->getRelationshipFromMethod($key);
    }
    /**
     * Check if the "attribute" exists as a method on the model, 
     * or exist as relationResolvers.
     * @param  string $key
     * @return bool
     */
    public function isRelation($key)
    {
        return method_exists($this, $key) ||
            (static::$relationResolvers[get_class($this)][$key] ?? null);
    }

Contributor

@themsaid as I mentioned in #37363 (comment)

the same applies if you try to access an attribute whose model has not been fully loaded (=hydrated), e.g. via custom selected columns

    public function testStrictModeThrowsOnAccessUnknownAttribute()
    {
        Schema::create('test_model4', function (Blueprint $table) {
            $table->increments('id');
            $table->string('optional_link')->nullable();
        });
        Model::enableStrictLoading();
        EloquentStrictLoadingTestModel4::create();
        $models = EloquentStrictLoadingTestModel4::select('id')->get();
        $models[0]->optional_link;
    }
…
class EloquentStrictLoadingTestModel4 extends Model
{
    public $table = 'test_model4';
    public $timestamps = false;
    protected $guarded = [];
}

It's not a problem if at least optional_link = null exists in the models $attributes, i.e. changing the test to this will work and not throw an exception:

        $models = EloquentStrictLoadingTestModel4::get();
        $models[0]->optional_link;

Contributor

@mfn I can see where you're coming from, but I think your last test is a feature and not a bug. Perhaps if you rename this to "strict attributes" (as opposed to strict relationships) you can see how getting a fatal error is desired if I forget to add a column in my select statement.

Edit:

Going through the linked issue for the previous PR, I can still see a valid concern from @mfn in respect to un-persisted models. They are indeed a problematic case and I don't have a formed opinion about them. I wanted to make it explicitly clear that I feel that forgetting a column during a select and getting an exception would be a good thing, though.

Member Author

@mfn isn’t that a bug in your code? Using the value of an attribute you explicitly decided not to load.

Contributor

I like this because if you use a package that has models / endpoints, you can enable in a test to inspect:

public function test_package_n_plus_one()
{
    $this->withoutExceptionHandling();
    Model::enableStrictLoading();
    $this->get('/notifications')->assertOk();
}

Contributor

@themsaid

isn’t that a bug in your code? Using the value of an attribute you explicitly decided not to load.

Maybe, the point is rather the change in behaviour: remember until this PR (and it's titled "throws an exception on lazy loading eloquent relations") access to unknown attributes just yielded null: in all Laravel versions. I'm not saying this is good or bad, just pointing it out.

@deleugpn

in respect to un-persisted models

As per what @themsaid said, the use case I came up with is "made up" in the sense that this isn't how this feature is supposed to used, so un-persisted models are not at risk.

I absolutely like the change 👍 but it should not affect reading non-relation properties in anyway IMHO.

I agree with @mfn.
Missing non-relation properties should return null which is the current behavior.
The exception should be thrown only if accessing a relation.
Or else this would be a breaking change.
So it is important to check first if the key is a relation or not.

Member Author

@mfn I don’t consider this a breaking change since it’ll uncover a bug in the code. Can’t think of a case where the code expects null from an unloaded attribute. How would that code know of the value is null or if the attribute is just not loaded?

we can check for the existence of a relation with the name though, will think about that until tomorrow.

Member Author

@mfn update the PR so it doesn't through an exception in the case you mentioned.

@themsaid Also, I suggest this following code for some benefits:

    public function getRelationValue($key)
    {
        // First, we should check if the relation is already loaded so we can return it immediately. 
        // By doing this, it save us a little execution runtime by avoiding condition checks if key is a relation and 
        // if withStrictLoading is enabled.
        if ($this->relationLoaded($key)) {
            return $this->relations[$key];
        }
        // Second, let us check if key is a relation, if not, we can just return null because we are only after those relation keys.
        if (! $this->isRelation($key)) {
            return;
        }
        // Here is the use-case where the relation is not yet loaded, so we can check if withStrictLoading is enabled
        // then we can throw an exception.
        if ($this->withStrictLoading) {
            throw new StrictLoadingViolationException($this, $key);
        }
        // This is the default use-case where get relationship from method.
        return $this->getRelationshipFromMethod($key);
    }

You can compare that this is similar behavior with the current code because we are priotizing the relationLoaded check first,
then if it is not loaded that's the time we do the logic for getting the relation from method with just extra validation if withStrictLoading is enabled.

Member Author

@ajcastro thanks for your thoughts. I've applied that to the PR ❤️

You're welcome @themsaid. Happy to contribute! 😃

Contributor

This looks really nice now!

I re-checked https://github.com/laravel/framework/pull/29089/files and wondered what the problems were. The cleverness of this PR also comes from setting this flag when calling hydrate(), which the other PR back then didn't and thus suffered the problem that newly created objects "never properly worked".

The flag is called "strict loading", so eventually we can still add "strict attributes" at some point later if we figure things out…

Member

Did a bit of tweaking... can now call preventLazyLoading with an argument to disable in certain situations:

Model::preventLazyLoading(! app()->isProduction());

Yep, and still hard coded environment variables ;/