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

[Suggestion] Abstract use of ramsey/uuid lib #47

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@MattKetmo
Member

MattKetmo commented Dec 21, 2015

Hello,

I created a Uuid namespace to encapsulate call to Rhumsaa/Ramsey uuid library.
This is needed if to seamlessly upgrade to version ~3.0 of the lib (which changed its namespace).
Using a specific interface with a simple generate() method allows to decouple from the implementation and let users choose the version of the lib they want to use.

One thing could be debated because it can be considered as a non-BC change: I now handle string instead of Uuid object. But it could possible to create a simple Uuid class with toString() and equals() methods.

@prolic

This comment has been minimized.

Member

prolic commented Dec 21, 2015

Nice idea. I personally would prefere using Uuid objects implementing the UuidInterface (https://github.com/ramsey/uuid/blob/3.1.0/src/UuidInterface.php). Let's see what @codeliner thinks of it, he's currently on vacation.

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 21, 2015

Using this UuidInterface means updating every prooph packages to ramsey/uuid: ^3.0 directly and keep the coupling with this lib. That's a possible solution but not the one I'm suggesting :)

@prolic

This comment has been minimized.

Member

prolic commented Dec 21, 2015

No, I meant prooph can create its own UuidInterface and implement that on the proxies. But currently I would prefer updating to ramsey/uuid >= 3 directly. Let's see what @codeliner says about this when he's back.
Any way, thanks for your contribution.

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

Hey @MattKetmo and @prolic ,

sorry for the delay.
I like the idea of decoupling prooph from uuid library with an UuidInterface and proxy classes for Rhumsaa/Ramsey uuid library.

Our initial idea was to mark the current versions of prooph components as LTS versions (will be done at the beginning of next year) and switch to Uuid 3.x in the next major versions of prooph components. But decoupling is always a good idea even for such a basic helper like ramsey/uuid. We missed that in the past.

If we want to tackle the update with an interface and proxy classes, we should also consider the same for beberlei/assert. If Benjamin ever wants to rename his vendor to benjamin, eberlei or whatever, we will be prepared :)

@MattKetmo Can you add an interface + proxies in your PR? I've created a develop branch for prooph/common as we also need to update other components as soon as the new interface is available. Please submit your updated PR against the new develop branch.
Oh, and btw. great slides: https://moquet.net/talks/phpcon-2015/ . Except the framework overview. You are missing a green circle around prooph 😃
Thanks for your contribution. We really appreciate it.

@sandrokeil

This comment has been minimized.

Member

sandrokeil commented Dec 22, 2015

@codeliner Why not create a new repository for the Uuid Wrapper, so other people can use it too, independend of prooph/common?

I don't know if it is useful to create for every external library a wrapper repository. We should do that only if it makes sense, like for ramsey/uuid. So people are free to use the 2.x or 3.x series.

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

@sandrokeil sounds reasonable. But then I'd say the wrapper should be provided by @ramsey.
A stand-alone wrapper for rhumsaa|ramsey/uuid under the prooph vendor is way too small for me. prooph/common is our shared kernel for all other prooph components and the perfect place for our own wrapper. If we want to write a wrapper useful for other projects then this is best placed under the ramsey namespace.

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

We can provide a PR for ramsey and if it's rejected we can provide it under prooph/common.

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

@prolic 👍

@sandrokeil

This comment has been minimized.

Member

sandrokeil commented Dec 22, 2015

@prolic @codeliner For clarification, is anybody working on a wrapper as PR to ramsey library? /cc @MattKetmo

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 22, 2015

Hey, I see the conversation has deviated from its original suggestion (find a way to not depend on a third party library).

Using another lib (the wrapper) instead of another (the concret implementation) doesn't solve the issue if we don't own that piece of code. It just moves it.

But maybe this "wrapper" will make the ramsey/uuid library safer for the short and long term. Maybe that's enough to consider that won't annoy end users. Or perhaps the ramsey/uuid:3.x branch should keep a reference to the Rhumsaa namespace (class \Rhumsaa\Uuid\Uuid extends \Ramsey\Uuid\Uuid {}). That would make thing much simpler and allow all users and libraries to migrate progressively.

Btw if we look at Broadway lib, they already did that kind of wrapper (qandidate-labs/broadway-uuid-generator, which was the main idea of this PR) to not depend directly on a third party (and also to not depend on a static method :)).

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 22, 2015

