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

[UID] Added the component + Added support for UUID#35940

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

Merged
fabpot merged 1 commit intosymfony:masterfromlyrixx:string-uuid
Mar 12, 2020

Conversation

lyrixx
Copy link
Member

@lyrixxlyrixx commentedMar 3, 2020•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PR
<?phprequire__DIR__.'/vendor/autoload.php';useSymfony\Component\Uid\Uuid;$u = Uuid::v1();dump($u);dump($u->isNull());dump($u->getType());dump($u->getVariant());dump($u->getTime());dump($u->getMac());dump($u->isEqualsTo($u));dump($u->compare($u));dump(Uuid::fromBinary($u->toBinary()));

dunglas, ro0NL, lyrixx, JoseCage, javiereguiluz, jderusse, PeterFour, odan, and andreybolonin reacted with thumbs up emojibrzuchal and Ma27 reacted with thumbs down emojitheofidry, gmponos, Koc, kunicmarko20, mnapoli, nunomaduro, Ocramius, simPod, maks-rafalko, brzuchal, and 3 more reacted with confused emojijeremyFreeAgent, nicolas-grekas, lyrixx, imiroslavov, linaori, yceruto, hugohenrique, ogizanagi, JoseCage, javiereguiluz, and 3 more reacted with heart emoji
@stof

This comment has been minimized.

@lyrixx

This comment has been minimized.

@lyrixx

This comment has been minimized.

@lyrixxlyrixxforce-pushed thestring-uuid branch 2 times, most recently from546a7b9 to232c30bCompareMarch 3, 2020 15:55
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice thank you, very simple!

Why in the string component ?

Because that's where it fits bests to me: theString component gathers simple "string-like" value objects; UUID is an important one of them.

sstok reacted with laugh emojidunglas reacted with confused emoji
@wouterj
Copy link
Member

Given the number of UUID libraries in PHP/Symfony land (refhttps://jolicode.com/blog/uuid-generation-in-php ), I would say this topic is oversaturated with libraries already. Is there a major pro for this new class instead of the Pecl extension/Symfony Polyfill or Ramsey UUID?

localheinz, pbowyer, GaetanRole, theofidry, gmponos, faizanakram99, Koc, samnela, er1z, brzuchal, and 2 more reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 3, 2020•
edited
Loading

Where do you see saturation here? pecl is low level: there need to be a value object in our OOP code. polyfill is ... a polyfill, and FFI+libuuid is a POC. Which means there is only one single available (and mainstream) implementation: ramsey/uuid.
The class proposed here provides the same benefits (a VO), with a very simple implementation.
Ppl that need to do fancy things with uuid - or that need guaranteed portability to 32b systems, will have to resort to ramsey/uuid. But the rest of the world will be happy to leverage a trivial implem.
At least I would.

lyrixx, OskarStark, bastnic, romainnorberg, jeremyFreeAgent, Pierstoval, B-Galati, derrabus, Azhovan, duplabe, and 2 more reacted with thumbs up emoji

@wouterj

This comment has been minimized.

@nicolas-grekas
Copy link
Member

this UUID class doesn't add any value on top of the PECL extension/polyfill

It does provide a lot: e.g a VO integrates with Doctrine types. The pecl extension doesn't.

About the doc for deps, there's nothing specific to this PR... The argument would apply to any new feature with optional deps...

@stof

This comment has been minimized.

@fabpot
Copy link
Member

Agreed on the new component proposal.

lyrixx, Pierstoval, nicolas-grekas, ogizanagi, and andreybolonin reacted with hooray emojibrzuchal reacted with confused emoji

@ro0NL
Copy link
Contributor

did we consider adding polyfill support for ramsey/uuid? esp. since it has 1st class pecl support already:

https://github.com/ramsey/uuid/blob/b31703e7c9752260ddc773c95110294fa9abc5ff/src/FeatureSet.php#L84
https://github.com/ramsey/uuid/blob/master/src/Generator/PeclUuidNameGenerator.php

Ppl that need to do fancy things with uuid - or that need guaranteed portability to 32b systems, will have to resort to ramsey/uuid

so, we shouldnt resort to ramsey/uuid for simple/non-fancy uuids? i feel like we're defeating it's UuidInterface + the fact most of the ecosystem is using it already.

It does provide a lot: e.g a VO integrates with Doctrine types.

so does ramsey/uuid.

im rather skeptical aboutvendor-a/uuid +vendor-b/uuid :)

theofidry, wouterj, faizanakram99, samnela, ahmed1886, and sstok reacted with thumbs up emoji

@lyrixxlyrixx changed the title[String] Added support for UUID[UUID] Added the componentMar 5, 2020
Copy link
Contributor

@localheinzlocalheinz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

👍

Looks good to me code-wise.

However, I agree with@theofidry.

I believe the general question the core team need to discuss is whether Symfony wants to follow the same route Zend Framework did about a decade ago (pulling in a component for every single use case), or whether that is really necessary.

Pulling in a component for every single use case has the following down-sides:

  • more work required by maintainers
  • more maintainers required
  • more documentation required
  • more bug fixes required
  • more signal and noise on the centralsymfony/symfony repository
  • killing successful 3rd-party libraries

Not sure ifsymfony/symfony really needs to become that big black 🕳, sucking in everything around it.

pbowyer, kunicmarko20, arnolem, kocsismate, goetas, CapitaineJSparrow, shochdoerfer, and yivi reacted with thumbs up emoji
Copy link
Member

@dunglasdunglas left a comment

Choose a reason for hiding this comment

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

(I've no strong opinion regarding if we need a new component or if we can just rely onramsey/uuid, but 👍 regarding the code).

@nicolas-grekas

This comment has been minimized.

@OskarStark

This comment has been minimized.

@Taluu

This comment has been minimized.

@nicolas-grekas

This comment has been minimized.

@lyrixxlyrixx changed the title[UUID] Added the component[Uid] Added the component + Added support for UUIDMar 11, 2020
@lyrixxlyrixx changed the title[Uid] Added the component + Added support for UUID[UID] Added the component + Added support for UUIDMar 11, 2020
@wouterj
Copy link
Member

Just a random thought: Would the idea of the Uid component be stronger if we add aUidInterface contract, that can be used by any unique identifier introduced?

@lyrixx
Copy link
MemberAuthor

@wouterj What would be the interface ?

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

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

2 minors

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Still good to me after the recent changes. Good to go on my side.

@fabpot
Copy link
Member

Thank you@lyrixx.

latrach reacted with thumbs up emojilyrixx reacted with heart emoji

@fabpotfabpot merged commitd108f7b intosymfony:masterMar 12, 2020
@lyrixxlyrixx deleted the string-uuid branchMarch 13, 2020 16:03
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@TaluuTaluuTaluu approved these changes

@stofstofstof left review comments

@localheinzlocalheinzlocalheinz left review comments

@OskarStarkOskarStarkOskarStark approved these changes

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@ogizanagiogizanagiogizanagi approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

14 participants
@lyrixx@stof@wouterj@nicolas-grekas@fabpot@ro0NL@theofidry@OskarStark@Taluu@dunglas@javiereguiluz@localheinz@ogizanagi@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp