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

[Uid] add AbstractUid and interop with base-58/32/RFC4122 encodings #36074

Merged
merged 1 commit into from Mar 15, 2020

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 14, 2020

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

This PR provides a base AbstractUid class that is shared by Uuid and Ulid.

It adds new methods that provide interoperability between all types of UIDs but also between different encodings of UIDs:

  • toBase58() using the bitcoin alphabet - that's 22 chars aka "short-ids" - case sensitive
  • toBase32() using Crockford's alphabet - 26 chars ids - case insensitive
  • toRfc4122() to represent as a UUID-formatted string - 36 chars

This adds to toBinary() and to fromString(), the latter being able to cope with any of the 4 representations to create any kind of UIDs.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 14, 2020

I don't know the future plans of this component (will we keep adding other popular UIDs?) but this doesn't look future-proof to me. Any method that we add here could look outdated or unused in other future UID generators.

This reminds me to the salt property of password encoders. It was obvious to add it to an interface because every encoder uses it ... until some stopped using it and the interface no longer looked good for modern encoders.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:uuid-abstract branch from c6c1a90 to 6c79c9e Mar 14, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 14, 2020

All serious id schemes use 128-bit identifiers, which is wide enough to deal with any use cases for the foreseeable future. Then, base58, base32, uuid-format are 3 widely used representation of 128-bit numbers. They fit various needs: 58 bit for compactness, 32 bit for case insensitivity, uuid-format for interop with e.g. DB storages.

My opinion is that all three of them are generic enough to land in the component.

Your comparison with salted-encoders is good: we did add them to the component because that's what ppl were widely using at the time. We don't know the future but we know these encoding schemes are widely used and have good reasons to remain so.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:uuid-abstract branch from 6c79c9e to 29c12d4 Mar 14, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 14, 2020

will we keep adding other popular UIDs?

UUIDv6 would make sense. I'm not sure about others. I think we should work on integrating with Doctrine before considering other schemes. Then future will tell, there are no plans...

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:uuid-abstract branch from 29c12d4 to eb851a5 Mar 14, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 15, 2020

Can you rebase?

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:uuid-abstract branch from eb851a5 to ea3a194 Mar 15, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 15, 2020

Now rebased.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:uuid-abstract branch from ea3a194 to d8479ad Mar 15, 2020
@fabpot
fabpot approved these changes Mar 15, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 15, 2020

Thank you @nicolas-grekas.

@fabpot fabpot merged commit ade47dd into symfony:master Mar 15, 2020
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:uuid-abstract branch Mar 15, 2020
Tobion added a commit to Tobion/symfony that referenced this pull request Mar 15, 2020
nicolas-grekas added a commit that referenced this pull request Mar 15, 2020
This PR was merged into the 5.1-dev branch.

Discussion
----------

[UID] remove unused property

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |
| License       | MIT
| Doc PR        |

leftover from #36074

Commits
-------

901e62a remove unused uuid property
symfony-splitter pushed a commit to symfony/uid that referenced this pull request Mar 15, 2020
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

4 participants
You can’t perform that action at this time.