[9.x] Fix isset() throwing when strict mode is enabled

[axlon]

Fixes #44558

Previously discussed in #44283 and #44558

Trying to describe the problem

Recently a new feature was introduced to prevent developers from accidentally accessing attributes that don't exist on their models. While I think this is a really great feature to have, I currently find it unusable due to how it handles isset(). Isset allows developers to 'test' a variable or property before actually using it, preventing errors. For example:

class User
{
    public ?string $name;
}

$user = new User();

// Somewhere else in the code

if (!isset($user->name)) {
    // Is name not set or is it set to null? We don't have a clue!
    // In reality it does not really matter because we know its not a value we can do something with
} else {
    echo "Hello {$user->name}!";
}

Strict models changes this up, by suddenly having models act differently from all other objects (any I've come across anyway):

class UserModel extends Model
{
}

$user = UserModel::select('id')->firstOrFail();

// Somewhere else in the code

if (!isset($user->name)) {
    // 💥 The model throws an exception
} else {
    echo "Hello {$user->name}!";
}

Since Laravel presents models like normal objects with normal properties, it feels very weird when they suddenly don't act like them.

It is still possible to work around all this, but you end up with weird constructions because you have to be sure you're dealing with a model, e.g.:

function sayHello(User|UserModel $user): void
{
    if ($user instance UserModel) {
        $name = $user->getAttributeValue('name');
    } else {
        $name = $user->name ?? null;
    }

    if ($name !== null) {
        echo "Hello $name";
    }
}

But if isset() still worked like normal, this code could have been a lot shorter:

function sayHello(User|UserModel $user): void
{
    $name = $user->name ?? null;

    if ($name !== null) {
        echo "Hello $name";
    }
}

I think its also noteworthy to point out that ?? and ??= also make use of isset() under the hood. This means they can no longer be used on models freely, because it opens you up to potential errors when strict mode is enabled (as is evident by #44558)

What does this PR do?

This PR makes isset() return false when testing an uninitialized model attribute in strict-mode. This creates parity between strict-mode models, and 'regular' objects with typed properties; accessing an uninitialized property will throw an error, testing a uninitialized property will not.
This also immediately fixes the issue linked above, because the framework (and packages) can go back to treating models like regular objects.

[taylorotwell]

I'm honestly getting a bit worried about strict mode as a concept. 🫤

@inxilpro thoughts?

[taylorotwell]

@axlon can you PR a test for this?

[inxilpro]

@taylorotwell I would be OK with this solution as a compromise if most folks agree with it. But I'm not certain most folks do. I know @aarondfrancis and @donnysim chimed in on the other PR. I wonder who else has an opinion.

I also would like to clarify, this will not throw an exception:

$user = new UserModel();

if (!isset($user->name)) {
    // 💥 The model throws an exception
} else {
    echo "Hello {$user->name}!";
}

The bug referenced in #44558 comes down to the fact that the mailable is trying to access $user->name but the application is using a different column (maybe first_name and last_name?). By default, Laravel will work fine. But if you turn on strict mode, the whole point is that it should trigger an exception. If you choose to turn on strict mode, you have to follow stricter rules. That's the trade-off.

(As an aside, I generally dislike the current trend towards strict types, private final readonly nonsense everywhere. I think that this particular feature is a case where "strictness" is useful—it's guarding against an actual, real-world problem, rather than protecting you from theoretically passing a string to an addNumbers function.)

Strict models changes this up, by suddenly having models act differently from all other objects (any I've come across anyway)

Models are arguably different from other objects. They represent data stored elsewhere. They come with the ability to only retrieve a portion of that data. This means that the semantics of isset() are trickier. I think the default, non-strict option is great for many people. For the folks who want the preventAccessingMissingAttributes protection, I think it should apply to both getting the attribute and checking if it's set.

I can say from my experience with a very large Laravel app (100,000's of lines of code, 10,000's of lines of tests), turning preventAccessingMissingAttributes on broke things. But in every case, it was actually a legit bug that had gone unnoticed. I really think having the option is a great tool, and I'd love to see it continue to work as-is.

[axlon]

@taylorotwell I'll send in additional tests (and fix the current one because it contains a false positive) today

@inxilpro in my tiredness (it was almost 12AM when I wrote all this), I seem to have oversimplified my example. But in reality it doesn't matter, the sayHello() function doesn't know (nor should it care) whether the model was just created or if it was (partially) queried, or if strict mode is even turned on at all. I updated the examples to make it a little more clear what I mean

[inxilpro]

But in reality it doesn't matter, the sayHello() function doesn't know (nor should it care) whether the model was just created or if it was (partially) queried, or if strict mode is even turned on at all.

@axlon Your sayHello method should work just fine with strict mode turned on as it was. These would all work:

sayHello(new User());
sayHello(User::create(['email' => 'e@mail.com']));
sayHello(User::find(1));
sayHello(User::inRandomOrder()->first());

The only case that would break is:

sayHello(User::select('email')->first());

And in this case, it should break (when strict mode is on), because your function is interacting with the name attribute on the user but you didn't retrieve the data.

[aarondfrancis]

I agree with @inxilpro on this one.

First, I should say that this PR is an ok tradeoff and if it makes the Laravel team's jobs easier, let's merge it. I don't agree that it's technically correct though.

Strict mode is opt in. And when you opt in there are going to be some behaviors you have to work around. The thing I keep coming back to is that if you call isset on an attribute that wasn't selected, there is no way to provide an honest answer.

Is the attribute set? We don't know, because it wasn't selected from the database. And you may say "well it doesn't matter, we can just treat it as null!" but what if it isn't really null? The entire point of the preventAccessingMissingAttributes is to protect you from that behavior.

If it helps, let's forget not selecting from the database altogether. Let's consider a typo or a renamed attribute.

If you had an attribute named blocked and then later renamed it is_blocked but forget to update it somewhere, you'll get no warning. This is exactly what preventAccessingMissingAttributes is supposed to prevent!

// Oops, used the old name. No warning!
if (isset($user->blocked)) {
    // 
}

Or if you misspell it:

// Oops, typo. No warning!
if (isset($user->is_blokced)) {
    // 
}

I also think the argument that sayHello shouldn't care if the model is partially hydrated is kind of true, but misses the point. You should care!

Change the example to calculateTax and it becomes more clear.

function calculateTax(User $user, $amount)
{
    if (!isset($user->tax_rate)) {
        return 0;
    }

    return $user->tax_rate * $amount;
}

$user = User::select('id')->first();

calculateTax($user, 100);

calculateTax shouldn't care about how the model was hydrated, but you should super care if someone is trying to access tax_rate and it wasn't selected. That's why preventAccessingMissingAttributes exists.

At the end of the day I think the isset use case is so small that it doesn't really matter if this gets merged or not, but I think for the sake of technical accuracy, isset should continue to throw!

[donnysim]

I'll be honest I'm more confused why people use isset on an attribute when you can just compare with null or if you into them functions - is_null, you get the same result, though I use custom hasAttribute function. As I mentioned in original PR I've used strict mode for couple years now and it's either you agree that it throws or you don't enable it, there should be no in between for what it tries to achieve or solve. I even leave it turned on on production without any problems.

I think it's either, the framework commits to it and adds explicit checks for attribute existence in codebase or doesn't and should remove this to avoid further problems.

But hey, for me strictness was never a problem, even in production, heck I would love to have a super lightweight model without all the shenanigans that current implementation has, and I especially dislike not removing outdated features (old way of attribute mutators etc). You just have to adapt to it, the same way I've never used ==, always strict comparison, casts etc., never has been a root cause of a problem if you ensure what you work with is what you expect.

I think people are too stubborn to adjust their code to work with it, for what it's goal is.

[garygreen]

I'm glad to see this has been merged. The whole purpose behind isset is to determine if a property has been defined, it shouldn't throw an exception imo.

When using the null-coalescing operator, this uses isset under the hood, so it does unfortunately mean it silently fails on properties that aren't defined:

$user = new stdClass;
$name = $user->name; // PHP throws
$name = $user->name ?? 'Blah'; // PHP doesn't throw

I think PHP's default behaviour here is reasonable given the purpose of the null-coalescing operator. Maybe in an ideal world when defining declare(strict_types = 1); it could throw an exception in the above, but that's one for the PHP devs.

A middle ground here is to explicitly use getAttribute() function on the Eloquent model when you find yourself needing strictness and using the null-coalescing operator:

$user = User::select(['id'])->first();
$name = $user->getAttribute('name') ?? 'blah'; // MissingAttributeException

$user->name = null;
$name = $user->getAttribute('name') ?? 'blah'; // blah

It's a welcome sight seeing stricter attributes and lazy loading in Laravel! 😀

[donnysim]

So put more thought into it, yeah I can see why you wouldn't want to throw on isset, but at the same time it's just avoiding the point that null can still mean that it exists. I think the big culprit here might be offsetExists checking for null value here instead if it actually exists or not - null doesn't mean it doesn't exist. Also mutators returning null should never be held as non-existing but throw if they access an attribute that is not present without ensuring it's existance. So I think the whole offset exists concept here is incorrect at the fundamental level that prevents proper overall behavior.

I kind of don't know now, it might be that the model is too bloated to properly support both scenarios and will have to take a stance whether it throws on isset or silently hides mutator etc. problems with try/catch, because what most people are talking is about direct attribute access, there's also mutators etc. that also access those attributes and this change hides those errors if used with isset etc.. The isset might fail not because your attribute is missing, but because the mutator accessed other attributes that are not present.

[donnysim]

And as a side note, if the offset does exist, in current situation you're eating a performance penalty because offsetGet executes the same logic all over again.

[garygreen]

but at the same time it's just avoiding the point that null can still mean that it exists. I think the big culprit here might be offsetExists checking for null value here instead if it actually exists or not - null doesn't mean it doesn't exist

That's the way isset works in PHP, so aligning with that behaviour makes sense.

$a = null;
dd(isset($a)); // false

I think what your describing is property_exists

As opposed with isset(), property_exists() returns true even if the property has the value null.

Eloquent models aren't proper objects with defined properties, it's all magically handled in attributes so property_exists would never find the defined attribute, even if it is.

So to handle null would require use of array_key_exists

$user = new User;
$user->name = null;
array_key_exists('name', $user->getAttributes()); // true
array_key_exists('email', $user->getAttributes()); // false

This PR doesn't change the way it's worked in Laravel for many years, it only prevents isset from throwing on strict properties which makes sense.

[donnysim]

Well, you're just confirming my point - people are only thinking about direct attributes, there's more than just attributes we're dealing here - it includes mutators.

public function getTitleAttribute()
{
    return $this->name ?: $this->email;
}

$user = User::query()->first(['id', 'email']);

// what do you actually expect here from?
isset($user->title);

It returns false because name is not present, not because title is not present, but we don't know that now.
There are cases where people override existing attributes with a custom mutators that also include other attributes which could now fail silently for the wrong reason and I think this defeats the purpose of strict mode of catching these kind of bugs.

[sebastiaanluca]

Am currently checking out a recent bug in our app code, and while debugging it, I traced it back to this PR being the breaking change 😬

TL;DR: Very unexpected that the opt-in strict mode setting suddenly started returning false in some parts of our code instead of throwing the missing attribute exception (because you know, we opted in and are counting on it so we can fix things 😅). The optional callback handler was there to report it (or return whatever value you want) and not break the app for the user, but now the feature just doesn't work anymore when using isset() or ??, only when directly accessing the attribute on the model.

Our case:

In a resource class, we want to use the distance if it's defined/set:

return [
    'name' => $this->resource->name,
    'distance' => $this->resource->distance ?? null,
];

On staging and production, we report these to Sentry using a custom callback (Model::handleMissingAttributeViolationUsing();).

With distance missing on the model:

• it reports the exception and returns false on production
• it just returns false locally —no warnings, no exceptions, …

The ?? operator uses the model's offsetExists() to check if the attribute exists (basically an isset()).

I think the only workaround would be to re-throw the exception in the custom callback so it halts in all situations (doesn't work unless you throw a custom exception that's not handled), as I understood there's no further development related to the feature:

• Add hasAttribute method to Eloquent Models #44717
• Callback of Model::handleMissingAttributeViolationUsing called when using isset to check if attribute exists #44834

[inxilpro]

Yeah, it's definitely a bummer that this got so contentious. As far as I can tell, Taylor isn't interested in touching strict mode anymore, so I don't think it's likely that this will get fixed. I think the only option would be to implement offsetExists() in your own model to restore the strict behavior…

[driesvints]

It's indeed super unfortunate that so many got hit by changing behavior. We're now at a point where we don't want to modify or touch anything about this anymore to stop just that.