Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FWB][Serializer][Form][Validator] Uid integration#36317

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
guillbdx wants to merge33 commits intosymfony:masterfromguillbdx:uid-integration

Conversation

@guillbdx
Copy link

@guillbdxguillbdx commentedApr 2, 2020
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?I think he UUID Validator should get deprecated? Please confirm this point.
TicketsFix#36102
LicenseMIT

Integration of the Uid Component to Symfony stack:

  • a normalizer/denormalizer for Serializer
  • a constraint validator
  • a DataTransformer and FormType
  • a command line to generate new UIDs

Copy link
Contributor

@maxheliasmaxhelias left a comment

Choose a reason for hiding this comment

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

I love the idea very much but I don't agree with the form integration. Normally, you should never have uid fields in a form. Otherwise a new doctrine type in the bridge would be really nice.

$io =newSymfonyStyle($input,$output);

if (!class_exists(AbstractUid::class)) {
thrownew \RuntimeException('Unable to execute this command as the Symfony Uid Component is not installed.');
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove the service in the FrameworkExtension file when theAbstractUid doesn't exist instead of throwing an exception

</service>

<serviceid="serializer.normalizer.uid"class="Symfony\Component\Serializer\Normalizer\UidNormalizer">
<argument>%kernel.debug%</argument>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove this argument

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@maxhelias
Copy link
Contributor

BTW, let me know when you have finished, i could test it

@ro0NL
Copy link
Contributor

ro0NL commentedApr 4, 2020
edited
Loading

i tend to prefer concrete UUID, ULID, ... validators. It allows for more fine grained control per type. E.g. ensure a specific UUID version maybe.

I'd also think we should either support sf/uid as well as ramsey/uuid, but not an issue for me.

Im not sure about the form type either, nor the console command.

@dunglas
Copy link
Member

👍 for the serializer part
for the validator part, I agree with@ro0NL, it must be possible to specify the type of UUID that is allowed

I've not reviewed other parts.

@guillbdx
Copy link
Author

guillbdx commentedApr 6, 2020
edited
Loading

Hi@ro0NL@dunglas , that is the case, thetypes option allow you to choose betweenULID,UUID_V1,UUID_V3, etc.

Another possibility would be to have 2 options :

  • type: either ULID or UUID
  • versions: UUID version (this option would have no effect if type is ULID)

Or another possibility: 2 different constraints, 1 ULID constraint and 1 UUID constraint, with aversions option on the UUID one.

What do you think is better?

@xabbuh
Copy link
Member

Maybe you could split the PR into smaller ones so that people can focus on the components they know best?

maxhelias reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

Or another possibility: 2 different constraints, [...]
What do you think is better?

i strongly prefer constraints per case.

@guillbdx
Copy link
Author

I've split this PR in 3 smaller PR, one per component:

I'm closing this PR.

@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dunglasdunglasAwaiting requested review from dunglas

@xabbuhxabbuhAwaiting requested review from xabbuh

1 more reviewer

@maxheliasmaxheliasmaxhelias left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

[Uid] integration with the other components

7 participants

@guillbdx@maxhelias@ro0NL@dunglas@xabbuh@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp