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 autowirable UidFactory #36097

Closed
wants to merge 1 commit into from

Conversation

@nicolas-grekas
Copy link
Member

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.

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 16, 2020
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:uuid-gen branch 5 times, most recently from 5755725 to c563c68 Mar 16, 2020
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 16, 2020
1 of 1 task complete
Copy link
Contributor

ro0NL left a comment

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

src/Symfony/Component/Uid/Ulid.php Show resolved Hide resolved
@ro0NL

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 nicolas-grekas:uuid-gen branch 3 times, most recently from 6b2ac89 to 24e043b Mar 20, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 20, 2020

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 nicolas-grekas force-pushed the nicolas-grekas:uuid-gen branch from 24e043b to 573aa13 Mar 20, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 20, 2020

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 nicolas-grekas:uuid-gen branch Mar 20, 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.