[Serializer] Improve denormalization of backed enums #50201
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| 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); |
There was a problem hiding this comment.
| $givenType = get_debug_type($data); | |
| $givenType = \get_type($data); |
There was a problem hiding this comment.
if you mean gettype function, it returns "integer" and ReflectionNamedType::getName returns "int', so get_debug_type fits better
| } | ||
|
|
||
| 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); |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/Symfony/Component/Serializer/Normalizer/BackedEnumNormalizer.php
Outdated
Show resolved
Hide resolved
9804c36 to
90ffb90
Compare
| * 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 |
There was a problem hiding this comment.
| * 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 |
The goal is to improve error handling when denormalizing an enum.
Currently, if a string value is given when an integer is expected for the BackedEnum then such an error is thrown:
TypeError: TestEnum::from(): Argument #1 ($value) must be of type int, string givenThis PR adds validation for such cases.
Also in case of an invalid enum case, the NotNormalizableValueException is thrown.