Inconsistant Cast Behavior for Eloquent

[ragingdave]

When using the fill method, any $casts are ignored. Ex.

'valid' => 'boolean'

Works: $object->valid = 'something'; // Will convert to a 1 for database storage

Doesn't work: $arr = ['valid' => 'something']; $object->fill($arr); // will stay as 'something'

Temporary Fix is just to make a setValidAttribute($valid) mutator but I would think that in order to have consistent behavior across the board, $casts should be respected anyway that attributes can be set.

[GrahamCampbell]

Looks like a bug to me.

[ibrasho]

I'm pretty sure that $object->valid = 'somthing'; will not cast the value unless you try to access it.

Try to dd() or dump() the object after you do that to see the actual value.

So this is expected behaviour.

[ibrasho]

Both Model::fill() and Model::__set() use Model::setAttribute() to set the attribute value.

[ragingdave]

Hmm... I somehow am not able to replicate this again, maybe I was doing something funky. Either way to add on/change this bug a bit. The documentation says the following: "Otherwise, you will have to define a mutator for each of the attributes, which can be time consuming." which is what led me to believe that the cast would occur in setting and getting.

On top of that there is some implicit casting going on if you have a json castable item. as seen in Model::isJsonCastable() which is called from Model::setAttribute(). I would say there should either be cast checking for any column set as castable or none as this was actually a bit confusing.

[FractalizeR]

Model::setAttribute() doesn't make any casts in 5.1 source. Only Model::getAttribute() does. Is this intended?

[ragingdave]

...It actually does.... https://github.com/laravel/framework/blob/5.1/src/Illuminate/Database/Eloquent/Model.php#L2766 https://github.com/laravel/framework/blob/5.1/src/Illuminate/Database/Eloquent/Model.php#L2770 both of those reference implicit casting for specific cases which is where this ticket came from.

[FractalizeR]

@ragingdave But that relates only for dates and json casts. Not $casts attrubute related fields

[ragingdave]

Again actually it does.... https://github.com/laravel/framework/blob/5.1/src/Illuminate/Database/Eloquent/Model.php#L2685 leads to https://github.com/laravel/framework/blob/5.1/src/Illuminate/Database/Eloquent/Model.php#L2676 So it in fact does deal with $casts. It doesn't care specifically about casting it using castAttribute, but it does cast an array/json/object/collection to a string using json_encode.

[rkgrep]

I guess it was done for a reason - according to castAttribute method phpDoc casting is made to a native PHP type. So it is made to avoid incorrect handling of values from different database types (e. g. MySQL uses integer to implement booleans while PostgreSQL has native boolean type).

You should control types written to database from you code while Eloquent prevents your code from getting unexpected types.

This differs from JSON - which must always be encoded to string before passing to PDO.

[ragingdave]

Wouldn't that specific of casting happen in the specific connection? I assume eloquent shouldn't care about the underlying PDO that is being used, so in that train of thought any casting to handle that type of requirement at a storage engine level should not be a Model but instead be in the Connection layer perhaps when the statements get prepared and whatnot to convert specific types to database friendly ones?

[rkgrep]

@ragingdave Ideally - it is. But actually this is possible only by analyzing schema configuration (because TINYINT(1) in MySQL is not always boolean). But migrations are not present in all projects - e. g. I don't duplicate migrations on projects using the same DB. And also this would be very slow - as it is needed to execute all migrations one by one to get final schema configuration.

So implementing this in a Model is much more handy - you always know which fields you will get from DB and which casting type you should use. And the same vice versa - you always know what kind of data you should store in a Model field.

I think if casting will be enabled on attribute setting, then peoples will tend to avoid proper validation (mistakely believing the Model will properly cast values), thus causing unexpected data written to database.

[wlepinski]

I had a similiar problem today regarding the casting implementation. I was using the 5.0.31 version with a custom Pivot class. On this class I had:

protected $casts = [
  'details' => 'array'
]

