Skip to content

[Serializer] add UidNormalizer #36406

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

Closed
wants to merge 6 commits into from

Conversation

guillbdx
Copy link

@guillbdx guillbdx commented Apr 9, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets #36102
License MIT

UUID and ULID normalizer.

@nicolas-grekas nicolas-grekas changed the title [Serializer][FWB] UidNormalizer. [Serializer] add UidNormalizer Apr 10, 2020
@guillbdx guillbdx force-pushed the serializer-uid-normalizer branch from 3e2abfb to 74235ab Compare May 5, 2020 19:15
@guillbdx guillbdx force-pushed the serializer-uid-normalizer branch from 5ff8932 to abe7069 Compare May 5, 2020 19:25
@@ -18,6 +18,7 @@ CHANGELOG
* Made `BrowserKitAssertionsTrait` report the original error message in case of a failure
* Added ability for `config:dump-reference` and `debug:config` to dump and debug kernel container extension configuration.
* Deprecated `session.attribute_bag` service and `session.flash_bag` service.
* Added `UidNormalizer` to the framework serializer.
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed

<service id="serializer.normalizer.uid" class="Symfony\Component\Serializer\Normalizer\UidNormalizer">
<!-- Run before serializer.normalizer.object -->
<tag name="serializer.normalizer" priority="-880" />
</service>
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to the new PHP config file.
For the priority, I would use -915 to be consistent with the other very specialized normalizers.

@@ -8,6 +8,7 @@ CHANGELOG
* added support for `\stdClass` to `ObjectNormalizer`
* added the ability to ignore properties using metadata (e.g. `@Symfony\Component\Serializer\Annotation\Ignore`)
* added an option to serialize constraint violations payloads (e.g. severity)
* added `UidNormalizer`
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to the 5.2.0 section.

*/
public function normalize($object, string $format = null, array $context = [])
{
if (!$object instanceof AbstractUid) {
Copy link
Member

Choose a reason for hiding this comment

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

The condition can be removed as the supports method garuantees that already.

/**
* {@inheritdoc}
*
* @throws InvalidArgumentException
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed

/**
* {@inheritdoc}
*
* @throws NotNormalizableValueException
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed

*/
public function denormalize($data, string $type, string $format = null, array $context = [])
{
if (!class_exists(AbstractUid::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed

try {
$uid = Ulid::class === $type ? Ulid::fromString($data) : Uuid::fromString($data);
} catch (\InvalidArgumentException $exception) {
throw new NotNormalizableValueException('The data is not a valid '.$type.' string representation.');
Copy link
Member

Choose a reason for hiding this comment

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

You should use sprintf here.

@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

@guillbdx Can you have a look at the feedback?

@fabpot
Copy link
Member

fabpot commented Sep 10, 2020

Anyone willing to take over this PR?

@fabpot
Copy link
Member

fabpot commented Sep 11, 2020

Closing in favor of #38151

@fabpot fabpot closed this Sep 11, 2020
fabpot added a commit that referenced this pull request Sep 11, 2020
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Serializer] add UidNormalizer

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #36102
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

UUID and ULID normalizer.

Continuation of #36406

Commits
-------

d6a8993 Update based on feedback
1c21c78 UidNormalizer.
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants