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 change set computation with enums #10074

Merged
merged 2 commits into from Oct 10, 2022

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Sep 27, 2022

Fixes #10071 , uses test cases from #10073 by @mzk

My attempt at solving the issue. The ReflectionEnumProperty::getValue() method returns the value of the enum, while some hydrators pass an enum object in the data set to the UnitOfWork::$originalEntityData array.

The SimpleObjectHydrator doesn't convert strings into enums, while the ObjectHydrator (or to be more precise the AbstractHydrator::gatherRowData() method) does. Even though this doesn't seem to be causing any issues, the inconsistency seems unintentional to me, so if you think it should be fixed please let me know and I'll open a PR.

@michnovka
Copy link
Contributor

This is a good fix.

I think SimpleObjectHydrator should also convert strings into enums.

@HypeMC
Copy link
Contributor Author

HypeMC commented Oct 1, 2022

This is a good fix.

I think SimpleObjectHydrator should also convert strings into enums.

See #10088

@michnovka
Copy link
Contributor

@derrabus this is an important fix for enums, can we have it merged for the next 2.13 point release?

@derrabus derrabus added the Bug label Oct 10, 2022
@derrabus derrabus added this to the 2.13.4 milestone Oct 10, 2022
@derrabus derrabus merged commit 1ed0057 into doctrine:2.13.x Oct 10, 2022
35 checks passed
@derrabus
Copy link
Contributor

Thank you @HypeMC.

@HypeMC HypeMC deleted the change-set-computation-with-enums branch Oct 10, 2022
derrabus added a commit to derrabus/orm that referenced this pull request Oct 13, 2022
* 2.14.x:
  Allow doctrine/event-manager 2 (doctrine#10123)
  Psalm 4.29 (doctrine#10128)
  Address deprecation of Table::changeColumn() (doctrine#10121)
  Make code blocks consistent (doctrine#10103)
  Fix change set computation with enums (doctrine#10074)
  PHPStan 1.8.8, Psalm 4.28.0 (doctrine#10115)
  fix deprecated trigger help comment
@mroeling
Copy link

Is there an e.t.a. on a new release with this fix please?

@greg0ire
Copy link
Member

@mroeling we are waiting on #10088 to release this.

if ($orgValue instanceof BackedEnum) {
$orgValue = $orgValue->value;
}

// skip if value haven't changed
if ($orgValue === $actualValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is broken, and I'm pretty sure this is why. Based on the changes in line 747, we probably also need to have:

if ($actualValue instanceof BackedEnum) {
    $actualValue = $actualValue->value;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don think so. Check the code a bit above:

https://github.com/HypeMC/orm/blob/a1ed32f697f66d33de42fe96b1ea90e873ba2d5f/lib/Doctrine/ORM/UnitOfWork.php#L670

then

https://github.com/HypeMC/orm/blob/a1ed32f697f66d33de42fe96b1ea90e873ba2d5f/lib/Doctrine/ORM/UnitOfWork.php#L699

So actual value is fetched through reflection. And if you check the ReflectionEnumProperty::getValue()

https://github.com/HypeMC/orm/blob/a1ed32f697f66d33de42fe96b1ea90e873ba2d5f/lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php#L36-L63

It returns the $enum->value already. Therefore your proposed

if ($actualValue instanceof BackedEnum) {
    $actualValue = $actualValue->value;
}

seems redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong. All I know is that this patch broke several of our entities that use enums, and that seemed like a possibility. I haven't tested the theory.

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

Successfully merging this pull request may close these issues.

Fetched Entity with native Enum field with non empty changeSet (Enum <=> string)
7 participants