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

Convert internal exception in UuidDenormalizer #4200

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Convert internal exception in UuidDenormalizer #4200

merged 1 commit into from
Apr 13, 2021

Conversation

guilliamxavier
Copy link
Contributor

Q A
Branch? 2.6
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #4189
License MIT
Doc PR

@guilliamxavier
Copy link
Contributor Author

For the exception, I hesitate between ApiPlatform\Core\Exception\DeserializationException and Symfony\Component\Serializer\Exception\NotNormalizableValueException 🤔

Also I guess this will need new test(s), but do you already agree on the principle?

@alanpoulain
Copy link
Member

I don't think NotNormalizableValueException would lead to a 400 status code, isn't it (but it probably should)?

@guilliamxavier
Copy link
Contributor Author

Yes it would, like all implementations of Symfony\Component\Serializer\Exception\ExceptionInterface. But I thought it may be better to use an ApiPlatform exception

@alanpoulain
Copy link
Member

Oh yes you are right. Maybe using NotNormalizableValueException would be better then to be like the denormalizers in Symfony?

@guilliamxavier
Copy link
Contributor Author

@alanpoulain: OK I will use the Symfony one after all, thanks.

@soyuka: Thanks but I thought that InvalidIdentifierException (already thrown in ApiPlatform\Core\Bridge\RamseyUuid\Identifier\Normalizer\UuidNormalizer) was for e.g. GET /foos/invalid-uuid, while this (in ApiPlatform\Core\Bridge\RamseyUuid\Serializer\UuidDenormalizer) is for e.g. POST /foos {"uuid":"invalid-uuid"} (similar to the difference between ApiPlatform specific DateTimeIdentifierDenormalizer and Symfony generic DateTimeNormalizer)?

@soyuka
Copy link
Member

soyuka commented Apr 12, 2021

My bad I thought this was related to identifier conversion not normalization, I'd be 👍 to transform this exception indeed. NotNormalizableValueException looks like the way to go here.

@guilliamxavier guilliamxavier marked this pull request as ready for review April 12, 2021 14:46
@alanpoulain alanpoulain merged commit 14bd387 into api-platform:2.6 Apr 13, 2021
@alanpoulain
Copy link
Member

Thank you @guilliamxavier.

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

Successfully merging this pull request may close these issues.

None yet

3 participants