Ensure consistent original data with enums #10088

HypeMC · 2022-10-01T18:01:29Z

As mentioned in #10074, the SimpleObjectHydrator doesn't convert strings into enums, while the ObjectHydrator does, causing inconsistent values in the UnitOfWork::$originalEntityData array. This PR aims to fix that.

michnovka · 2022-10-10T10:00:36Z

I think this is the last piece of the puzzle to have consistent treating of enums inside the internals. The changes are compatible with the last PR https://github.com/doctrine/orm/pull/10058/files merged into 2.13.3, Id like to merge this into the next 2.13 point release together with #10074 @derrabus

derrabus · 2022-10-10T10:03:41Z

Can you please rebase on 2.13.x? I'd like to see that this PR does not break with the changes of #10074.

michnovka · 2022-10-10T10:32:56Z

@derrabus #10119 check this, all checks pass, so you can merge this one and close my PR. Thanks

HypeMC · 2022-10-10T10:34:49Z

@derrabus Done

EDIT:
Oh looks like @michnovka beat me to it 😄

derrabus · 2022-10-10T11:25:48Z

tests/Doctrine/Tests/ORM/Functional/EnumTest.php

@@ -347,15 +349,10 @@ public function testEnumWithNonMatchingDatabaseValueThrowsException(string $card
            [$metadata->fieldMappings['id']['columnName'] => $card->id]
        );

-        $this->expectException(MappingException::class);


Why do we need to change the expectation here? This could qualify as a breaking change if calling code expects a MappingException in this scenario.

@derrabus Before the change the string was converted to an enum in the ReflectionEnumProperty class

orm/lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php

Lines 88 to 107 in 1ed0057

private function initializeEnumValue($object, $value): BackedEnum

{

if ($value instanceof BackedEnum) {

return $value;

}

$enumType = $this->enumType;

try {

return $enumType::from($value);

} catch (ValueError $e) {

throw MappingException::invalidEnumValue(

get_class($object),

$this->originalReflectionProperty->getName(),

(string) $value,

$enumType,

$e

);

}

}

Now it's converted earlier in the AbstractHydrator class instead

orm/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

Lines 700 to 709 in 1ed0057

private function buildEnum($value, string $enumType)

{

if (is_array($value)) {

return array_map(static function ($value) use ($enumType): BackedEnum {

return $enumType::from($value);

}, $value);

}

return $enumType::from($value);

}

Since the AbstractHydrator class throws a different exception the test fails. I'm not sure if this inconsistency in exceptions is a bug itself.

@HypeMC we should wrap with the same try-catch block and throw MappingException I think

@HypeMC can you please incorporate this change?

Yes, I've already started, unfortunately it's not as trivial as I thought. The current exception message contains the class name, which is currently not always available. I'll try to finish the PR in the following days.

replace

orm/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php

Lines 143 to 145 in b41f5ec

if ($value !== null && isset($cacheKeyInfo['enumType'])) {

$value = $this->buildEnum($value, $cacheKeyInfo['enumType']);

}

with

if ($value !== null && isset($cacheKeyInfo['enumType'])) { try { $value = $this->buildEnum($value, $cacheKeyInfo['enumType']); } catch (ValueError $e) { throw MappingException::invalidEnumValue( $entityName, $cacheKeyInfo['fieldName'], (string) $value, $cacheKeyInfo['enumType'], $e ); } }

One issue though - the AbstractHydrator::hydrateRowData() throws only HydrationException, so throwing MappingException from here will result in psalm errors. @derrabus what do you suggest, should we update the @throws PHPDoc or should we wrap the Mapping exception into HydrationException (this by the same logic could be considered BC break)

@michnovka Good point, I wanted to change the buildEnum method to throw a MappingException, but that would actually also be a BC break, so this makes the most sense.

stephanvierkant · 2022-10-18T14:32:06Z

What's the status of this PR? It's not clear to me what should be done to get this merged.

michnovka

I think its OK now

michnovka · 2022-11-02T15:59:00Z

@derrabus can we add this to the 2.13.4 milestone? Thanks

derrabus · 2022-11-02T16:01:57Z

Can we write a functional test that reproduces the problem we're attempting to fix here?

michnovka · 2022-11-02T16:37:00Z

@derrabus Well this really does not fix any actual functional bug, its just inconsistency in how originalEntityData is populated. Some hydrators store enums, some store strings (enum values).

See original post - #10074 (comment)

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.

With this fix, all hydrators store originalEntityData enum fields as native enums.

michnovka · 2022-11-02T16:46:16Z

@derrabus have a look here - #10198

derrabus · 2022-11-02T17:03:41Z

So it's really just about the internal state of the persisters? Why's that so important to you that you want to push it into a bugfix release?

michnovka · 2022-11-02T17:09:49Z

well we introduced this inconsistency with other enum-related PRs that were part of 2.13.x ( #10041 and #10058 ) and this issue did have functional impact prior to #10074 . So I wanted to close this last piece of related issues. TBH I did think it does have functional implication as I didnt debug myself the actual effects of #10074 on this issue. Its clear now it doesnt, but still worth a merge into 2.13.x for the above mentioned reasons

NoiseByNorthwest · 2022-11-02T17:27:44Z

It has functionnal implications, our application does not work with 2.13.2 & 2.13.3 due do enum related bugs. For instance this issue will cause false positive changeset (same logical enum value but different type).

derrabus · 2022-11-02T17:31:58Z

@NoiseByNorthwest Would you be able to provide a functional test that reproduces the issue your application has?

michnovka · 2022-11-02T17:58:06Z

@NoiseByNorthwest changeset computations are fixed by #10074 which is planned for 2.13.4 release

NoiseByNorthwest · 2022-11-02T21:51:20Z

@NoiseByNorthwest Would you be able to provide a functional test that reproduces the issue your application has?

I have a FooEntity type with a (many-to-one) relationship to a BarEntity type. The BarEntity type has an enum property.

When I create a FooEntity from an existing BarEntity, the resolved changeset will consider the BarEntity as updated since its enum property will be wrongly considered as changed.

That causes, among other annoyances, the BarEntity::updatedAt property to be updated.

greg0ire · 2022-11-21T19:39:11Z

People are starting to ask me an ETA for the next release. Should I release 2.13.4 now, or should we wait for somebody to transform @NoiseByNorthwest 's summary above into a test?

HypeMC · 2022-11-22T07:31:07Z

People are starting to ask me an ETA for the next release. Should I release 2.13.4 now, or should we wait for somebody to transform @NoiseByNorthwest 's summary above into a test?

@greg0ire I can take a look at it today. I'll let you know how it goes.

Warxcell · 2022-11-23T06:46:34Z

I have (eventually) a test-case for you #10245 :)

michnovka · 2022-11-23T16:03:07Z

@greg0ire I think it has been confirmed now in #10245 that there is no more error. You can make the release, and ideally merge this PR into 2.13.4 too

greg0ire

The commit message consists in a title.

This is fine for trivial changes, but here, to understand this change, one has to have prior knowledge of whatever was discussed in #10074, which itself mentions #10071, which is unclear to me… #10073 is a bit more clear though.

Long story short, I don't want to have to read 3 issues and 4 PRs before deciding whether to merge this so if you want this merged, you'll have to:

create a crystal clear commit message explaining what is broken, and why your changes fix the current situation
write a test that shows the current bug using high level APIs, not only plumbing APIs

I will release without this for now.

greg0ire · 2022-11-23T19:50:02Z

tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php

+        $hydrator = new SimpleObjectHydrator($this->entityManager);
+        $result   = $hydrator->hydrateAll($stmt, $rsm)[0];
+
+        self::assertEquals(Suit::Clubs, $result->suit);


This assertion passes without your changes.

greg0ire · 2022-11-23T19:51:23Z

tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php

+        $result   = $hydrator->hydrateAll($stmt, $rsm)[0];
+
+        self::assertEquals(Suit::Clubs, $result->suit);
+        self::assertEquals(Suit::Clubs, $this->entityManager->getUnitOfWork()->getOriginalEntityData($result)['suit']);


This one doesn't, but it uses the API of UnitOfWork, which users are not really suppose to deal with. This makes it hard for me to understand where you're driving at with this test.

greg0ire · 2022-11-23T19:51:44Z

tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php

+    /**
+     * @requires PHP 8.1
+     */
+    public function testEnumsAreBuilt(): void


I don't understand what this method name means.

greg0ire · 2022-11-23T19:53:50Z

tests/Doctrine/Tests/ORM/Persisters/BasicEntityPersisterResultMappingTest.php

+    {
+        $statement = $this->persister->getSelectSQL([]);
+        self::assertEquals('SELECT t0.id AS id_1, t0.suit AS suit_2 FROM Card t0', $statement);
+        self::assertEquals(['suit_2' => Suit::class], $this->persister->getResultSetMapping()->enumMappings);


Same here, who uses that API directly? I don't understand what you are fixing here…

greg0ire · 2022-11-23T20:29:05Z

Ok so remove the tests, and target 2.14.x, since this is more likely to break stuff than to fix it.

greg0ire · 2022-11-23T20:29:46Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

@@ -697,7 +697,7 @@ protected function registerManaged(ClassMetadata $class, $entity, array $data)
     *
     * @return BackedEnum|array<BackedEnum>
     */
-    private function buildEnum($value, string $enumType)
+    protected function buildEnum($value, string $enumType)


If we are going to expose this to other classes, let's at least make it final

Suggested change

protected function buildEnum($value, string $enumType)

final protected function buildEnum($value, string $enumType)

greg0ire · 2022-11-23T20:31:12Z

is best explained by the above linked comment on this PR

Please reuse whatever you see fit as a commit message body. I will read that.

michnovka · 2022-11-23T20:36:51Z

Ensure consistent UnitOfWork::$originalEntityData regardless of hydrator type for enum fields.

michnovka · 2022-11-23T20:39:25Z

I would really get rid of the tests @greg0ire , you are right it makes no sense since only "plumbing" APIs are affected

greg0ire · 2022-11-23T20:40:12Z

If there was a test to be added, it would be in AbstractHydratorTest => tests for things that should apply to all hydrators. It makes no sense not to have tests for high level API if we know what we are fixing, but here, we don't.

michnovka · 2022-12-06T23:03:57Z

whats the decision here, do we remove the tests?

greg0ire · 2022-12-06T23:10:22Z

I'd say either remove them or promote them to AbstractHydratorTest if doable. What the current tests are doing is only ensuring a behavior that happens to be the same as other hydrators now.

michnovka · 2022-12-06T23:11:47Z

@HypeMC can you please do either of what @greg0ire suggested? Thanks

HypeMC · 2022-12-06T23:17:50Z

I'll take a look to see what can be done.

Previously different hydrators would store the original data for enum fields in different ways, the SimpleObjectHydrator would keep them as strings while other hydrators would convert then to native php enums. This would make the data in the internal UnitOfWork::$originalEntityData array inconsistent which could've caused problems in the long run. Now, all hydrators convert enum fields to native php enums ensuring the original data is always consistent regardless of the hydrator used.

HypeMC · 2022-12-07T04:02:23Z

@greg0ire I've removed the tests as I didn't see a way to promote them to the AbstractHydratorTest. The changes I made are in the SimpleObjectHydrator & the AbstractHydratorTest only mocks the AbstractHydrator.

I've also switched the target branch to 2.14.x & updated the commit message.

SenseException · 2022-12-08T22:59:56Z

@greg0ire @HypeMC I'm confused about the removal of the tests and why there are no test cases for the newly added code. If someone removes the newly added code at some time in the future there would be no test to catch that and reintroduce this inconsistency again.

greg0ire · 2022-12-09T07:03:31Z

If we keep the test and somebody changes other hydrators, we would also reintroduce an inconsistency, that's why I recommended we either promote this test as an abstract test or remove it. It gives a false sense of security on top of being hard to understand.

michnovka · 2022-12-20T10:54:54Z

This was not merged into 2.14.0. However I really dont think this is a new feature, as it is only changing internal states. Do we need to retarget 2.15.x or can we merge into 2.14.1?

greg0ire · 2022-12-20T15:50:20Z

Seems fair, let's merge as is.

HypeMC mentioned this pull request Oct 1, 2022

Fix change set computation with enums #10074

Merged

derrabus added the Bug label Oct 10, 2022

michnovka mentioned this pull request Oct 10, 2022

PR 10088 rebased for tests #10119

Closed

HypeMC force-pushed the enums-in-simpleobjecthydrator branch from 8022b52 to b41f5ec Compare October 10, 2022 10:34

derrabus reviewed Oct 10, 2022

View reviewed changes

HypeMC force-pushed the enums-in-simpleobjecthydrator branch from b41f5ec to 3480c5d Compare October 11, 2022 21:11

W0rma mentioned this pull request Oct 13, 2022

Unit of Work calculates unnecessary UPDATE queries for enum properties in cascaded collections #10125

Closed

michnovka previously approved these changes Oct 18, 2022

View reviewed changes

stephanvierkant previously approved these changes Oct 25, 2022

View reviewed changes

derrabus added this to the 2.13.4 milestone Nov 2, 2022

greg0ire reviewed Nov 23, 2022

View reviewed changes

HypeMC dismissed stale reviews from stephanvierkant and michnovka via fb85326 December 7, 2022 03:33

HypeMC force-pushed the enums-in-simpleobjecthydrator branch from 3480c5d to fb85326 Compare December 7, 2022 03:33

HypeMC changed the base branch from 2.13.x to 2.14.x December 7, 2022 03:35

HypeMC force-pushed the enums-in-simpleobjecthydrator branch from fb85326 to 9d5ab4c Compare December 7, 2022 03:55

greg0ire approved these changes Dec 7, 2022

View reviewed changes

greg0ire requested review from derrabus and SenseException December 7, 2022 06:49

michnovka approved these changes Dec 7, 2022

View reviewed changes

greg0ire merged commit f8bf84d into doctrine:2.14.x Dec 20, 2022

greg0ire added this to the 2.14.1 milestone Dec 20, 2022

HypeMC deleted the enums-in-simpleobjecthydrator branch December 20, 2022 16:08

github-actions bot mentioned this pull request Jan 16, 2023

Merge release 2.14.1 into 2.15.x #10413

Merged

X-Coder264 mentioned this pull request Feb 3, 2023

Fix audit revision query for enum column mapping sonata-project/EntityAuditBundle#534

Merged

	private function initializeEnumValue($object, $value): BackedEnum
	{
	if ($value instanceof BackedEnum) {
	return $value;
	}

	$enumType = $this->enumType;

	try {
	return $enumType::from($value);
	} catch (ValueError $e) {
	throw MappingException::invalidEnumValue(
	get_class($object),
	$this->originalReflectionProperty->getName(),
	(string) $value,
	$enumType,
	$e
	);
	}
	}

	private function buildEnum($value, string $enumType)
	{
	if (is_array($value)) {
	return array_map(static function ($value) use ($enumType): BackedEnum {
	return $enumType::from($value);
	}, $value);
	}

	return $enumType::from($value);
	}

	if ($value !== null && isset($cacheKeyInfo['enumType'])) {
	$value = $this->buildEnum($value, $cacheKeyInfo['enumType']);
	}

	protected function buildEnum($value, string $enumType)
	final protected function buildEnum($value, string $enumType)

Ensure consistent original data with enums #10088

Ensure consistent original data with enums #10088

Conversation

HypeMC commented Oct 1, 2022

michnovka commented Oct 10, 2022

derrabus commented Oct 10, 2022

michnovka commented Oct 10, 2022

HypeMC commented Oct 10, 2022 • edited

Choose a reason for hiding this comment

HypeMC Oct 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanvierkant commented Oct 18, 2022

michnovka left a comment

Choose a reason for hiding this comment

michnovka commented Nov 2, 2022

derrabus commented Nov 2, 2022

michnovka commented Nov 2, 2022

michnovka commented Nov 2, 2022

derrabus commented Nov 2, 2022

michnovka commented Nov 2, 2022

NoiseByNorthwest commented Nov 2, 2022

derrabus commented Nov 2, 2022

michnovka commented Nov 2, 2022

NoiseByNorthwest commented Nov 2, 2022

greg0ire commented Nov 21, 2022

HypeMC commented Nov 22, 2022

Warxcell commented Nov 23, 2022 • edited

michnovka commented Nov 23, 2022 • edited

greg0ire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Nov 23, 2022

michnovka commented Nov 23, 2022

michnovka commented Nov 23, 2022

greg0ire commented Nov 23, 2022 • edited

michnovka commented Dec 6, 2022

greg0ire commented Dec 6, 2022

michnovka commented Dec 6, 2022

HypeMC commented Dec 6, 2022

HypeMC commented Dec 7, 2022

SenseException commented Dec 8, 2022

greg0ire commented Dec 9, 2022

michnovka commented Dec 20, 2022

greg0ire commented Dec 20, 2022

HypeMC commented Oct 10, 2022 •

edited

HypeMC Oct 10, 2022 •

edited

Warxcell commented Nov 23, 2022 •

edited

michnovka commented Nov 23, 2022 •

edited

greg0ire commented Nov 23, 2022 •

edited