Model::offsetExists calls getAttribute and may result in double calls

[brunobg]

• Laravel Version: 7,30.2, 8.23.1, dev-master
• PHP Version: 7.4, would apply to others as well
• Database Driver & Version: not related to database

Description:

This is the current implementation of Model::offsetExists():

    /**
     * Determine if the given attribute exists.
     *
     * @param  mixed  $offset
     * @return bool
     */
    public function offsetExists($offset)
    {
        return ! is_null($this->getAttribute($offset));
    }

It calls getAttribute(), which may call actual functions as in the case of accessors. This is a problem when there's a combination of checking/getting:

if (isset($model['myattribute'])) {
   $myvar = $model['myattribute'];
}

For normal fields this is ok, but when you have an accessor, which may not be pure, it's a problem. It may call a database, and you have a performance hit. It can generate logs or other write operations that are now doubled.

This issue is affecting users and has been reported as a side effect in other projects: webonyx/graphql-php#759 and nuwave/lighthouse#1671.

Steps To Reproduce:

Outlined above.

Fix suggestion:

I suggest a hasAttribute method on Eloquent/Concerns/HasAttributes.php that mirrors getAttribute() but doesn't actually call anything:

    /**
     * Determine if the given attribute exists.
     *
     * @param  mixed  $key
     * @return bool
     */
    public function hasAttribute($key)
    {
        if (!$key) {
            return false;
        }

        // If the attribute exists in the attribute array or has a "get" mutator we will
        // get the attribute's value. Otherwise, we will proceed as if the developers
        // are asking for a relationship's value. This covers both types of values.
        if (array_key_exists($key, $this->attributes) ||
            array_key_exists($key, $this->casts) ||
            $this->hasGetMutator($key) ||
            $this->isClassCastable($key)) {
            return true;
        }

        // Here we will determine if the model base class itself contains this given key
        // since we don't want to treat any of those methods as relationships because
        // they are all intended as helper methods and none of these are relations.
        if (method_exists(self::class, $key)) {
            return false;
        }
        return $this->hasRelationValue();
    }

    /**
     * Get a relationship.
     *
     * @param  string  $key
     * @return mixed
     */
    public function hasRelationValue($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 true;
        }

        // If the "attribute" exists as a method on the model, we will just assume
        // it is a relationship and will load and return results from the query
        // and hydrate the relationship's value on the "relationships" array.
        if (method_exists($this, $key) ||
            (static::$relationResolvers[get_class($this)][$key] ?? null)) {
            return true; // TODO: see below
        }
    }

If you agree to it I can send a PR.

The only questionable point is the last if in hasRelationValue(). It assumes that the relation exists from the class data, but it may throw on getRelationshipFromMethod(). There seems to be no more reliable way to do it. The current implementation can throw on isset(), while my suggestion would only throw in offsetGet.

I consider this issue is relatively important, as it doubles operations on non-trivial model accessors for all users.

[taylorotwell]

We likely won't be changing this behavior on a patch release. If your accessor does computationally expensive operations - consider caching or memoizing those results.

[brunobg]

Yes, I can patch my code for this bug, thanks. That'snot the point.

This is not a good behaviour that is likely affecting a lot of people out there. There's absolutely no reason to actually call the accessor on offsetExists(). It should be a pure function and the way it's implemented has side-effects for no good reason.

The implementation is unsound and should at least be considered for the next release. Closing the issue will sweep it under the carpet.

[spawnia]

I think there is very little potential for harm in fixing this, it would only affect users who are somehow reliant on having isset() calls on their models produce some sort of side effect. Very unlikely.

The implementation looks simple enough, we could try a PR and see how the test suite copes and how well that is received. Let's just target the next major version to be totally safe.

[brunobg]

hey @spawnia. Is there any repo that I use that you are not a maintainer? I feel I owe you a beer. Or perhaps you owe me one :D

I'll post a PR tomorrow to get this started. Thanks for the support.

[spawnia]

I tried my hand at implementing your proposed changes in master, looks like the test suite is still passing. Here is the git diff: model-has-attribute-diff.txt

[spawnia]

On a side note, I am not a maintainer of Laravel yet, although i have contributed some small stuff in the past.