Use PECL uuid function

[jderusse]

This PR replaces the RamserUUID object uuid_create.

Calls to the PECL library are faster and avoid storing complexe object for nothing.

[stayallive]

I'm fine with this, the leaner the better.

I am wondering why you have chosen for UUID_TYPE_DCE over UUID_TYPE_RANDOM what we used before (which should be v4). I know those 2 are the same in the polyfill but I can't find any documentation on the PECL extension and if it makes a difference, but I prefer UUID_TYPE_RANDOM better since it's more recognizable for the uninformed reader.

Also the build is failing because you left some @throws comments and unused imports (check the Travis logs) :)

[ste93cry]

Change looks good to me, the only thing I see from the announcement of this polyfill is that:

• for a v4 UUID the PECL extension is 3.25x slower than the polyfill while Ramsey is 1.30x slower
• the difference of time between the polyfill and Ramsey is not so big, while it is for the PECL extension (the PECL extension is the slowest while generating v4 UUIDs)

[stayallive]

That is interesting... it seems like ramsey/uuid might not be so bad after all and might be best to keep that since it looks like it is quite a bit faster than the PECL extension.

The polyfill itself is even faster but it looks like with the PECL extension you get a big downgrade in performance (comparing against ramsey/uuid) but without the PECL extension there is a bit of a win.

Overal it might be better to keep ramsey/uuid to be honest.

Good find (by @ste93cry) on the announcement, did not even question the "extension is faster" statement: https://symfony.com/blog/introducing-the-new-symfony-uuid-polyfill

Even more info here: https://jolicode.com/blog/uuid-generation-in-php

[ste93cry]

Ramsey can use the PECL extension too, however by default its usage is disabled and must be manually enabled, even if it's available. Also I don't know how much this extension is installed, so I cannot comment on it. The downside of this change is that the polyfill automatically uses it if it finds that it's available and there is no way to prevent it

[stayallive]

I agree, so if the consideration to make this change is speed I think this is going backwards when the PECL extension is actually installed and only slightly better if it's not :)

As far as I've gathered the PECL extension might be more secure/resistent to collisions since it uses a better random source, but that is not critically important in this case.

[ste93cry]

Ramsey should use the random_bytes feature of PHP as far as I saw, but I didn't look at the full source code. One of the reasons I chose it instead of keeping the custom function like in 1.x was because it's a battle-tested library with a strong and safe implementation. I really think that the only thing that should be taken in consideration here is if we want to depend on such 3rd party library just to generate a string (because we only do that) or if we are keen to slow down things for users having the PECL extension installed but make it faster, but probably less safe, for those who don't. One of the reasons the extension is slower is because it opens a file descriptor to /dev/urandom every time a UUID v4 is generated

https://jolicode.com/blog/uuid-generation-in-php

Given that this change is just an internal change and doesn't affect in any way the public API it makes sense in my opinion to merge it and accept the tradeoffs of slowing things down if the extension is enabled because I doubt that such extension is so commonly used

[jderusse]

Thank for your review and comments. Yes PECL, is 3 micro seconds slower than Ramsey/polyfill.
But I think that Event are not instanciated that often, and we can afford it.

The purpose of this PR was more about removing a dependency to a package (that already have 3 major version and a 4th in dev) and removing the burdon of maintaing/updating the dependency while keep Backward Compatibility with old version.

Feel free to close the PR if you think we won't affort such performance drawback, I wouldn't be offended 😉

[stayallive]

Given that this change is just an internal change and doesn't affect in any way the public API it makes sense in my opinion to merge it and accept the tradeoffs of slowing things down if the extension is enabled because I doubt that such extension is so commonly used

This is a good point @ste93cry, in that case i'm also down for this!

---

Edit: I've upped the dependent version of the polyfill because symfony/polyfill#209

[ste93cry]

The purpose of this PR was more about removing a dependency to a package

As I already said, I'm more than happy to drop a dependency if we can and it makes sense and in this case for what we are doing which is just generating a UUID and then converting it to a string it totally makes sense, so you have my approval for the change. There is just that small change on the test class that I requested, then for me LGTM

[stayallive]

I've made the change on the test class (for the @throws) and just upped the version constraint for the polyfill (see: symfony/polyfill#209). Should be all good now :)

[jderusse]

tests are green. (sorry @stayallive i force-pushed you commit).

Looks like Appveyor is out

[ste93cry]

Looks like Appveyor is out

I'm looking at the issue, since PHP 7.1 is not maintained anymore the link to download the Windows version is not working anymore 🤦‍♂

[stayallive]

@jderusse looks like everything is here so no worries :)

Just some AppVeyor issues, but the PR is solid, thanks!

[Jean85]

Very good job everyone, this PR is pretty solid! Let's hope to fix AppVeyor ASAP... Maybe we can look at how Symfony fixed its build?

[ste93cry]

@jderusse can you please force push to retrigger the CI? Once all checks are green I will merge

[jderusse]

@ste93cry Done. But this PR targets develop (changed by you 2 days ago) While #938 has been merged in master.

[ste93cry]

Yes sorry my bad, I didn't notice it! I will rebase master on develop as soon as possible and then we should be fine here too

[ste93cry]

LGTM, thank you for the hard work