Thanks @Eevans, we'll try to get this done as soon as we can

Not a thorough review, but some notes of concern suggesting we should perhaps use the version 3 of the library, and so review that instead of 2.8.3.

Security notes

The Uuid::compareTo method doesn't seem to run in constant time, and so shouldn't be used to compare anything where a timing attack would be a concern.

Other notes

• The application requires as a -dev dependency \Moontoast\Math but uses it in main code (Uuid::getDateTime).
• MediaWiki isn't a 64-bits ony application, and so any call to Uuid::getTimeLow, Uuid::getFields or Uuid::getNode methods should be avoided.

These two last concerns are mitigated in the current master tree, so it only apply to 2.8.3, not version 3 of the library.

Thank you @csteipp and @Dereckson for the promptly response! @Eevans, any reason not to switch to ver 3 of the lib?

MediaWiki core has a UIDGenerator class that creates v4 ids, is there a reason that won't work? (I'm totally unfamiliar between the difference between v1 and v4)

In T120133#1847368, @Legoktm wrote:

MediaWiki core has a UIDGenerator class that creates v4 ids, is there a reason that won't work?

The reason for importing this lib is that we need both v1 and v4 UUIDs in the extension emitting events.

(I'm totally unfamiliar between the difference between v1 and v4)

v4 UUIDs are time UUIDs (i.e. UUIDs based on the current time stamp) and are therefore not unique. v1 UUIDs, on the other hand, guarantee uniqueness across processes and machines and we use it for setting event IDs in order to be able to de-duplicate them.

And on the EventBus change https://gerrit.wikimedia.org/r/#/c/254086/11/composer.json

ramsey/uuid" claims to provide "RFC 4122 version 1, 3, 4, and 5 UUID"
MediaWiki core already has such support embedded via includes/utils/UIDGenerator.php
So I would get rid of that ramsey/uuid dependency and reuse MediaWiki core built-in class.
The only usage being for a v1 UUID (based on time/ mac address) maybe our getTimestampedID128 is good enough for that purpose :-}

ramsey/uuid is being proposed for mediawiki/vendor with https://gerrit.wikimedia.org/r/#/c/256575/ I commented there:

I am not opposed to introducing this dependency, but I am wondering whether we could add UUID v1 in core or replace our home made generator with ramsey/uuid :-)

So again. I am not opposed to ramsey/uuid. But I find it annoying we will end up with duplicate code between core and the 3rd party lib.

In T120133#1847395, @hashar wrote:

So again. I am not opposed to ramsey/uuid. But I find it annoying we will end up with duplicate code between core and the 3rd party lib.

+1, I sympathise. I'd be perhaps more open to the latter. UUID generation is tricky, so I'd rather have it maintained elsewhere.

In T120133#1846778, @Dereckson wrote:

Not a thorough review, but some notes of concern suggesting we should perhaps use the version 3 of the library, and so review that instead of 2.8.3.

Unfortunately, version 3 is PHP v5.4+.

In T120133#1847382, @mobrovac wrote:

In T120133#1847368, @Legoktm wrote:

MediaWiki core has a UIDGenerator class that creates v4 ids, is there a reason that won't work?

The reason for importing this lib is that we need both v1 and v4 UUIDs in the extension emitting events.

(I'm totally unfamiliar between the difference between v1 and v4)

v4 UUIDs are time UUIDs (i.e. UUIDs based on the current time stamp) and are therefore not unique. v1 UUIDs, on the other hand, guarantee uniqueness across processes and machines and we use it for setting event IDs in order to be able to de-duplicate them.

Actually, that's backward, if I understand you. Version 1 UUIDs are based on a timestamp (100 nano-second intervals from an epoch based on the adoption of the Gregorian calendar), and a "node ID" (typically the machine's mac address). Version 4 UUIDs are based entirely on (pseudo)random numbers. Version 4 is a successor to version 1, the biggest complaint with v1 being that they are not sufficiently opaque, (which incidentally, is exactly why they are so convenient, assuming you aren't concerned about the security implications of leaking the when and where of generated IDs.

https://en.wikipedia.org/wiki/Universally_unique_identifier#Variants_and_versions

In T120133#1847395, @hashar wrote:

And on the EventBus change https://gerrit.wikimedia.org/r/#/c/254086/11/composer.json

ramsey/uuid" claims to provide "RFC 4122 version 1, 3, 4, and 5 UUID"
MediaWiki core already has such support embedded via includes/utils/UIDGenerator.php
So I would get rid of that ramsey/uuid dependency and reuse MediaWiki core built-in class.
The only usage being for a v1 UUID (based on time/ mac address) maybe our getTimestampedID128 is good enough for that purpose :-}

ramsey/uuid is being proposed for mediawiki/vendor with https://gerrit.wikimedia.org/r/#/c/256575/ I commented there:

I am not opposed to introducing this dependency, but I am wondering whether we could add UUID v1 in core or replace our home made generator with ramsey/uuid :-)

So again. I am not opposed to ramsey/uuid. But I find it annoying we will end up with duplicate code between core and the 3rd party lib.

I agree. And there is this (authored by @aaron; previously abandoned), which would introduce v1 generation into UIDGenerator. It is not uncontroversial either though, I'm afraid.

I am more and more convinced (as already suggested here), that we should eliminate the extension's dependency on ramsey/uuid (at least for now), and instead generated a v4 UUID (which UIDGenerator is capable of doing). The only drawback that I am aware of, is that we would be unable to extract a timestamp from the ID, which should not be an issue since there is a separate attribute in each event that contains an identical ISO8601 timestamp.

If it is determined that we absolutely need a v1 UUID (despite the redundant timestamp), then we can take up the issue of an external dependency vs. @aaron's UIDGenerator changeset (or similar) at a later date.

In T120133#1848258, @Eevans wrote:

I am more and more convinced (as already suggested here), that we should eliminate the extension's dependency on ramsey/uuid (at least for now), and instead generated a v4 UUID (which UIDGenerator is capable of doing). The only drawback that I am aware of, is that we would be unable to extract a timestamp from the ID, which should not be an issue since there is a separate attribute in each event that contains an identical ISO8601 timestamp.

Just an update here; As of patch 23, I have done exactly this, removed the ramsey/uuid dependency and made use of a v4 UUID generated by includes/UIDGenerator.php

Per @Eevans and @aaron MediaWiki core now has support for UUIDv1 https://gerrit.wikimedia.org/r/#/c/56567 \O/