Skip to content

[Serializer] Improve denormalization of backed enums #50201

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from
Open

[Serializer] Improve denormalization of backed enums #50201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Symfony/Component/Serializer/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ CHANGELOG
* `JsonSerializableNormalizer`
* `ObjectNormalizer`
* `PropertyNormalizer`
* Make `BackedEnumNormalizer` throw `NotNormalizableValueException` when the backed type doesn't match
* Make `BackedEnumNormalizer` throw `NotNormalizableValueException` when the value does not belong to a backed enumeration
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Make `BackedEnumNormalizer` throw `NotNormalizableValueException` when the backed type doesn't match
* Make `BackedEnumNormalizer` throw `NotNormalizableValueException` when the value does not belong to a backed enumeration
* Make `BackedEnumNormalizer` throw `NotNormalizableValueException` when the backed type doesn't match or when the value does not belong to a backed enumeration


6.2
---
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Component/Serializer/Normalizer/BackedEnumNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Symfony\Component\Serializer\Normalizer;

use Symfony\Component\PropertyInfo\Type;
use Symfony\Component\Serializer\Exception\InvalidArgumentException;
use Symfony\Component\Serializer\Exception\NotNormalizableValueException;

Expand Down Expand Up @@ -69,14 +68,17 @@ public function denormalize(mixed $data, string $type, string $format = null, ar
}
}

if (!\is_int($data) && !\is_string($data)) {
throw NotNormalizableValueException::createForUnexpectedDataType('The data is neither an integer nor a string, you should pass an integer or a string that can be parsed as an enumeration case of type '.$type.'.', $data, [Type::BUILTIN_TYPE_INT, Type::BUILTIN_TYPE_STRING], $context['deserialization_path'] ?? null, true);
$backingType = (new \ReflectionEnum($type))->getBackingType()->getName();
$givenType = \get_debug_type($data);

if ($givenType !== $backingType) {
throw NotNormalizableValueException::createForUnexpectedDataType(sprintf('Data expected to be "%s", "%s" given. You should pass a value that can be parsed as an enumeration case of type %s.', $backingType, $givenType, $type), $data, [$backingType], $context['deserialization_path'] ?? null, true);
}

try {
return $type::from($data);
} catch (\ValueError $e) {
throw new InvalidArgumentException('The data must belong to a backed enumeration of type '.$type);
throw NotNormalizableValueException::createForUnexpectedDataType('The data must belong to a backed enumeration of type '.$type, $data, [$backingType], $context['deserialization_path'] ?? null, true);
Copy link
Member

Choose a reason for hiding this comment

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

this change basically reverts #47128
see #50192 also

Copy link
Author

Choose a reason for hiding this comment

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

#47128 was resolving #46977 issue which is covered by the \Symfony\Component\Serializer\Tests\SerializerTest::testCollectDenormalizationErrorsWithWrongEnum test and it works as expected. It doesn't throw TypeError but gives PartialDenormalizationException as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @nicolas-grekas here, it seems to revert the previous work.
If you take a look at #46977 (for instance #47128 (comment)), and #48821, you'll see that the InvalidArgumentException is intentional.

Copy link

Choose a reason for hiding this comment

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

Throwing InvalidArgumentException instead of NotNormalizableValueException undoes the feature done by #42502

I first viewed #43047 (comment)
Then I was happy that may be #40830 + #42502 would be a solution to my problem. But then #47128 undid that functionality again. It was partially reverted back to an NotNormalizableValueException in #50192

The whole point of having it as an NotNormalizableValueException is that you should be able to serialize DTOs with enums and then collect all the errors at the end in an PartialDenormalizationException. You should take care not to ruin that functionality.

}
}

Expand Down
9 changes: 8 additions & 1 deletion src/Symfony/Component/Serializer/Tests/Normalizer/BackedEnumNormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,16 @@ public function testDenormalizeObjectThrowsException()
$this->normalizer->denormalize(new \stdClass(), StringBackedEnumDummy::class);
}

public function testDenormalizeBadBackingTypeThrowsException()
{
$this->expectException(NotNormalizableValueException::class);
$this->expectExceptionMessage('Data expected to be "string", "int" given. You should pass a value that can be parsed as an enumeration case of type '.StringBackedEnumDummy::class.'.');
$this->normalizer->denormalize(1, StringBackedEnumDummy::class);
}

public function testDenormalizeBadBackingValueThrowsException()
{
$this->expectException(InvalidArgumentException::class);
$this->expectException(NotNormalizableValueException::class);
$this->expectExceptionMessage('The data must belong to a backed enumeration of type '.StringBackedEnumDummy::class);

$this->normalizer->denormalize('POST', StringBackedEnumDummy::class);
Expand Down
8 changes: 5 additions & 3 deletions src/Symfony/Component/Serializer/Tests/SerializerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
use Symfony\Component\Serializer\Tests\Fixtures\NormalizableTraversableDummy;
use Symfony\Component\Serializer\Tests\Fixtures\Php74Full;
use Symfony\Component\Serializer\Tests\Fixtures\Php80WithPromotedTypedConstructor;
use Symfony\Component\Serializer\Tests\Fixtures\StringBackedEnumDummy;
use Symfony\Component\Serializer\Tests\Fixtures\TraversableDummy;
use Symfony\Component\Serializer\Tests\Fixtures\TrueBuiltInDummy;
use Symfony\Component\Serializer\Tests\Fixtures\UpcomingDenormalizerInterface as DenormalizerInterface;
Expand Down Expand Up @@ -1208,7 +1209,7 @@ public function testCollectDenormalizationErrorsWithEnumConstructor()
$this->assertSame($expected, $exceptionsAsArray);
}

public function testNoCollectDenormalizationErrorsWithWrongEnum()
public function testCollectDenormalizationErrorsWithWrongEnum()
{
$serializer = new Serializer(
[
Expand All @@ -1223,8 +1224,9 @@ public function testNoCollectDenormalizationErrorsWithWrongEnum()
DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
]);
} catch (\Throwable $th) {
$this->assertNotInstanceOf(PartialDenormalizationException::class, $th);
$this->assertInstanceOf(InvalidArgumentException::class, $th);
$this->assertInstanceOf(PartialDenormalizationException::class, $th);
$this->assertArrayHasKey(0, $th->getErrors());
$this->assertSame("The data must belong to a backed enumeration of type ".StringBackedEnumDummy::class, $th->getErrors()[0]->getMessage());
}
}

Expand Down