I tried to update to v5.0.33 and I noticed that when I asked for the value of details using my pivot I was receiving a string instead of an array. I did a diff between v5.0.31 and v5.0.33 and found 2 method calls on Pivot.php class.

$this->forceFill($attributes);
$this->syncOriginal();

The forceFill method seems to be the problem in my case, it calls the setAttribute method that checks the attribute using isJsonCastable and encode the value twice. So for a object stored as { c1: "on" } as string becomes serialised version of the string "{ c1: \"on\" }" which cannot be decoded using json_decode.

For me, as a workaround I had to implement the method getDetailsAttribute on the Pivot and remove the property from $casts array to avoid the double encoding problem.

I'm still figuring out a more smart solution.

[algorhythm]

Why is this issue 'closed'? Is it fixed? You've referenced my originally issue 'Laravel Eloquent Model 'getDirty()' behaviour #10735' to this one and now it's closed?!

[mfn]

I was hit with a strange issue in our codebase today, more related to https://github.com/laravel/framework/issues/10792 actually:

• assume an attribute some_flag using the following migration for a MySQL database: $table->boolean('some_flag')->default(0);
• have a $casts on the model: 'some_flag' => 'boolean',
• fetch model from DB, $model->some_flag is returned as false (properly cast, everything good!)
• I now perform: $model->some_flag = false;
• The output of $model->getDirty() is (using artisan tinker):

>>> $model->getDirty()
=> [
     "some_flag" => false,
   ]

Why this? Because the $original value is 0 and not false:

>>> $model->getOriginal('some_flag')
=> 0

and getDirty() does this comparison (reformatted from original code for smaller line lenght):

if (
    $value !== $this->original[$key] &&
    ! $this->originalIsNumericallyEquivalent($key)
   ) {

In the end, it's doing the check false !== 0.

Casts are not considered here, which may impact your expectations:

• if you use getDirty() to rely on changes to a model (e.g. for recording)
• $model->save() will cause unnecessary saves to the database

[paslandau]

Can confirm that the issue still exists. It does also apply to MySQL decimals btw.

Im using this workaround for now.

[badeend]

I'm having the exact same problem as @mfn here.

[iget-master]

I related the same issue on #13742 with MySQL decimal fields.

The problem looks like be on this method:

    protected function originalIsNumericallyEquivalent($key)
    {
        $current = $this->attributes[$key];
        $original = $this->original[$key];
        return is_numeric($current) && is_numeric($original) && strcmp((string) $current, (string) $original) === 0;
    }

The comparation is_numeric($original) && strcmp((string) $current, (string) $original) === 0 does not compare properly, since the decimal field from mysql cames as "14.00" and the float value 14.00 is casted to string as "14".

Shouldn't this values be casted to float and compared?

    protected function originalIsNumericallyEquivalent($key)
    {
        $current = $this->attributes[$key];
        $original = $this->original[$key];
        return is_numeric($current) && is_numeric($original) && (float) $original == (float) $current;
    }

[murwa]

This issue still persists for laravel 5.1.*

I have a Location model, with a polymorphic relation to a couple of other models. On this models, I have a casts defined as:

protected $casts = [
    'address_components' => 'json',
    'types'              => 'json',
    'components'         => 'json',
 ];

From the related models, when saving I'm doing:

$this->location()->create($location);

then update like so:

$this->location()->update($location);

The save method causes a double cast on the defined fields, causing the end result to be a string. However, update works as expected.

[cdarken]

This is still an issue on laravel 5.2.45. If I call getAttributes() on a model and then create another object with those attributes, on save the attribute casted as array is double encoded.

My fix was to override the method getAttributes() in a BaseModel class in my app like so:

    public function getAttributes()
    {
        $attributes = parent::getAttributes();

        foreach ($attributes as $name => $attribute) {
            if ($this->isJsonCastable($name)) {
                $attributes[$name] = $this->fromJson($attribute);
            }
        }

        return $attributes;
    }

[mfn]

Expanding on my https://github.com/laravel/framework/issues/8972#issuecomment-190656490 : We're switching from MySQL to Postgres and notice these problems are gone. In our test suite we do a (few) less SQL UPDATE statements, because the getDirty() comparison less often triggers a dirty object.

The reasoning behind this is that we use the ->boolean() type in our migration which, in postgres due native boolean support, is the actual postgres type boolean which ends up back in Laravel natively as boolean too, returned from the PDO driver.

As such, for my case, it's basically solved.

It seems this either needs a in-between-layer between taking the PDO result and hydrating the data into Laravel, specifically for MySQL, or MySQL gets (or has aleady?) a native boolean type too.

[bencromwell]

@mfn shouldn't you technically be able to cast the result of any int column to a bool with Eloquent's casts property, even if MySQL did have native booleans that you could migrate to? If so it would suggest the resolution should be above the PDO layer rather than in it - it would be a valid use case even on Postgres.

[mfn]

@mfn shouldn't you technically be able to cast the result of any int column to a bool with Eloquent's casts property, even if MySQL did have native booleans that you could migrate to?

Mind you that this "casting" only happens when you invoke the attribute getter from a model.

But when you retrieve a model, fresh from the DB, the hydrated values internally in the model, stored in $original are not cast. But these values are referenced when determining whether the model is dirty or not.

To given an example with a boolean:

• since MySQL doesn't have a native boolean type, the $original will contain the value 1 for a truthy value (people usually just use TINYINT(1) for this; even https://github.com/barryvdh/laravel-ide-helper thinks so and will update the models PHPDOC in such cases with boolean)
• When you invoke the getter, with a cast, you get an actual true
• Now assume you set the value to true. Right, makes no sense, the value is already true.
• Wrong, however: the value in $original is still the 1. You set a true so the getDirty() comparison will, in fact, determine that the model is dirty, because 1 !== true
• This will lead to needless updates in the database

As long as you're using the native boolean type in PgSQL, this won't happen there because $original will already be filled with an actual true value and as such getDirty() will in such a case not determine that the model is dirty.

What's with the contrived example, setting the value to the same value again?

Well, just think about a PATCH API endpoint. The request issues some updates to boolean fields, setting some values to true. Now even if that value in MySQL would be 1 already, but because of the above mentioned comparison failing, we still would trigger updating the model. And suddenly things are not so contrived anymore.

Granted, it's a small thing but if you're concerned what's going on under the hood, these things may trip you over and left wonder.

All examples I have with 1 and true equally apply to 0 and false too.

Also see https://github.com/laravel/framework/issues/10792 (which was closed a kind-a dupe of this one). Also 2: I gave already thorough example I just realized, see https://github.com/laravel/framework/issues/8972#issuecomment-190656490

[bencromwell]

I have ended up with this workaround in my base model if anyone is interested.

    /**
     * Get the attributes that have been changed since last sync.
     *
     * @return array
     */
    public function getDirty()
    {
        $dirty = [];

        // cast any original attributes to the types they are expected to be
        foreach ($this->original as $key => $value) {
            if (isset($this->casts[$key])) {
                $this->original[$key] = $this->castAttribute($key, $value);
            }
        }

        foreach ($this->attributes as $key => $value) {
            // cast any of the current attributes to the types they are expected to be
            if (isset($this->casts[$key])) {
                $value = $this->castAttribute($key, $value);
            }

            if (! array_key_exists($key, $this->original)) {
                $dirty[$key] = $value;
            } elseif ($value !== $this->original[$key] &&
                                 ! $this->originalIsNumericallyEquivalent($key)) {
                $dirty[$key] = $value;
            }
        }

        return $dirty;
    }

Section of a now passing unit test for a model using casts:

        /** @var Asset $asset */
        $asset = Asset::findOrFail(1);
        $asset->is_active = true;
        $asset->save();

        $asset = Asset::findOrFail(1);
        $isActive = $asset->is_active;
        // probably unnecessary to wrap the extra equality check
        // however, considering the behaviour in question concerns casting I went with an explicit strict check here
        $this->assertTrue($isActive === true);

        $asset->is_active = true;
        // property hasn't changed so it should be clean
        $this->assertFalse($asset->isDirty());

Relevant part of the model itself:

    protected $casts = [
        'is_available' => 'boolean',
        'is_active' => 'boolean',
    ];

[robclancy]

Hey @GrahamCampbell still up to the same old bullshit I see. This bug still exists but you closed it without saying why. There are actually various bugs I keep running into with original attributes since they don't use casts or mutators. But why would I try open a new issue about it when it will just get closed for no reason?

I use $casts with boolean for an attribute. I have controller update this (sets the same value, true to it). It is now dirty because in the database it is 1 and the attribute is seen as true due to casts (same would happen with mutators).

Original values need to be using the casts and mutator systems or there needs to be a second set of $this->original for this instead of doing things like the hacky originalIsNumericallyEquivalent.

Reopen the damn issue.

[robclancy]

I have created a trait to fix this for my needs. It doesn't account for some things I don't need but will now work for all casts except object and mutators that return a scalar or array.

This is done on Laravel 5.3.x. But from what I looked at for 5.4 the only changes made were moving code to traits, not changing it.

The trait:

<?php

namespace App;

// TODO: remove when bug is fixed, https://github.com/laravel/framework/issues/8972
trait FixEloquentDirty
{
    // NOTE: this still doesn't handle appends or extra casts. I could have copy pasted
    // a bunch of stuff from `attributesToArray` but I don't need it anyway for this.
    // The proper fix for this should be using refactored methods from `attributesToArray`
    // NOTE 2: the methods I was talking about that should be refactored in `attributesToArray`
    // have been refactored in Laravel 5.4 (except where it handles appends)
    public function syncOriginal()
    {
        foreach ($this->attributes as $key => $value) {
            $this->originalCasted[$key] = $this->getAttributeValue($key);
        }

        foreach ($this->getDates() as $key) {
            if (! isset($this->originalCasted[$key])) {
                continue;
            }

            // needs to be the same as the database, skipping the models `$this->dateFormat`
            $this->originalCasted[$key] = $this->asDateTime($this->originalCasted[$key])
                ->format($this->getConnection()->getQueryGrammar()->getDateFormat());
        }

        return parent::syncOriginal();
    }

    // this probably doesn't need that hacky `originalIsNumericallyEquivalent` anymore either so removed it
    public function getDirty()
    {
        $dirty = [];
        foreach ($this->attributes as $key => $value) {
            // this clearly needs a refactor
            if (! array_key_exists($key, $this->originalCasted)) {
                $dirty[$key] = $value;
            } elseif (is_scalar($value) && $value !== $this->originalCasted[$key]) {
                $dirty[$key] = $value;
            } elseif (is_array($value) && $value != $this->originalCasted[$key]) {
                $dirty[$key] = $value;
            }
            // TODO: checking Arrayable here would make it work with collections
            // then StdClass could be made compatable from `$casts` `object` if
            // you compare a json_encode or convert to array and compare
        }

        return $dirty;
    }
}

And using it in my base model, note that you need to set protected $originalCasted = []; in the model or it won't work.

<?php

namespace App;

... 

use Illuminate\Database\Eloquent\Model as BaseModel;

class Model extends BaseModel
{
    // TODO: remove when bug is fixed, https://github.com/laravel/framework/issues/8972#issuecomment-279907478
    use FixEloquentDirty;
    protected $originalCasted = [];

    ...
}

That is the way I envision it being fixed since I have had various issues with original and actually needing them run through casts and mutators. However it could be slow and give eloquent a performance hit in general so maybe instead, just doing some basic checks like running through $casts and if something is set to bool also check for false vs 0 and true vs 1.

[robclancy]

Honestly it needs to be either checking what the model has like what I am doing above (bad for performance but good for me until fixed properly), store what is dirty as it is set (this is a common way to do it and doesn't impact performance), or compare what you got from the database to what you will set in the database... anything else, like what is happening now, is a hack and will have bugs of some form.

[cjaoude]

looks like $casts is just a getter. so use a mutator to enforce persisting the right data type.

it would be cool if there was a $mutates option just for casting types on mutation, rather than create individual mutators just for that

...it would actual be nice if the docs were more explicit on what something doesn't do.

[vesper8]

still happening with Laravel 5.4

Is there an easy way to make $model->fill() apply the $casts ? I don't understand about mutators, never used them. Can someone provide an example please?

And why doesn't this get patched? Leads to very inconsistent behavior

[fahmilatib]

Yeah still happening with laravel 5.4

[halaei]

@vesper8 I will be happy if someone fix it. I tried to fix it but the patch was rejected and I don't know why! Was it because the patch was wrong or @taylorotwell have a better solution in mind, or he just doesn't think it should be fixed at that time?

https://github.com/laravel/framework/pull/17474#issuecomment-274508335

[errogaht]

has this issue with boolean attributes, i have casts for few fields as boolean and when updating they are always dirty because original values is 0 or 1 so a make this trait to fix it

<?php

namespace App\Traits;

trait ModelCustomization
{
    public function setAttribute($key, $value)
    {
        if ($this->hasCast($key, 'boolean')) {
            $value = (int)$value;
        }
        parent::setAttribute($key, $value);

        return $this;
    }
}

[aocneanu]

@errogaht Very nice solution. I named it 'RevertBooleanCasts'

[martintern]

Laravel 5.5 seems to have fixed it. I made a test for it:

<?php namespace Tests\Unit;

use App\Models\Account;
use Illuminate\Foundation\Testing\DatabaseTransactions;
use Tests\TestCase;
use Illuminate\Foundation\Testing\RefreshDatabase;

class IsDirtyTest extends TestCase
{
    use DatabaseTransactions;

    /**
     * @test
     * @dataProvider booleanProvider
     * @param $first
     * @param $second
     * @param $expected
     */
    public function boolean_casting_should_not_make_model_dirty($first, $second, $expected)
    {
        $account = Account::where('active', '=', $first)->first();

        $account->active = $second;

        $this->assertSame($expected, $accountJob->isDirty());
    }

    public function booleanProvider()
    {
        return [
            [1, 1, false],
            [1, true, false],
            [true, true, false],
            [1, 0, true],
            [1, false, true],
            [true, false, true],
            [0, 1, true],
            [0, true, true],
            [false, true, true],
        ];
    }

}

And now I see Laravel 5.5 tests this is as well in testDirtyOnCastOrDateAttributes(): https://github.com/laravel/framework/blob/5.5/tests/Database/DatabaseEloquentModelTest.php (72)

[tflori]

It's not that easy to update a large application, tho it would be nice if they can port this fix back to 5.3 (?)

[crashkonijn]

I created a trait which includes the fixes to 5.5. Tested with L5.3.

[rmccullagh]

Why is this closed? This is a huge bug that should be taken seriously.

[rmccullagh]

For anyone reading this, ensure that your JSON string stored inside your column is actually JSON, and not a string of JSON text. For example,

The column should look like this:

{"Foo": "Bar"}

NOT this:

"{"Foo": "Bar"}"

[milewski]

This issue still exists in laravel 7.... a quicky workaround :

$castedDirty= collect($model->getDirty())->map(fn($value, $key) => $model->getAttribute($key))->toArray()

[NicoHood]

I am not sure, is this still an issue with version 9?

[Moumit]

Just override

    public function getDirty()
    {
        //Getting data from parent
        $dirty = parent::getDirty();

        //add type check
        foreach ($this->attributes as $key => $value) {
            if (!array_key_exists($key, $dirty)) {
                if (gettype($this->original[$key]) != gettype($value)) {
                    $dirty[$key] = $value;
                }
            }

        }

        return $dirty;
    }

[SergkeiM]

I am not sure, is this still an issue with version 9?

Still an issue