[Serializer] throw more specific exceptions #21239
Conversation
xabbuh
commented
Jan 11, 2017
•
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #20534 |
| License | MIT |
| Doc PR |
|
👍 |
| */ | ||
| public function denormalize($data, $class, $format = null, array $context = array()) | ||
| { | ||
| if (!preg_match('/^data:([a-z0-9][a-z0-9\!\#\$\&\-\^\_\+\.]{0,126}\/[a-z0-9][a-z0-9\!\#\$\&\-\^\_\+\.]{0,126}(;[a-z0-9\-]+\=[a-z0-9\-]+)?)?(;base64)?,[a-z0-9\!\$\&\\\'\,\(\)\*\+\,\;\=\-\.\_\~\:\@\/\?\%\s]*\s*$/i', $data)) { | ||
| throw new UnexpectedValueException('The provided "data:" URI is not valid.'); | ||
| throw new NotNormalizableValueException('The provided "data:" URI is not valid.'); |
There was a problem hiding this comment.
IMHO this example here should actually continue to throw the old exception. This is decodeable, but just not valid in the domain. I guess this example would be a 422 instead?
There was a problem hiding this comment.
But the same is true for the other places where we throw a NotNormalizableValueException in this PR, isn't it?
| @@ -79,7 +79,7 @@ public function denormalize($data, $class, $format = null, array $context = arra | |||
|
|
|||
| $dateTimeErrors = \DateTime::class === $class ? \DateTime::getLastErrors() : \DateTimeImmutable::getLastErrors(); | |||
|
|
|||
| throw new UnexpectedValueException(sprintf( | |||
| throw new NotNormalizableValueException(sprintf( | |||
There was a problem hiding this comment.
Maybe can we set a specific exception code depending of the cause to be able to easily detect if it's an invalid data: URI, or an invalid date and so on?
There was a problem hiding this comment.
I have thought about that too. The reason I did not add it is consistency. We would only be able to add codes for Symfony's own normalizer. However, it's quite common to add your own normalizers in libraries/projects. As a consequence exception codes would only be useful when they are issued by built-in normalizers. I don't really like that. What do you think?
There was a problem hiding this comment.
We can maybe reserve a range for app codes?
|
@xabbuh Any feedback on the comments? |
|
@xabbuh What about this one? |
|
I think I need @dunglas to answer my comments. :) |
|
PR rebased |
|
Thank you @xabbuh. |
This PR was merged into the 3.4 branch. Discussion ---------- [Serializer] throw more specific exceptions | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20534 | License | MIT | Doc PR | Commits ------- aa30d04 [Serializer] throw more specific exceptions
|
Thank you for solving this problem! |