Skip to content
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

[8.x] Fix getDirty method when using class castables (e.g. As(Encrypted)ArrayObject/As(Encrypted)Collection) #38774

Closed

Conversation

gdebrauwer
Copy link
Contributor

What is the issue?

In MySQL, a JSON column adds spaces between the JSON keys and the JSON values, even when you explicitly try to save it without spaces.

// If you try to save this in a MySQL json column:
{"foo":"bar"}
// Then MySQL will save it like this:
{"foo": "bar"}

This causes an issue when using the AsArrayObject casting class on a JSON attribute. Currently, the originalIsEquivalent method (used by getDirty method) does nothing special when an attribute uses a class castable. This means that it will do a strict string comparison between the original and the new value, which will result in a false because of the spaces in the original value. This causes a lot of unnecessary update queries because Eloquent thinks a model contains changes.

'{"foo": "bar"}' !== '{"foo":"bar"}'

How to fix it?

To fix this, we need to cast the original attribute using the class castable before comparing the two values. This casting is done in the originalIsEquivalent method itself. It does not use the castAttribute because that causes issues with the classCastCache (which caches only based key instead of key-value combination).

I also updated the get methods of the As(Encrypted)ArrayObject and As(Encrypted)Collection casts. The get method currently uses the value in the $attributes array, while it should use the provided $value.

@GrahamCampbell GrahamCampbell marked this pull request as draft September 12, 2021 12:55
@GrahamCampbell
Copy link
Member

Please un-mark this as draft, when ready for review.

@gdebrauwer gdebrauwer marked this pull request as ready for review September 12, 2021 12:56
@taylorotwell
Copy link
Member

A cast test is failing.

@taylorotwell taylorotwell marked this pull request as draft September 13, 2021 13:45
@gdebrauwer gdebrauwer marked this pull request as ready for review September 13, 2021 14:44
@driesvints
Copy link
Member

@gdebrauwer rebase again and the tests should pass now.

@driesvints driesvints marked this pull request as draft September 13, 2021 14:51
@gdebrauwer gdebrauwer force-pushed the is-dirty-bug-with-array-objects branch from 96e9272 to 899e3c1 Compare September 13, 2021 18:19
@gdebrauwer gdebrauwer marked this pull request as ready for review September 13, 2021 18:23
@taylorotwell
Copy link
Member

Honestly not sure I want to tinker with casting comparisons on a patch release.

I also don't think this is a full proof way for solving casting comparisons on custom casts. What if the get returns an object like Address that is made of multiple values from $attributes like street_line_1 and street_line_2, etc. Since you're passing in the "current" attributes the created object will not be an accurate reflection of the original attributes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants