[10.x] Use model cast when builder created updated at value

[timacdonald]

fixes #47769

If a model is using a cast on the updated_at column, it is not respected when the eloquent builder creates the updated_at value.

<?php

namespace App\Models;

use App\Casts\UnixTimeStampToCarbon;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class Customer extends Model
{
    use HasFactory;

    protected $casts = [
        'created_at' => UnixTimeStampToCarbon::class,
        'updated_at' => UnixTimeStampToCarbon::class,
    ];
}

When updating a model, in most cases the updated_at value is created in the eloquent model, where the cast is respected as expected.

When the model creates the value the builder receives the updated_at value in the array it is asked to put into the database. When this happens, everything is good.

When the value of updated_at has not changed, i.e. the time is the same as last time it was updated - which you might find happens in a unit test - the updated_at is not marked as "dirty" and the model does not pass the updated_at column through to the builder.

In this case, the builder manually creates and adds the updated_at value to the array it is putting into the database.

When the builder creates the updated_at value it does not use any casts that are in place on the model for the updated_at column.

For timestamp only values, if you are storing the updated_at as an integer, as per #47769, and not other dates, the value attempted to be insert into the database will be 2020-01-01 00:00:00 formatted and MySQL will throw an exception as it is expecting an integer.

Performance implications

I ran some benchmarks using 10,000 iterations.

Updated at set by the builder, i.e. time has not progressed

If you do not have a cast in place for the updated_at column: there is no change ☑️

If you do have a cast in place for the updated_at column: 0.006ms > 0.02ms ⬇️

Updated at set by the model, i.e. time has progressed

0.006ms > 0.0ms (micro optimisation) ⬆️

So the implications are only for the cases where you have a cast in place and time has not progressed, which I would guess is 99.999999999999% of the time in a unit test and not going to impact an actual user.

[willpower232]

Makes sense to me and has @driesvints vote too #47769 (comment)

Do you need me to patch my code and confirm its all hunky dory?

[timacdonald]

@willpower232 that would be super. The test I added replicates the exception and scenario you were hitting, so either way I think this PR makes sense.

However, I would absolutely love for you to actually test this. Let's mark as draft until you can confirm.

[timacdonald]

The outer array_key_exists check is a micro optimisation here, as per the performance section of the description. It can be safely removed if we don't want it.

[timacdonald]

Updated to fix for mutators / attributes

[willpower232]

I swapped your repo and branch in for the framework folder in a testing copy and got the following error from running tests

Undefined array key "updated_at" (expand for stack trace)

Undefined array key "updated_at"

/var/www/vendor/laravel/framework/src/Illuminate/Testing/TestResponse.php:148
/var/www/tests/Feature/Http/Controllers/CustomerControllerTest.php:164
/var/www/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:174

3) Tests\Feature\Http\Controllers\UserControllerTest::testUpdateUserRoles
Expected response status code [201, 301, 302, 303, 307, 308] but received 500.
Failed asserting that false is true.

The following exception occurred during the last request:

ErrorException: Undefined array key "updated_at" in /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:1150
Stack trace:
#0 /var/www/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(254): Illuminate\Foundation\Bootstrap\HandleExceptions->handleError(2, 'Undefined array...', '/var/www/vendor...', 1150)
#1 /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(1150): Illuminate\Foundation\Bootstrap\HandleExceptions->Illuminate\Foundation\Bootstrap\{closure}(2, 'Undefined array...', '/var/www/vendor...', 1150)
#2 /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(1042): Illuminate\Database\Eloquent\Builder->addUpdatedAtColumn(Array)
#3 /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1213): Illuminate\Database\Eloquent\Builder->update(Array)
#4 /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1130): Illuminate\Database\Eloquent\Model->performUpdate(Object(Illuminate\Database\Eloquent\Builder))
#5 /var/www/app/Http/Controllers/UserController.php(45): Illuminate\Database\Eloquent\Model->save()
<further lines removed for brevity>

$timestamp = $this->model->newInstance([$column => $timestamp])->getAttributes()[$column];

I presume this means that the attribute isn't returned by getAttributes for some reason? Am I missing a change to the cast class we have?

[timacdonald]

@willpower232 give it another shot, if you don't mind. Didn't account for attribute guarding.

[willpower232]

Aha that seems to be correct, its passing all my tests now 🎉

[timacdonald]

Thanks for testing this, @willpower232

[willpower232]

No problemo, thanks for figuring it all out!

[maxxscho]

@timacdonald I have a mutator on the updated_at column which prevents setting updated_at on creating a model. If I update a model I get the following error:

ErrorException: Undefined array key "updated_at"
../vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php:254
../vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:1167
../vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:1057
../vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1213
../vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1130

Creating a models works as expected. Do you have any idea?

[willpower232]

@maxxscho are you able to share your mutator code?

[timacdonald]

I’m very sorry, I didn’t see a notification for your message @maxxscho

As @willpower232 asked, if we could please see the mutator we can take a look. It might be good to open an issue and provide the mutator so I can take a look.

Mention me in the issue and I will jump in.

[maxxscho]

@willpower232 sure.

public function setUpdatedAtAttribute($value): void
{
    if (! $this->id) {
        return;
    }

    $this->updated_at = $value;
}

Basically only if the model has an ID (it has been persisted in the DB) the updated_at attribute should get a value (this peace of the software is very old so it might look a little bit odd 😄 )

[willpower232]

Your idea makes sense but shouldn't it be

$this->attributes['updated_at'] = $value;

[timacdonald]

We can likely resolve the issue with

$timestamp = $this->model->newInstance()
    ->forceFill([$column => $timestamp])
    ->getAttributes()[$column] ?? $timestamp;

Funnily enough I actually had this in the original version but removed it cause I couldn’t think of when it would happen - but now I know!

We need to add a test to validate the fix and ensure it doesn’t break again in the future.

I’m on my phone and not in till tomorrow.

will ping @jbrooksuk @driesvints to see if they have time to PR a test with the above fix for the Tuesday release 🙏

[timacdonald]

The ?? $timestamp is for here.

[maxxscho]

@timacdonald I quickly tried this fix and my tests are green again.

[driesvints]

@maxxscho I'm having a hard time reproducing this in a test. Can you share your full model please?

[maxxscho]

@driesvints of course.

<?php

namespace App\Models;

use Carbon\Carbon;
use Illuminate\Database\Eloquent\Builder;

class UserRunStreak extends BaseModel
{
    /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'user_id',
        'current',
        'record',
        'updated_at',
    ];

    protected $casts = [
        'current' => 'integer',
        'record' => 'integer',
    ];

    /**
     * Set updated_at attribute only on updates, not create.
     */
    public function setUpdatedAtAttribute($value): void
    {
        if (! $this->id) {
            return;
        }

        $this->updated_at = $value;
    }

    /**
     * Increase current.
     */
    public function increaseCurrent(): void
    {
        $this->current += 1;
        $this->save();
    }

    /**
     * Increase record.
     */
    public function increaseRecord(): void
    {
        $this->record += 1;
        $this->save();
    }

    /**
     * Reset current.
     */
    public function resetCurrent(): void
    {
        $this->current = 0;
        $this->save();
    }

    /**
     * Scope query to filter active results.
     */
    public function scopeNotUpdatedFor24hours(Builder $query): Builder
    {
        return $query->where('updated_at', '<', Carbon::now()->subDay()->toDateTimeString());
    }
}

[driesvints]

@maxxscho do you somehow manually set the updated_at when creating a model?

[maxxscho]

@driesvints no, it's not set manually on creating. The reason for the mutator is to keep updated_at null on creating.

[maxxscho]

@driesvints a little bit off-topic:
I'd love to help on this issue but I never contributed to Laravel. I forked the repo, cloned it, installed via composer install and tried to run the tests but all MySQL tests are skipped with the message Test requires a MySQL connection. I didn't find any documentation how to set up MySQL for the tests. Currently I don't have that much time - will take a look after the work into this.
Or is there a simple step I missed? (Sorry to bother you with this 🙂 )

[driesvints]

I just have too few info to reproduce this sorry. Can you create a repo for me with an app that reproduces this?

laravel new bug-report --github="--public"

[driesvints]

Nvm, got it: #48230. Thanks

[maxxscho]

@driesvints and @timacdonald thx for this quick help 🚀