Drupal 8 has a broken UUID generator

Another thing -- the method of obtaining the initial hex string seems flawed to me.

The original (not truncated) hashes of the 16 random bytes should be at least nearly as good as the data it was based on; but truncating those hashes to 32 characters will undoubtedly introduce collisions, thus reducing the UUID space (because there is no margin for error at all -- every single permutation of 32 hex chars is needed to represent all the possible 16-byte values).

OTOH, PHP has bin2hex() which will do precisely what we really want.

FWIW I checked the spec and its errata carefully when writing this patch (and reviewing/writing patches for the UUID contrib module), but I agree that someone else should verify the correctness of the code.

The relevant sections of the RFC aren't very long. The only real confusion is over the bit-ordering, which the referenced errata clarifies. The new comments in the code should make it as easy as possible to follow.

There's also http://en.wikipedia.org/wiki/Universally_unique_identifier#Version_4_.28... (which obviously isn't the authoritative source, but is a simpler statement of the format).

A fairly gratuitous title change in the hope that someone reviews this 20-month-old fix before Drupal 8 is released.

I've hesitantly upgraded the priority to "Major" on the basis that this component has system-wide use. I thought "Normal" seemed appropriate at the time, but it will seem a little absurd to me if D8 gets released with a knowingly-incorrect UUID generator, and I presume this is completely lost in the sea of 2,500 "normal" D8 bugs (albeit not necessarily much more visible in the sea of 500 "major" D8 bugs).

Thank you for reviewing this, alexpott.

I'll change the comments if necessary, but...

I know there are coding standards for comments, but are they not in the service of maximising code readability and comprehension, and consequently open to exceptions where sensible?

For the record, I believe these comments:

$clock_seq_hi_and_reserved &= 0x3F; // 00111111
$clock_seq_hi_and_reserved |= 0x80; // 10000000

are objectively better than these comments:

// 00111111.
$clock_seq_hi_and_reserved &= 0x3F;
// 10000000.
$clock_seq_hi_and_reserved |= 0x80;

The positioning and alignment of the former makes both the hexadecimal values and the purpose of the sequence of binary operations instantly comprehensible and clear. The latter version of the comments can only detract from that clarity. (Not to any huge extent, sure; but it's still making it worse than it was, while doubling the number of lines in the process!)

You might elaborate on the comments in the latter version so they're not just a number on a line, but then you're padding out something which should have required no padding -- I believe the original version makes maximum sense with minimum waste/distraction. Why would we mess with that?

I'm not especially bothered by adding a full stop to the end of each comment (although, again, it's not an improvement for the binary number comments), but I really do object to not using end-of-line comments in cases when they make the code easier to read. I'll go ahead and change them if they need to be changed, of course; I'm just not in favour of making things objectively worse for the sake of conventions, so I wanted to say something about it before doing it, in case exceptions are allowable?

n.b. There is also a stated exception whereby "If each line of a list needs a separate comment, the comments may be given on the same line", and I would suggest that the instances in this patch are of a similar purpose, even if not actual list items.

I've tweaked alexpott's revised comments for readability; added mention of the missing $clock_seq_low to the list of variable names; and switched to binary format for the $clock_seq_hi_and_reserved manipulations in order to restore the clarity of those calculations without the need for the comments I'd used originally.

I'm happy with the revised comments. Those and some minor formatting aside, the only code change between #2 and this version is the change from 0x3F to 0b00111111, and 0x80 to 0b10000000 (n.b. "Binary integer literals are available since PHP 5.4.0" -- http://php.net/manual/en/language.types.integer.php ).

Hi wiifm :)

And a trivial change from "4" to '4', in case that was going to bother anyone :)

Spotted and fixed a typo in a comment.

Edit: Actually, the typo is in the original code. The RFC uses the name 'node', not 'nodes'.

I'll fix and re-roll...

Correct fix for the node/nodes naming mismatch.

Sigh. I'm hating the commenting standards quite a bit right now.

Ok, I'd best get phpcs set up and make sure I'm not submitting any more invalid comments. I'll post a new version shortly.

Well I've silenced that complaint with just an empty // instead of a completely blank line between those comments, and phpcs is happy.

I also took another look at the referenced ruby code and decided that this PHP bears so little resemblance to that, that there's just no purpose to pointing to it as even a loose basis. I'd keep it if they were noticeably similar, but they're just not IMHO, so I've removed those comments from the header.

Any chance of another review? To the best of my knowledge, this patch is good to go.

Many thanks, alexpott.

It doesn't seem like the 8.0.x commit has occurred?