Implement the compare method on the eloquent casts to improve the isDirty check

[SanderSander]

Resolves #1032

[clementbirkle]

It looks like the Illuminate\Contracts\Database\Eloquent\ComparesCastableAttributes interface might be missing from your implementation.

[SanderSander]

It looks like the Illuminate\Contracts\Database\Eloquent\ComparesCastableAttributes interface might be missing from your implementation.

This is on purpose, If the ComparesCastableAttributes interface would be implement it would mean that the minimal required laravel version becomes 12.18.0 or higher for this package, while the current version contraint is "illuminate/contracts": "^10.0|^11.0|^12.0",.

By not implementing it, we can be sure that that everything is backward compatible. Laravel doesn't check if the interface is implemented, but does a simple check if the method exists on the cast class itself. So the fix would work for 12.18 and higher while staying compatible with earlier versions.

[clementbirkle]

Ah okay, I hadn't considered the backward compatibility aspect — that makes perfect sense. Smart solution!

I just noticed two small things:

1. The comparison doesn't use the nullsafe operator, which means if an attribute is nullable and its value changes from null to a non-null object, it could throw an error.
2. Not 100% sure, but I think toArray() is slightly faster than toJson().

Maybe the comparison could be improved like this:

return $this->get($model, $key, $firstValue, [])?->toArray() === $this->get($model, $key, $secondValue, [])?->toArray();

[SanderSander]

I updated the method so that it uses the toArray method.

For the nullsafe operator, I noticed that the method originalIsEquivalent in the laravel trait hasAttributes does check for null values, so for the implemention we can be sure that at least laravel doesn't pass null into the method.

        if (! array_key_exists($key, $this->original)) {
            return false;
        }

        $attribute = Arr::get($this->attributes, $key);
        $original = Arr::get($this->original, $key);

        if ($attribute === $original) {
            return true;
        } elseif (is_null($attribute)) {
            return false;
        }

I added this case into the unit test, but to play "safe" I could add the nullsafe operator anyway.

Furthermore I updated the typehints in DataEloquentCast, in the set method we check both if the value is an instance of BaseData and TransformableData, The TransformableData interface contains the castUsing method.

[rubenvanassche]

Looking good, could you please take look at the failing tests?

If that's done this one can be merged.

Note for myself quickly cleanup a few parts of the tests.

[SanderSander]

@rubenvanassche Done, I added a check on the laravel version so that the tests are skipped for older versions.

})->skip(fn() => version_compare(app()->version(), '12.18.0', '<'));

One thing is that I see that for laravel 12 the following is used Locking laravel/framework (v12.0.1) so that would mean the tests are never executed. This is because the prefer-lowest stability flag

Nvm, it also uses prefer-stable, all good!

[rubenvanassche]

Thanks!

[jhonaikerf]

Hi @rubenvanassche @SanderSander

From last version we have some erros with this implementation.

We are using nullable dtos, so the method toArray() dont apply to this.

Maybe add the nullsafe operator, which means if an attribute is nullable and its value changes from null to a non-null object.

@clementbirkle made the same comment

[rubenvanassche]

Isn't this the case?

For the nullsafe operator, I noticed that the method originalIsEquivalent in the laravel trait hasAttributes does check for null values, so for the implemention we can be sure that at least laravel doesn't pass null into the method

Otherwise feel free to PR it

[SanderSander]

Hi @rubenvanassche @SanderSander

From last version we have some erros with this implementation.

We are using nullable dtos, so the method toArray() dont apply to this.

Maybe add the nullsafe operator, which means if an attribute is nullable and its value changes from null to a non-null object.

@clementbirkle made the same comment

Hey, can you give me an example, so that I can write a test for your use case?

I have covered the use case with null values, see the following test https://github.com/spatie/laravel-data/pull/1033/files#diff-f7aacc43a3f446f7d13176dbaef1985b860b5fed2e78e4905359c6eb4baefe5bR262

So maybe i missed something, but would like a bit more information because I'm not completely sure how to re-create

[nick-potts]

https://order-editing.sentry.io/share/issue/727b1319eb03410796778ec30cc4df0f/

this is what we're getting. This is going from it being null to non-null.

[SanderSander]

@nick-potts Thanks! I'm making a PR to fix this issue

[mdietger]

We are experiencing the same issue

#message: "Call to a member function toArray() on null"
    #code: 0
    #file: "
/var/www/html/vendor
/spatie/laravel-data/
src/Support/EloquentCasts/DataEloquentCast.php
"
    #line: 91
    trace: {▼
      
/var/www/html/vendor
/spatie/laravel-data/
src/Support/EloquentCasts/DataEloquentCast.php:91
 {▼
        
Spatie\LaravelData\Support\EloquentCasts
\
DataEloquentCast->compare($model, string $key, $firstValue, $secondValue): bool
 …
        › 
        ›     return $this->get($model, $key, $firstValue, [])->toArray() === $this->get($model, $key, $secondValue, [])->toArray();
        › }
      }

There is a null check for $attribute but not for $original.

Link for PR -> #1046