-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Only merge cached casts for accessed attribute #57627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 12.x
Are you sure you want to change the base?
Changes from all commits
faf2ab8
8c2a6fb
4026ff4
3ad3900
391dc21
98fa005
a09a642
f9fed57
cca9ca9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -537,7 +537,9 @@ public function getAttributeValue($key) | |
| */ | ||
| protected function getAttributeFromArray($key) | ||
| { | ||
| return $this->getAttributes()[$key] ?? null; | ||
| $this->mergeAttributeFromCachedCasts($key); | ||
|
|
||
| return $this->attributes[$key] ?? null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1896,6 +1898,17 @@ protected function mergeAttributesFromCachedCasts() | |
| $this->mergeAttributesFromAttributeCasts(); | ||
| } | ||
|
|
||
| /** | ||
| * Merge the a cast class and attribute cast attribute back into the model. | ||
| * | ||
| * @return void | ||
| */ | ||
| protected function mergeAttributeFromCachedCasts(string $key) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the same as mergeAttributesFromCachedCasts, but it uses the new extracted methods mergeAttributeFromClassCasts and mergeAttributeFromAttributeCasts that allow to do it for just one attribute |
||
| { | ||
| $this->mergeAttributeFromClassCasts($key); | ||
| $this->mergeAttributeFromAttributeCasts($key); | ||
| } | ||
|
|
||
| /** | ||
| * Merge the cast class attributes back into the model. | ||
| * | ||
|
|
@@ -1904,15 +1917,26 @@ protected function mergeAttributesFromCachedCasts() | |
| protected function mergeAttributesFromClassCasts() | ||
| { | ||
| foreach ($this->classCastCache as $key => $value) { | ||
| $caster = $this->resolveCasterClass($key); | ||
| $this->mergeAttributeFromClassCasts($key); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extracted the code that was inside this loop to a separate method, so I can use it here and also independently for getAttribute |
||
| } | ||
| } | ||
|
|
||
| $this->attributes = array_merge( | ||
| $this->attributes, | ||
| $caster instanceof CastsInboundAttributes | ||
| ? [$key => $value] | ||
| : $this->normalizeCastClassResponse($key, $caster->set($this, $key, $value, $this->attributes)) | ||
| ); | ||
| private function mergeAttributeFromClassCasts(string $key): void | ||
| { | ||
| if (! isset($this->classCastCache[$key])) { | ||
| return; | ||
| } | ||
|
|
||
| $value = $this->classCastCache[$key]; | ||
|
|
||
| $caster = $this->resolveCasterClass($key); | ||
|
|
||
| $this->attributes = array_merge( | ||
| $this->attributes, | ||
| $caster instanceof CastsInboundAttributes | ||
| ? [$key => $value] | ||
| : $this->normalizeCastClassResponse($key, $caster->set($this, $key, $value, $this->attributes)) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1923,23 +1947,34 @@ protected function mergeAttributesFromClassCasts() | |
| protected function mergeAttributesFromAttributeCasts() | ||
| { | ||
| foreach ($this->attributeCastCache as $key => $value) { | ||
| $attribute = $this->{Str::camel($key)}(); | ||
| $this->mergeAttributeFromAttributeCasts($key); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extracted the code that was inside this loop to a separate method, so I can use it here and also independently for getAttribute |
||
| } | ||
| } | ||
|
|
||
| if ($attribute->get && ! $attribute->set) { | ||
| continue; | ||
| } | ||
| private function mergeAttributeFromAttributeCasts(string $key): void | ||
| { | ||
| if (! isset($this->attributeCastCache[$key])) { | ||
| return; | ||
| } | ||
|
|
||
| $callback = $attribute->set ?: function ($value) use ($key) { | ||
| $this->attributes[$key] = $value; | ||
| }; | ||
| $value = $this->attributeCastCache[$key]; | ||
|
|
||
| $this->attributes = array_merge( | ||
| $this->attributes, | ||
| $this->normalizeCastClassResponse( | ||
| $key, $callback($value, $this->attributes) | ||
| ) | ||
| ); | ||
| $attribute = $this->{Str::camel($key)}(); | ||
|
|
||
| if ($attribute->get && ! $attribute->set) { | ||
| return; | ||
| } | ||
|
|
||
| $callback = $attribute->set ?: function ($value) use ($key) { | ||
| $this->attributes[$key] = $value; | ||
| }; | ||
|
|
||
| $this->attributes = array_merge( | ||
| $this->attributes, | ||
| $this->normalizeCastClassResponse( | ||
| $key, $callback($value, $this->attributes) | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The affected tests are quite brittle (they do an assertions on the exact call count to a method). The PRs goal is to directly reduce those calls. The test is a good example of where the optimization helps. In these tests $subject->id is called which causes mergeAttributesFromCachedCasts to run for the encrypted/casted columns (secret_array, secret_collection). This is now expectedly not happening anymore and I have therefore just reduced the call count in the assertion by one to reflect this improvement |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of calling
return $this->getAttributes()[$key] ?? null;
I do exactly what getAttributes does, but instead of calling mergeAttributesFromCachedCasts
I call the new
mergeAttributeFromCachedCasts($key) so it only does it for the attribute I am currently trying to get