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

fix: [Model] update does not work when using custom entity with toRawArray() #8271

Closed

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 29, 2023

Description
See #8260 (comment)

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 29, 2023
@kenjis kenjis mentioned this pull request Nov 29, 2023
5 tasks
Copy link
Member

@michalsn michalsn left a 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)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

@kenjis
Copy link
Member Author

kenjis commented Nov 29, 2023

This bug and the code around it is only for $onlyChanged = true.
It would not exist without $onlyChanged.
Performance would be better with large data and fewer fields to change, but the code is considerably more complex because of $onlyChanged.

If we were to create a custom Entity, implementing $onlyChanged would be quite tedious. It requires a process to determine the initial values and cannot be easily integrated with the current Model.

@kenjis kenjis marked this pull request as draft December 1, 2023 08:38
@kenjis
Copy link
Member Author

kenjis commented Dec 1, 2023

It seems the bug #7797 is not fixed, and something wrong in Model.
I'm checking now.

@kenjis
Copy link
Member Author

kenjis commented Dec 1, 2023

I sent a new PR #8282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants