[9.x] Opt-in Model::preventAccessingMissingAttributes() option

[inxilpro]

Sometimes, an appropriate optimization is to only select the database columns that you need for a specific feature/view. Unfortunately, this comes with the trade-off of having to then keep your select() statement in sync with the attributes you're accessing elsewhere in your app. This can lead to bugs that are harder to catch, because they fail silently.

For example, imagine you have a high-traffic index view for a model, and the model consists of many columns of potentially large data. To speed up that page, you choose to select only the 3-4 columns that you need for that view. Later, you add a flagged field to that model, and update the view to call @if($model->flagged) ..., but forget to update the select() statement in your controller. Your "flagged" logic will happily treat null as falsy, and your app will continue showing flagged items on that view because of the select() oversight.

This PR offers an opt-in solution: Model::preventAccessingMissingAttributes(). When set to true, Eloquent will throw a MissingAttributeException when trying to retrieve an attribute that does not exist (after checking accessors/casts/relations/etc). It will only throw this exception if both the attribute is missing and the model exists in the database. This lets you safely use things like the null coalescing assignment operator on new models while guarding against accidentally treating "null because this wasn't retrieved" the same as "null because this value is null in the database".

For example, this would trigger an exception:

Model::preventAccessingMissingAttributes();

$user = User::select('name', 'email')->first();

$user->status ??= 'new'; 

// Triggers MissingAttributeException: "The attribute [status] either does not exist or was not retrieved."

But, this will still work:

Model::preventAccessingMissingAttributes();

$user = new User();

maybeInferUserStatusSomehow($user);

$user->status ??= 'new';

It's also worth noting that if you want an attribute to always be null by default, you can always set that on a model-by-model case:

class User {
  protected $attributes = [
    'status' => null,
  ];
}

Personally, I never use select() on Eloquent models because of this concern. Having an option to guard against missing attributes would definitely make me more comfortable with making select() optimizations from time-to-time.

Credit for this goes to @aarondfrancis — he started working on something similar on his Twitch stream and then brought on a lively conversation about it on Twitter. This is a slightly different approach to the problem, but is very heavily inspired by that conversation.

[taylorotwell]

Does this do anything to address any of the problems that were associated with this prior PR?

#29089

[inxilpro]

This does address the issue with un-persisted models. I'm running our production tests right now with the flag turned on, and so far the only issues that I've run into are:

• One third-party package that relied on the "null by default" behavior. My thought here is that if you want to use that package, you either PR a fix or don't enable this feature until they account for it.
• Several actual bugs in our code base (places that were doing @if($model->[attribute that has been renamed]) and blissfully continuing, assuming that that attribute was false).

I've confirmed that the relation code example in that PR does in fact work here:

$model = new Model();
$model->relation()->associate($relation); // This works fine

I can dig into MorphTo and HasManyThrough a little more, since that I was brought up in that PR, but I'm fairly certain they work fine.

[inxilpro]

I just also confirmed that both of these cases work fine (both mentioned in the original PR):

Comment::with('commentable')->get();
Country::has('posts')->get();

The only other gotcha I've run into is a piece of code that was checking if a count had been loaded via withCount() and if not, loading it lazily. Something like:

if (! isset($user->friends_count)) {
  $user->loadCount('friends');
}

Had to be refactored to:

if (null === $user->getAttributeValue('friends_count'))) {
  $user->loadCount('friends');
}

It might be nice to add a new helper method for "is this attribute in the $attributes array?" but I was trying to keep the scope of this PR as small as possible…

[axlon]

It might be nice to add a new helper method for "is this attribute in the $attributes array?" but I was trying to keep the scope of this PR as small as possible…

@inxilpro should an exception be thrown at all when checking for property existence with isset? I'd expect isset() to quietly return false just like it would when checking a non-existent property on a "normal" object. An easy way to achieve this would be to try-catch the new exception within the __isset() function, but I don't know if that is desirable

[inxilpro]

should an exception be thrown at all when checking for property existence with isset?

This is something I went back and forth on. Here's the scenario that's not ideal:

// Some call that only selects specific data
$team = $user->teams()->select('id', 'slug')->first();

// Elsewhere…
$team->name ??= "{$user->name} Team";

Now I've accidentally overwritten the team's name because I didn't realize that the name hasn't been retrieved from the database. If the __isset() operation handles this missing attribute in a try/catch, this bug still exists. My preference is that preventAccessingMissingAttributes would result in this triggering an exception. If the value is null, then isset() will return false. But if the attribute wasn't retrieved for an existing model, isset() will throw an exception. In the end, that feels true to the spirit of the feature.

[axlon]

Now I've accidentally overwritten the team's name because I didn't realize that the name hasn't been retrieved from the database.

Maybe I'm misunderstanding here, but to me it seems like the wrong operator was used if the goal was not to assign a property. If the intention is to use this feature to spot the fact that you (accidentally) assigned a property that wasn't selected, then you forgot to cover basic assignments (e.g. $team->name = 'Laravel team').

Right now this would work:

$team = Team::query()->select('id')->firstOrFail();
$team->name = 'Laravel team';

But this would throw:

$team = Team::query()->select('id')->firstOrFail();

if (! isset($team->name)) {
    $team->name = 'Laravel team';
}

To me this doesn't make much sense, because these snippets essentially do the same thing except one of them guards against overwrites. I think this feature should only focus on trying to access a value that was not selected, and should excude isset and set.

[donnysim]

I'm using my version of this (a bit different implementation because I also disable some other features of the model) for a long time and despite some additional code I have to add sometimes like hasAttribute() check, I'm way more satisfied with this than just pure fail silently, can't describe how many times I've got an error and it saved me so much time where I forgot to pick a relation key and it doesn't hydrate the relations or pure errors where the field is missing but is checked by some methods. I can safely say that I'm not going back to the silent fail way ever again.

My only change from my experience would be to include the model type in the error message like:

The attribute [{$key}] either does not exist or was not retrieved for \Models\User model.`

because in loops or mixed lists of multiple models it's easier to pinpoint which model is exactly failing from just a look at the message.

[inxilpro]

@axlon Personally, I think the following is a good balance:

// Works fine with un-persisted models
$user = new User();
isset($user->email); // returns false

// Throws exception when the column is not selected, because I can't know if it's
// not set because it's NULL in the database, or because I didn't fetch the data
$user = User::select('id', 'name')->get();
isset($user->email); // throws a MissingAttributeException

// Works fine when the column is selected
$user = User::select('id', 'name', 'email')->get();
isset($user->email); // returns true if email attribute is not null

@donnysim that's a great point. I'll update the exception.

[aarondfrancis]

@inxilpro that's exactly the behavior I would expect. Calling isset on an attribute I didn't select should definitely throw, IMO, because Laravel doesn't know if I left it out of the selection on purpose or not! (And of course, this is all opt-in, so we have a little more leeway.)

[taylorotwell]

Added Model::shouldBeStrict method to easily enable all of this "strict" behavior added recently. 😎

[ankurk91]

Model::shouldBeStrict() should be added to skeleton

Here is safe way to get unknown properties

$user = User::select(['id', 'email'])->find(1);

$user->getAttributeValue('phone_number');  // null

[mesiarm]

@taylorotwell Trying to access relationship, which is defined with wrong foreign key should also throw error? Now it returns null: #36353