@codeliner About my slides I underlined Broadway mainly because its the lib I used to illustrate the examples, and also because it's much simpler for beginner to understand the concepts (than Prooph or other libs).

I started to read in depth the code of Prooph a few days ago and have some questions (about metadata in eventstore and concurrent processes) but I will use the google group instead of github issues ;)

@ramsey

This comment has been minimized.

ramsey commented Dec 22, 2015

I would be happy to review a PR for this on ramsey/uuid, but my preference would be for a separate library, since I want to encourage people to upgrade to 3.x instead of remaining on 2.8, which I am not maintaining (except for minor issues/bugs).

The upgrade path to 3.x is very simple and primarily requires only changing your namespace references.

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 22, 2015

I understand upgrading your lib in one app or lib is trivial, but the main issue is to upgrade the libs which we don't own but depend on it. Plus it force a non-BC change for those libraries (and the users which depend on it). Upgrading dependencies of dependencies is definitely not something trivial :)

I will try to provide an "replacement add-on" for users who use ramsey/uuid:3.x but want to use a lib which requires (ramsey|rhumsaa)/uuid:2.x. Let's see if it works ;)

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

@ramsey thx for joining the discussion.

@MattKetmo awesome! As soon as your add-on is ready let us know, so we can update prooph components as well.

@ramsey

This comment has been minimized.

ramsey commented Dec 22, 2015

I'm interested in taking a look at it. Alternately, I could do like Guzzle, so that 2.x can be installed and used independently of 3.x.

Obviously, I would prefer to see others follow the upgrade path, but that might not always be an option. :-)

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

@MattKetmo You can also use our chat for questions and discussions (about metadata in eventstore and concurrent processes): https://gitter.im/prooph/improoph

Also I'd like to know why broadway is easier to understand for beginners? What can we improve to change that?

Regarding third party dependencies in general: Uuid and Assertion are the only external dependencies which are directly used by prooph components. All other dependencies are optional or moved to integration packages (which was a lot of work). These two dependencies are "standards" for us like \DateTime or SPL classes even if they are not part of PHP. We use them in every project.

However, hiding every external dependency behind an interface in the prooph namespace is a good idea. No question. But we would need to do the same for beberlei/assert´and that is a lot of work.

I'd say we should wait with a decision and first check if your add-on solves the uuid dependency problem. @sandrokeil @prolic What do you think?

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 22, 2015

Ok my PoC seems to work :)

I've created 3 libraries:

  • mattketmo/foobar which depends on ramsey/uuid: ^2.8
  • mattketmo/barbaz which depends on ramsey/uuid: ^3.0
  • mattketmo/hello which depends on mattketmo/foobar and mattketmo/barbaz

Here is the requires section of the hello app:

"require": {
    "ramsey/uuid": "^3.0",
    "mattketmo/foobar": "*",
    "mattketmo/barbaz": "*"
}

As expected, a composer install fails.

Note that the "ramsey/uuid": "^3.0" here is not mandatory but it shows
the developer wants to use the 3.x version of the lib, but can't because
of one other dependency.

Now I created a tiny package ramsey/uuid-legacy with ony:

<?php

namespace Rhumsaa\Uuid;

class Uuid extends \Ramsey\Uuid\Uuid
{
}

Plus some options in composer.json (provide and replace).

Then I add that package in my hello app:

"require": {
    "ramsey/uuid": "^3.0",
    "ramsey/uuid-legacy": "*",
    "mattketmo/foobar": "*",
    "mattketmo/barbaz": "*"
}

And now I can do a composer install :)

You can find the PoC at MattKetmo/ramsey-uuid-legacy.
There are 2 commits: the first one where you can't do a composer install in the hello/ folder and the second where it works with the "add-on".

I think that the most simplest thing I can do.
I only created the main Uuid class but the idea is to do the same with the other (public) classes of the 2.8 branch.

Also ramsey/uuid-legacy is not a good name (maybe ramsey/uuid-2x-bridge?).

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

@ramsey thanks for joining the discussion.

Generally I feel like we are doing too much here. The upgrade to uui 3.x series is very easy and can be done within minutes (just use phpstorm search-and-replace-feature!). Creating and maintaining a wrapper lib for old 2.x series to prevent upgrading does not only takes lot of time, it's a bad idea imho.

So currently I would still prefer to just upgrade to 3.x series and when some users need to change their code base because of this, then first it's a good thing and second it's done within a couple of minutes.

@ramsey

This comment has been minimized.

ramsey commented Dec 22, 2015

Additionally, use semver properly with changelog notes about the breaking change.

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 22, 2015

With the "ramsey-uuid-legacy" hack, Prooph doesn't even need to upgrade to 3.x (quickly). I can now use Prooph components without downgrading ramsey/uuid to 2.x. And that's also true for any lib in the same situation.

@ramsey are you ok if I publish a ramsey/uuid-2x-bridge package?

@ramsey

This comment has been minimized.

ramsey commented Dec 22, 2015

I'm okay with you publishing it. I'll even update the ramsey/uuid README to mention it under my Upgrading section.

Thanks!

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

@prolic I would absolutely agree with you if the only problem would be that application + prooph components require the same uuid series. Things get more complex when different libraries require different versions. You cannot force all libraries to upgrade to 3.x at the same time and you cannot influence when the upgrade takes place.

We should think about upgrading soon but things like LTS versions need to be discussed first. Upgrading means new major versions and our last major versions are just one month old.

@MattKetmo

Prooph doesn't even need to upgrade to 3.x (quickly)

Thanks for that! Next year will start with a lot of non open source work so I consider every movable task a good task :). However, we will definitely schedule the upgrade to 3.x.

@prolic , @sandrokeil what about the Prooph\Common\UuidInterface? When I understand you correctly we don't need it (atm)?

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

Prooph\Common\UuidInterface is a good name.

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

@prolic So the idea is still that we define an interface on our side and change all components to only work with Prooph\Common\UuidInterface instead of Ramsey\Uuid?

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

oh sorry, I meant Prooph\Common\Uuid without the interface suffix of course ;)

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

Still prefer just upgrading to 3.x series, but yes.

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

Oh btw, we can make use of https://pecl.php.net/package/uuid with the interface.

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

hhm, also a nice option. Ok, but if you prefer a simple upgrade to 3.x then let us focus on that. We can add an interface later.

@prolic

This comment has been minimized.

Member

prolic commented Dec 22, 2015

Well, I am not alone here. It's not that I make any decisions on my own here :-)

@sandrokeil

This comment has been minimized.

Member

sandrokeil commented Dec 22, 2015

I would prefer to change nothing here at the moment and should upgrade to 3.x later. @MattKetmo has build a nice solution for this problem. 👍 We should add a hint to the docs for the bridge.

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 22, 2015

I love democracy (sometimes). We have 3 votes (prolic, sandrokeil, me) for upgrading to 3.x without a prooph uuid interface.

So @MattKetmo I'm going to close this PR without merging it as your ramsey/uuid-2x-bridge is a great alternative for now.

@codeliner codeliner closed this Dec 22, 2015

@ramsey

This comment has been minimized.

ramsey commented Dec 22, 2015

@prolic You may also use pecl-uuid with ramsey/uuid v3. For details and examples, see https://github.com/ramsey/uuid/wiki/Ramsey%5CUuid-Cookbook

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 22, 2015

Ok I'm fine with that 👍

Note that I finally published the package under the name mattketmo/uuid-2x-bridge because Packagist didn't let me submit a package using ramsey/ as a vendor.

I will publish a doc tomorrow to explain how to use it.

@MattKetmo

This comment has been minimized.

Member

MattKetmo commented Dec 23, 2015

@sstok

This comment has been minimized.

sstok commented Dec 24, 2015

Also I'd like to know why broadway is easier to understand for beginners? What can we improve to change that?

Prooph can look a bit "big" or overwhelming but I must say that the documentation makes things very clear and It's actually simpler to implement then Broadway.

All though an actual code example on the homepage would help newcomers (compared to a big flowchart). Like done in http://getprooph.org/service-bus/intro.html

@codeliner

This comment has been minimized.

Member

codeliner commented Dec 26, 2015

Thank you @sstok for your feedback. This is really important for us. I've put your intro suggestion on the todo for next version of the docs. We are working on a better front page (and logo) for the docs but need to contribute to bookdown.io first as it does not support a front page yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment