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

[Serializer] Fix denormalizing custom class in UidNormalizer #44600

Merged
merged 1 commit into from Dec 15, 2021

Conversation

@fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 13, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Considering we support denormalizing any subclass of AbstractUid::class we should not denormalize any $type !== Ulid::class to Uuid::class because this is wrong. We need to call $type::fromString (see TestCustomUid).
But fromString() is an abstract method, so it cannot be called directly on abstract classes, I guess we should have not supported them from the start. However, since they work right now (since all abstract classes fall back to the Uuid:class behavior) we cannot break it. So I propose to deprecate supporting abstract classes in 6.1 and remove it in 7.0.

@fancyweb fancyweb force-pushed the serializer/fix-custom-uid branch from 0634cf2 to ba9e002 Dec 13, 2021
} catch (\InvalidArgumentException $exception) {
throw new NotNormalizableValueException(sprintf('The data is not a valid "%s" string representation.', $type));
} catch (\Error $e) {
if (str_starts_with($e->getMessage(), 'Cannot instantiate abstract class')) {
Copy link
Contributor Author

@fancyweb fancyweb Dec 13, 2021

Choose a reason for hiding this comment

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

Catching the error and looking for the message looks better than doing reflection to me, maybe there's a better way?

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 15, 2021

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit fdc61b4 into symfony:5.3 Dec 15, 2021
8 of 12 checks passed
@fancyweb fancyweb deleted the serializer/fix-custom-uid branch Dec 15, 2021
nicolas-grekas added a commit that referenced this issue Dec 15, 2021
This PR was merged into the 5.3 branch.

Discussion
----------

[Serializer] Fix symfony/uid requirement

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | -
| Tickets       | -
| License       | MIT
| Doc PR        | -

#44600 needs 8247779

Commits
-------

6a0fd54 [Serializer] Fix symfony/uid requirement
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants