Skip to content

[Uid] add autowirable UidFactory #36097

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 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 16, 2020

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

While reviewing https://github.com/ulid/javascript/, I noticed that they define a way to change the source of random and the source of time, at least for testing purposes. UUIDv1 also has the same concept, as the RFC explains that the "node" part can be sourced from a MAC or from randomness.

This PR introduces the UidFactory factory class.
It's meant to be used as a service.
For this reason, this PR also defines a corresponding autowiring alias.

$factory = new UidFactory();

$ulid = $factory->ulid();
$v1 = $factory->uuidV1();
$v3 = $factory->uuidV3($v1, 'foo');
$v4 = $factory->uuidV4();
$v5 = $factory->uuidV5($v1, 'foo'));
$v6 = $factory->uuidV6();

This replaces the current static factories and empty constructors.

Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

is a service layer really better than Ulid::generate(...): Ulid?

@ro0NL
Copy link
Contributor

ro0NL commented Mar 16, 2020

i also consider UidFactory::createUuidV1 vs. Uuid::v1 a bit confusing. do i need to make this choice everytime?

@javiereguiluz
Copy link
Member

javiereguiluz commented Mar 17, 2020

Having (at least) three different ways to do the same thing seems excessive:

$v1 = (new UidFactory())->createUuidV1();
$v1 = Uuid::v1();
$v1 = new UuidV1();

This component was introduced as a lightweight alternative to the full-featured Ramsey library, so we may reconsider all this and simplify things if possible. Thanks!

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 17, 2020

What I didn't mention in the description is that UidFactory takes 3 constructor arguments, which allow configuring the time and entropy sources one may want to use in their app. Service configuration / DI FTW.

We could remove the static factories and the nullable constructors. That would ensure ppl always use the configurable way to create UIDs. On the other hand, this requires more code (autowiring will help a lot of course).

is a service layer really better than Ulid::generate(...): Ulid?

see just above :)

@ro0NL
Copy link
Contributor

ro0NL commented Mar 17, 2020

im wondering if SF should provide the service at all, in favor of invoking sources staticly where needed and/or let users design a service around it, if needed.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 17, 2020

So, you're OK to give up the possibility to configure the source? I feel like this is a core part of UUID generation - either for tests or for clusters, being able to control how they are generated...

@ro0NL
Copy link
Contributor

ro0NL commented Mar 17, 2020

i was hoping default sources fit 9/10 cases yes :) though ive no real experience with it, meaning i never had to alter such sources in practice.

For simplicity i tend to lean to (considering the UUID v4 case):

$notRandom =new UuidV4('0000...');
$defaultRandom = new UuidV4();
$maybeRandom = UuidV4::generate($randomSource);

UuidV4::setDefaultRandomSource($randomSource);
$defaultMaybeRandom = new UuidV4();

considering ramsey/uuid also has a static Uuid::setFactory.

That's 2 constructors :)

@nicolas-grekas nicolas-grekas force-pushed the uuid-gen branch 3 times, most recently from 6b2ac89 to 24e043b Compare March 20, 2020 19:30
@nicolas-grekas
Copy link
Member Author

PR now updated: static factories and empty constructors are removed, generator helpers are marked internal.

@nicolas-grekas nicolas-grekas changed the title [Uid] add autowirable UidFactory, providing configurable sources for time & entropy [Uid] add autowirable UidFactory Mar 20, 2020
@nicolas-grekas
Copy link
Member Author

I'm giving up on this PR. Not worth the burden this will add on DX.
Ppl that really want to change the time and random source can inject functions in the namespace.

@nicolas-grekas nicolas-grekas deleted the uuid-gen branch March 20, 2020 19:52
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
nicolas-grekas added a commit that referenced this pull request Feb 11, 2021
…stamps and randomness/nodes (fancyweb)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[Uid] Add UidFactory to create Ulid and Uuid from timestamps and randomness/nodes

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Ref #36097

When you migrate an existing resource identifier to an uid, you might want to choose the timestamp so that it is coherent with the creation date of the existing resource. (eg: I have a row in a table with id=1, created_at=2018-12-11 19:00:00, I would like to use that timestamp to create the resource Ulid).

I guess it can also be useful to choose the randomness of the Ulid or the node of the Uuid.

From what I understood, v3 and v5 don't need those features, this is why there are not in the factory.

See #39507 (review) for more details.

Commits
-------

88a99dd [Uid] Add UuidFactory to create Ulid and Uuid from timestamps, namespaces and nodes
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.

4 participants