[9.x] Model::offsetExists calls getAttribute and may result in double calls

[brunobg]

This patch adds hasAttribute() and hasRelationValue() to the HasAttributes trait. The reason, as outlined in #36026, is that the current implementation of offsetExists() in the Model class calls getAttributeValue(), which can have side effects when calling accessors. This can include simple performance issues such as accessing the database twice, and worse actual non-pure effects such as creating repeated log entries etc. There's no guarantee that accessors are pure and Laravel does not require it anywhere.

While this can be solved by "caching the result" as suggested in the original issue, it requires a) that all developers are aware of this problem, which is not documented anywhere and b) that any accessor implements caching. It's much simpler to change offsetExists to not have side effects.

This patch is unlikely to change the behavior of any application. The only conceivable problem is if the application is expecting that isset($model['something']) actually calls an accessor which has a side effect, and does not call $model['something'] after it. That is unlikely and really awkward code.

OTOH the current code is probably affecting negatively almost every existing Laravel application.

The current test suite covers this change extensively, which is why there's no specific test added. The patch is an exact copy of the existing code without actually calling the accessors.

I'm sending this PR to master as suggested by the reply on the issue, but IMHO it could be safely applied to the 8.x and 7.x branches.

I strongly urge you to consider this PR, please.

[GrahamCampbell]

No tests?

[brunobg]

No tests?

I wrote about this on the detailed post:

The current test suite covers this change extensively, which is why there's no specific test added. The patch is an exact copy of the existing code without actually calling the accessors.

[brunobg]

Still, if you think that some extra test is required I'll be happy to implement it. Just tell me what is not already covered by the current tests.

[taylorotwell]

I'm not making these types of breaking changes to Eloquent. It's hard for individual users of the framework to fathom how many people are using Eloquent and how much even small changes can break applications. If you have an Eloquent accessor that is causing seriously performance hindering side effects, consider just moving that behavior to a method?

[spawnia]

@taylorotwell How about we make the change purely additive, not changing the behaviour of offsetExists()? The hasAttribute() and hasRelationValue() methods are very useful additions for library authors/everyone who writes generic code that deals with models.

[brunobg]

I'm not making these types of breaking changes to Eloquent. It's hard for individual users of the framework to fathom how many people are using Eloquent and how much even small changes can break applications. If you have an Eloquent accessor that is causing seriously performance hindering side effects, consider just moving that behavior to a method?

Well, it's hard for "individual users" to find bugs affecting their applications, see that they are bugs, see that other "individual users" are having them and reporting around, explain how it's very unlikely that any application depends on such unexpected and wrong behavior, and yet be told "won't fix because it's a breaking change". Any bug fix is a breaking change then. Let's just stop releasing new versions. Any bugs find can be fixed by workarounds at the user applications.

As a "non individual user", you should be able to fathom how many people are using Eloquent with proper code and being affected by these bugs and how much bug fixes can improve applications. As a "non individual user", you should be able to fathom how much time is wasted in debugging "why am I getting two queries instead of one" by "individual users". And how "individual users" use Laravel because they imagine it will have bugs corrected as they are found.

So, why there are major versions at all? Because https://laravel.com/docs/8.x/upgrade broke a lot of my applications. I had to follow all that's there to make them work properly again. As an "individual user" I don't understand why I have to fix those things that were working properly before in my application and now broke when I updated, but the things that are broken won't be fixed. Care to explain the rationale please?

[taylorotwell]

Relax... 😄 After your patch isset($user['save']) returns true just because "save" is a method on the Uesr model.

What are you doing in an Eloquent accessor that is causing a performance bottleneck in your application? Let's start there.

[brunobg]

Relax :) I just learned that a long detailed reply is what it takes to grab attention after a wontfix.

My exact scenario, which has been reported by other people too, is that I'm using LighthousePHP, which calls GraphQL PHP, which checks if any requested fields are present in Model with a isset(). This calls the accessor twice. There's full report with stack traces in that issue, but essentially it's this code:

// my model
class Stuff extends Model {
    public function getFancyAttribute() {
       return DB::select('SELECT sometable.id FROM sometable');  // some fancy query here
    }
}

// in graphqlphp there's something more or less like this:
if (isset($stuffModel['fancy'])) {
    return $stuffModel['fancy'];
}

Since isset() actually resolves the data, it calls the accessor twice. From what I gather isset will return false for acessors that return null, but that matches PHP' isset() behavior for normal arrays, so that's fine.

Now, can it be fixed on the user code? Yes, by caching results or overriding offsetExists. But is it reasonable to expect that fix there? I don't think so. First because it's a bug that bites you -- I caught it because I was getting twice as many entries in an access log and that was weird, but otherwise I'd only have noticed it by doing a query profiling. Second, because it's rather sane to expect isset() not to have side effects. GraphQLPHP maintainers refused to fix it there since they expect that. As a user, I'm caught in the middle and trying to avoid even more people falling into this trap.

After your patch isset($user['save']) returns true just because "save" is a method on the Uesr model.

I'm more than happy to discuss a proper implementation -- I tried to keep as close as possible to the current code, and nothing broke on the test suite. But without the patch I guess (sorry, can't check it right now) that isset($user['save']) calls $user->save().

Can this change break some application somewhere? Sure. It's hard to conceive it, but perhaps someone wrote an isset(), forgot to call the actual accessor, and expects that the side-effect from the accessor gets called even if they don't read its return value. Is that likely? No, it's not, and it's not very reasonable code, right. Is the change likely to improve more than break? IMHO: yes. Any sane code that calls isset() followed by fetching data will avoid being called twice. At worst it's a performance improvement, at best it avoids problems. And as @spawnia says, the hasAttribute and hasRelationValue() methods are certainly a good addition.

"But accessors shouldn't have side effects, either" => ideally yes, but they are not required to be pure functions. They can do I/O, so they can call DBs, microservices, logs etc. My use case in particular is a query that triggers a counter -- so things are counted twice.

"But there's no way to be sure if an accessor will return null without running it" => I see that. Reflection might help if the accessor is typed and we want to go to a fancier route. Yet as much as I think that isset() should not exist and array_key_exists() be the only behavior instead, that's how PHP was written and I hope I don't have to escalate this to a RFC there :D There's also nothing forbidding you to return null in an accessor and be completely valid behavior. So there might not be a perfect solution that covers all cases perfectly, but this is certainly being a problem for people around and should merit a more thorough discussion.

"You can solve it at the application level" => true. The main problem is, this is a hidden behavior that can easily be overlooked, since nobody is expecting it. It's also rather cumbersome to write caches for each accessor (and easy to forget).

[suth]

"You can solve it at the application level" => true. The main problem is, this is a hidden behavior that can easily be overlooked, since nobody is expecting it. It's also rather cumbersome to write caches for each accessor (and easy to forget).

The reason I think this is the solution is because in my opinion the hidden behavior is introduced at the application level in this case. The idea that an accessor shouldn't have side effects isn't unique to Laravel, and changing the offsetExists() implementation would only be a bandaid. If simply accessing $model->property is a bottleneck, you'll run into it again somewhere else in your application that a developer wouldn't expect a side effect from reading a property. As for caching the value being easy to forget - it's much easier to forget a property has side effects, and if it's really a bottleneck it won't let you forget.

[brunobg]

The idea that an accessor shouldn't have side effects isn't unique to Laravel

There's no requirement for an accessor to not have side effects. And there shouldn't be. I'm not arguing that at all. The problem is isset() having side effects.

If simply accessing $model->property is a bottleneck

The problem isn't "hey, my method is slow and it's called twice, fix that for me". Or "hey, cache that automatically for me". The problem is "there's a side effect in checking it a property exists on a model". It can be really quick -- in fact, in my particular case I don't have a bottleneck there at all.

it's much easier to forget a property has side effects,

It's not, right? I don't expect methods to be pure unless they specify that. But I don't expect checks to have side effects:

• $class->method() no assumptions about side effects.
• 'method' in $class I expect it not to call method to verify if it exists and what it returns.