-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
5755725
to
c563c68
Compare
There was a problem hiding this 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
?
i also consider |
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! |
What I didn't mention in the description is that 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).
see just above :) |
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. |
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... |
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):
considering ramsey/uuid also has a static That's 2 constructors :) |
6b2ac89
to
24e043b
Compare
PR now updated: static factories and empty constructors are removed, generator helpers are marked internal. |
24e043b
to
573aa13
Compare
I'm giving up on this PR. Not worth the burden this will add on DX. |
…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
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.
This replaces the current static factories and empty constructors.