This should be directed towards 13.x/master because it contains breaking changes in the function definitions.

This should be directed towards 13.x/master because it contains breaking changes in the function definitions.

Thank you for the comment. I now changed the code so the original signatures remain the same

How does this affect situations where the value of the attribute you are trying to access is affected by a mutator which depends on an unrelated cached class cast?

@taylorotwell That is similar with the question no one replied to here #31778 (reply in thread)

So there are cases like that. Can you please give an example?

Update:

If the get Mutator uses $this->attributes this can happen

$model = User::query()->firstOrFail();

$carbon = $model->date_time_carbon_casted;

$carbon->addDay();

echo $model->col_with_get_mutator_that_depends_on_date_time_carbon_casted;

// will print 'value date time' without the added day.

In current scenario getAttributeValue will call transform on getAttributeFromArray that will call getAttributes that will merge all casted objects to prevent the above.

A workaround for performance is to use in the mutator $this->getAttributeFromArray if you have object cast for that column or getAttributeValue if you want the related value transformed.

@taylorotwell I added a new test to cover the mutator cast dependency:
\Illuminate\Tests\Integration\Database\DatabaseEloquentModelCustomCastingTest::testMutatorCanDependOnAnotherCastedAttribute

I am not sure if this covers the exact case that you described. Please provide a code example of the situation you are concerned about if the test does not already cover it.

@taylorotwell would be great if you could give this a quick look again to let me know if my test covers your concern. thank you

Not sure what the regular review cadence is or if the ticket is already part of some further process.
Pls let me know if you need any further input/tests from my side.

@ug-christoph

A comment above was updated with an example of what I'm talking about as a potential breaking change / behavior with dependent attributes:

Update:

If the get Mutator uses $this->attributes this can happen

$model = User::query()->firstOrFail();

$carbon = $model->date_time_carbon_casted;

$carbon->addDay();

echo $model->col_with_get_mutator_that_depends_on_date_time_carbon_casted;

// will print 'value date time' without the added day.
In current scenario getAttributeValue will call transform on getAttributeFromArray that will call getAttributes that will merge all casted objects to prevent the above.

A workaround for performance is to use in the mutator $this->getAttributeFromArray if you have object cast for that column or getAttributeValue if you want the related value transformed.

Basically, if you have a cast attribute that in its get depends on the value of another cast attribute it is possible it will not have been synced back yet and thus you would get an old value.

If someone was in this situation, they would need to switch to using getAttributeFromArray(anotherAttribute) inside their mutator that depends on other attributes.

@marius-ciclistu Would you mind adding a test case to my branch (or as a comment here) that reproduces your case.

To my understanding this test case that I added does exactly what you mentioned and it passes just fine:
https://github.com/laravel/framework/pull/57627/changes#diff-dd9fdc80583388d2a34ebee0db7bd9c258d28d073ea6a8e3c52c25766f03f9faR291

Thank you

@ug-christoph your test is not covering that scenario. You have an address virtual column cast and you modify its columns that are not casted to objects.

Forget about address. Think of 2 columns like date_of_birth and time_of_birth. Dob casted to Carbon. One is date in db and the other one is timestamp. (this is the only example I can think of now).

$model = Model::query()->firstOrFail();

// by accesing the dob column it will be placed in that property array for casts objects
$dobCarbon = $model->date_of_birth;


$dobCarbon->addDay();

echo $model->time_of_birth; //should contain that extra day.


// the get mutator or better said accessor for time_of_birth returns the year month day from DOB and the time from timestamp as timestamp. 

In my solution you can see that when transform is called I merge All casts:

@marius-ciclistu I think I was able to reproduce your case:

model:

    protected $casts = [
        'date_of_birth' => 'date',
        'time_of_birth' => 'datetime',
    ];

    protected function timeOfBirth(): \Illuminate\Database\Eloquent\Casts\Attribute
    {
        return \Illuminate\Database\Eloquent\Casts\Attribute::get(function ($value) {
            $dateAndTimeOfBirth = $this->date_of_birth->toDateString() . ' ' . Carbon::parse($value)->toTimeString();

            return $dateAndTimeOfBirth;
        });
    }

test:

    public function testCastedAttributeInMutator()
    {
        $model = new TestEloquentModelWithCustomCast(
            [
                'date_of_birth' => '2000-11-11',
                'time_of_birth' => '11:11:00',
            ]
        );
        $model->date_of_birth->addDay();
        $this->assertSame('2000-11-12 11:11:00', $model->time_of_birth);
    }

However, this test is failing in my PR, but also in the current laravel master branch. So this is not a breaking change that my PR introduces, but a bug/edge case in the current laravel code.

Try with getTimeOfBirth old accessor version. I remember this new one is buggy.

@marius-ciclistu

same behavior with this:

    protected function getTimeOfBirthAttribute(string $value): string
    {
            $dateAndTimeOfBirth = $this->date_of_birth->toDateString() . ' ' . Carbon::parse($value)->toTimeString();

            return $dateAndTimeOfBirth;
    }

    protected function getTimeOfBirthAttribute(?string $value): ?string
    {

        if ($value === null) return null;

        if (isset($this->attributes['date_of_birth']) {
            return Carbon::parse($this->attributes['date_of_birth'])->toDateString() . ' ' . Carbon::parse($value)->toTimeString();
        }

        return Carbon::parse($value)->toDateTimeString();
    }

in accessors you access the attributes directly.

@marius-ciclistu same thing.

This code/test already fails in the master branch. Not just in my PR.

Did you try with time of birth as full date time value instead of just H:m:s? The cast is datetime and your test has only time string.

@taylorotwell The scenario of @marius-ciclistu that you mentioned in your comment turned out to be a bug/edge case that is already the case in the master branch.

i.e.
custom casts still work as expected (covered in the test that I added in the PR)
datetime and timestamp casted attributes that are used in mutators do not work as expected in both master and the PR

So this scenario is independent of my PR and should probably be treated in a separate bugfix.

Can you provide a coded testcase for a breaking change that is introduced in my PR that passes in master, but not my branch?

Thank you

@ug-christoph we need a test that uses getClassCastableAttributeValue (see isClassCastable method from model).

@marius-ciclistu isnt this already the case here?
https://github.com/laravel/framework/pull/57627/changes#diff-dd9fdc80583388d2a34ebee0db7bd9c258d28d073ea6a8e3c52c25766f03f9faR291

It would be if you apply from the above example dob to address_line_one and tob to address_line_two analogy and you create AddressLineOne and AddressLineTwo casts analog to the above.

@ug-christoph

class DOBCaster implements CastsAttributes
{
    public function get($model, $key, $value, $attributes)
    {
        if ($value === null) return null;
        return Carbon::parse($value);
    }

    public function set($model, $key, $value, $attributes)
    {
        if ($value instanceof Carbon) return [$key => $value->toDateString()];

        if ($value === null) return [$key => null];

        return [$key => (string)$value];
    }
}

$m = new class ([
            'date_of_birth' => '2000-11-11',
            'time_of_birth' => '2000-11-11 11:11:00',
        ]) extends Model {

            protected $casts = [
                'date_of_birth' => DOBCaster::class,
            ];

            protected $fillable =[
                'date_of_birth',
                'time_of_birth',
            ];
            protected function getTimeOfBirthAttribute(?string $value): ?string
            {
                echo $value.'</br>';
                echo $this->attributes['date_of_birth'] .'</br>';
                echo $this->date_of_birth.'</br>';
                if ($value === null) return null;

                if (isset($this->attributes['date_of_birth'])) {
                    return Carbon::parse($this->attributes['date_of_birth'])->toDateString() . ' ' . Carbon::parse($value)->toTimeString();
                }

                return Carbon::parse($value)->toDateTimeString();
            }
        };

        $DOB = $m->date_of_birth;
        $DOB->addDay();

        echo $DOB->toDateTimeString().' DOB</br>';
        echo $m->date_of_birth.' $m->date_of_birth</br>';

        dd($m->time_of_birth);

prints

2000-11-12 00:00:00 DOB
2000-11-12 00:00:00 $m->date_of_birth
2000-11-11 11:11:00
2000-11-12
2000-11-12 00:00:00
"2000-11-12 11:11:00"

@marius-ciclistu if you can it might be helpful to share the exact test we should add.

@taylorotwell
@ug-christoph

--- <unnamed>
+++ <unnamed>
@@ -291,11 +291,12 @@
     public function testMutatorCanDependOnAnotherCastedAttribute()
     {
         $model = new TestEloquentModelWithCustomCast([
-            'address_line_one' => '110 Kingsbrook St.',
-            'address_line_two' => 'My Childhood House',
-        ]);
-        $model->address->lineOne = 'Changed St.';
-        $this->assertSame('Changed St. (My Childhood House)', $model->address_string);
+            'dob' => '2000-11-11',
+            'tob' => '2000-11-11 11:11:00',
+        ]);
+
+        $model->dob->addDay();
+        $this->assertSame('2000-11-12 11:11:00', $model->tob);
     }
 }
 
@@ -314,6 +315,8 @@
      * @var array
      */
     protected $casts = [
+        'dob' => DOBCaster::class,
         'address' => AddressCaster::class,
         'price' => DecimalCaster::class,
         'password' => HashCaster::class,
@@ -349,6 +352,20 @@
             return "{$address->lineOne} ({$address->lineTwo})";
         });
     }
+
+    protected function getTobAttribute(?string $value): ?string
+    {
+        if ($value === null) {
+           return null;
+        }
+
+        if (isset($this->attributes['dob'])) {
+            return Carbon::parse($this->attributes['dob'])->toDateString() . ' ' .
+                Carbon::parse($value)->toTimeString();
+        }
+
+        return Carbon::parse($value)->toDateTimeString();
+    }
 }
 
 class HashCaster implements CastsInboundAttributes
@@ -376,6 +393,31 @@
     public function set($model, $key, $value, $attributes)
     {
         return [$key => strtoupper($value)];
+    }
+}
+
+class DOBCaster implements CastsAttributes
+{
+    public function get($model, $key, $value, $attributes)
+    {
+        if ($value === null) {
+            return null;
+        }
+
+        return Carbon::parse($value);
+    }
+
+    public function set($model, $key, $value, $attributes)
+    {
+        if ($value instanceof Carbon) {
+            return [$key => $value->toDateString()];
+        }
+
+        if ($value === null) {
+            return [$key => null];
+       
+
+        return [$key => (string)$value];
     }
 }

Going to draft this in case pending we work around demonstrated edge case above. 👆

It would be a breaking change to how casts should access attributes if we merge as-is, but perhaps worth it for the performance benefit. Any other thoughts?

@taylorotwell macropay-solutions/maravel-framework#21

@marius-ciclistu thanks for the full test case. I have now added it to the PR along with the required changes to make it pass.

Calling mergeAttributesFromAttributeCasts when a mutator get/set is being called so any calls to $this->attributes within the mutator returns the latest data

@taylorotwell FYI

sidenote: [static analysis / Types (pull_request)] is failing, but that seems to be the case for all PRs at the moment, so I guess it is unrelated to my PR and will soon be fixed anyways.

@ug-christoph so the test revealed more places where the case was not handled?

Is this new version faster than the target branch?

@marius-ciclistu

I ran some benchmarks to compare the 3 variants:

1. without PR (=merge all casts on any property being accessed)

2. my original PR (=only merge casts for accessed property)

3. the current PR state (=only merge casts for accessed property on regular prop access, but merge all casts on mutator calls)

I ran this code which uses models with large json documents in the data column that have a classcast:

        $prospect = Prospect::query()->where(Prospect::DATA_BYTES, '>', 20000)->first();
        $data = $prospect->data;

        $start = microtime(true);
        for($i = 0; $i < 10000; $i++) {
            $id = $prospect->id;
        }
        $regularPropertyDuration = microtime(true) - $start;

        $start = microtime(true);
        for($i = 0; $i < 10000; $i++) {
            $test = $prospect->mutator_test;
        }
        $mutatorDuration = microtime(true) - $start;

following numbers are averages across multiple runs:

1. without PR:
   regularPropertyDuration: 35.07749581337
   mutatorDuration: 34.717057943344

2. original PR:
   regularPropertyDuration: 0.62853203773499
   mutatorDuration: 0.5781160068512

3. PR with mutator fix:
   regularPropertyDuration: 0.59091091156006
   mutatorDuration: 35.610692977905

So basically including the fix gets the performance of mutator access back to how it is currently in master. This makes sense since it just makes sure that the behavior of mutators dont change and the performance improvement changes only affect regular property access.

TLDR: yes. the regular property access is still significantly faster than in the target branch. Mutator access performance remains unchanged compared to the target. Since mutators are normally much rarer than regular properties the overall performance improvement from this PR will remain as initially proposed.

@ug-christoph are you curious to test also my poc? https://github.com/macropay-solutions/maravel-framework/pull/21.patch

@marius-ciclistu for your PR I get very similar durations:

regularPropertyDuration: 0.72791814804077
mutatorDuration: 35.626289129257