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

Model changes under consideration for version 4.x #256

Closed
ramsey opened this issue Jan 7, 2019 · 11 comments
Closed

Model changes under consideration for version 4.x #256

ramsey opened this issue Jan 7, 2019 · 11 comments

Comments

@ramsey
Copy link
Owner

ramsey commented Jan 7, 2019

Along the lines of #255 and as discussed in #241, I've been thinking through some of the changes I'd like to make to the ramsey/uuid model. I'd like to introduce better abstractions that allow more flexibility to introduce things like the non-standard version 6 UUID discussed in #228 or the helper method for COMB UUIDs (also non-standard) discussed in #205.

The following diagram is high-level and represents my current line of thinking around this. I'm providing it here to open discussion, so don't think of this as a final design.

I'd like to try to make sure UuidInterface and Uuid maintain as much BC with the current API as possible, to ease the upgrade path from 3.x to 4.x, but I'd also like to get feedback from others regarding this. How painful would that break be? This package is depended upon by many other projects in the Packagist ecosystem, so what happens to dependencies when some projects upgrade to 4.x, while others stay on 3.x? (I ran into a similar problem between 2.x and 3.x, but the number of dependent projects was much lower than it is now.)

So, this is open for discussion. I'd like to hear your thoughts.

ramsey-uuid-4 x

@ramsey ramsey added this to the Version 4.0.0 milestone Jan 7, 2019
@ramsey ramsey self-assigned this Jan 7, 2019
@Majkl578
Copy link

Majkl578 commented Jan 7, 2019

I was about to propose something similar 👍.

Basically each UUID type should be a separate class with their own specific methods, not like now - one god Uuid object with methods for all types (and throwing exception for incompatible calls).

what happens to dependencies when some projects upgrade to 4.x, while others stay on 3.x?

They will be mutually blocked. But that shouldn't be your primary concern - you would never release next major with this in mind.

@ramsey
Copy link
Owner Author

ramsey commented Jan 7, 2019

you would never release next major with this in mind

Can you elaborate on what you mean with this statement?

@ramsey ramsey pinned this issue Jan 7, 2019
@ramsey
Copy link
Owner Author

ramsey commented Jan 7, 2019

This is also related to #235

@Slamdunk
Copy link

Slamdunk commented Jan 8, 2019

I'd like to try to make sure UuidInterface and Uuid maintain as much BC with the current API as possible, to ease the upgrade path from 3.x to 4.x, but I'd also like to get feedback from others regarding this. How painful would that break be?

I think trying to retain the old API with new implementations would only add burdens and hard work with little gain.

Maybe a new API with a proxy implementation for old API that triggers deprecation errors pointing to a documentation on the correct upgrade path?

ramsey-uuid-4 x

What exactly are AbstractUuid and AbstractRfc4122Uuid? I already read Interfaces in this graph, so there's no need for Abstract classes while designing a project.

I would even like to suggest to never ever use any abstract class in a project like this where only ValueObjects are involved.

@ramsey
Copy link
Owner Author

ramsey commented Jan 8, 2019

@Slamdunk Good points about the abstract classes represented in the model diagram. I included them because I was considering that a lot of the functionality is shared and others may want to reuse some of that functionality, but that can be handled using traits, and those don't need to be represented in this diagram. I'll update the diagram. Thanks!

@ramsey
Copy link
Owner Author

ramsey commented Jan 8, 2019

Here's an updated diagram, removing the abstract classes:

ramsey-uuid-4 x

@Slamdunk
Copy link

Slamdunk commented Jan 8, 2019

I encourage you to try to provide functionality reuse only through composition.

Thinking about traits at this point of design process seems pretty much early to me: I would start with the interfaces you draw, then, if reuse appears hard and unintuitive I would split APIs into smaller one building appropriate factories when necessary. The resulted exposed code will be available like traits, but within precise contracts and predictable behaviors.

@ramsey
Copy link
Owner Author

ramsey commented Jan 8, 2019

if reuse appears hard and unintuitive I would split APIs into smaller one building appropriate factories when necessary

Can you elaborate more on this? The primary difference between types of UUIDs is in how they are generated, but the functionality for each, after generation, is basically the same (i.e. compareTo(), equals(), getBytes(), etc.). Are you suggesting that I have a base UUID object that is generated, and then each subtype (version 1, version 3, etc.) composes that object, implementing the same methods and proxying through to the underlying base object?

@ramsey
Copy link
Owner Author

ramsey commented Jan 10, 2019

@Slamdunk, just making sure you saw my question in the last comment. 😄

@pounard
Copy link

pounard commented Sep 6, 2019

maintain as much BC with the current API as possible, to ease the upgrade path from 3.x to 4.x, but I'd also like to get feedback from others regarding this. How painful would that break be?

We do extensively use this library for a year or such now, and the only public methods that we use are Uuid::uuidX() and Uuid::fromString() for creating, and UuidInterface for type hinting, if you don't change that, my migrations will be transparent !

As for the changes, something that I'd like to request are either:

  • better Uuid::fromString() performance, it's actually terribly slow and it makes a visible latency on some of our projects,
  • make it pluggable and be able to use a native extension for creating or parsing UUIDs, so that in case of terrible performance we can switch to another implementation yet keeping your interface for type hinting.

That was my 2 cents, by the way, thank you very much for maintaining this wonderful library that just works out of the box ! :)

@ramsey ramsey unpinned this issue Jan 8, 2020
@ramsey
Copy link
Owner Author

ramsey commented Jan 19, 2020

Closing this issue, since I've already implemented the new model changes in the 4.x branch. I'm still reworking a few things, but it's fairly settled at this point. Thanks for all the feedback in this thread!

@ramsey ramsey closed this as completed Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants