-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: [Model] update does not work when using custom entity with toRawArray() #8271
Conversation
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.
I have a general issue with the use of $onlyChanged = true
. I know there were good reasons behind it, but the reality is it brings almost zero benefits and is a source of frustration. Hopefully, we can step away from using it someday. But this is just a side note.
Back to the problem... I believe this is okay.
method_exists($data, 'toRawArray') | ||
&& ( | ||
! empty($properties) | ||
&& ! empty($this->primaryKey) | ||
&& ! in_array($this->primaryKey, $properties, true) |
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.
Why are we removing this check?
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.
This does not make sense.
$this->primaryKey
is the column name like id
.
$properties
is an array with property_name => value
.
If id
is in the array values, the condition is false
.
I thought it was a mistake, and it should use array_key_exists()
, but
when the primary key value is binary data and it is cast to a string by Entity casting,
we need to set raw binary data even when there is a string primary key value in $properties
.
Without casting, if there is a primary key value in $properties
, we don't need to set it again,
but setting the same value is no harm.
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.
Oh.. yes, that makes sense. Thanks.
This bug and the code around it is only for If we were to create a custom Entity, implementing |
It seems the bug #7797 is not fixed, and something wrong in Model. |
I sent a new PR #8282 |
Description
See #8260 (comment)
Checklist: