Low level UUID API in core

+++ b/modules/system/system.testundefined
@@ -2466,3 +2466,37 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+   * @param $uuid

@param $message,
@param $group,
@see DrupalTestCase::assertTrue()?

+++ b/modules/system/system.testundefined
@@ -2466,3 +2466,37 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+    $this->assertTrue(uuid_is_valid($uuid), $message, $group);

Considering the @return comment, missing a return here?

+++ b/modules/system/system.testundefined
@@ -2466,3 +2466,37 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+  function testUuidHandling() {
+    // This is a valid UUID, we know that.
+    $valid_uuid = '0ab26e6b-f074-4e44-9da6-1205fa0e9761';
+    // Test the uuid_is_valid() function.
+    $this->assertTrue(uuid_is_valid($valid_uuid), 'UUID validation works.');
+    // Test generating a UUID.
+    $uuid = uuid_generate();
+    $this->assertUUID($uuid, 'UUID generation works.');

Assert an invalid UUID fails validation? Regular Expressions can be a tricky business so I'm not sure how far to take that before false positives can be considered eliminated...

Assert the generated UUID passes validation? EDIT: Scratch that, looked at the wrong line...

Assert we won't get the same UUID twice in a row? Perhaps "obvious" that can't happen, but it's the basic purpose of this code so why not test it. No? ;)

Assert the correct callback got assigned?

14 days to next Drupal core point release.

Subscribe

Just FYI here is another userland lib for UUID generation:
https://github.com/fredriklindberg/class.uuid.php

Think we should put in some effort to create a solid standalone UUID lib with automatic fallback to the pecl lib. The above code and the work you guys have done should be a good basis there. I havent really spend some pondering an API or anything myself yet. Just wanted to throw this out there as the PHPCR/Jackalope project also needs UUID's:
https://github.com/phpcr/phpcr/blob/master/src/PHPCR/Util/UUIDHelper.php

Should use DrupalUnitTestCase, no?

We usually commit with one implementation. I'd like to see that here, personally. Just my .02

Is it worth considering making the uuid.inc a variable so that it could be swapped out? Currently there is no other way to generate UUIDs other than PHP or PECL. If that hard-limit is acceptable, then it's no matter - but what if we just had a uuid_generate() function that used always-available PHP generation. If someone has PECL they could use $conf['uuid_inc'] = 'sites/all/includes/uuid.pecl.inc';

Sub...

I don't see having a bunch of different possible ways to generate UUIDs as necessary. We need a universally-works way, and a fast way. :-) Whether that's our own code or a 3rd party class, I don't see a need to do more than that.

@Crell Yeah that absolutely makes sense.

#12: No, I don't think so. The only question for me is if we commit the patch as is or drop in the class Lukas links to above. :-)

Both link libraries require re-licensing and this is so freaking simple that I don't think an external library is necessary. As-is looks good to me.

As you can see here there are multiple versions for UUID generation:
http://en.wikipedia.org/wiki/Uuid#Variants_and_versions

Many of which seem to be supported by https://github.com/fredriklindberg/class.uuid.php
Also that lib is dual licensed BSD and Apache, BSD gives you total freedom in terms of licensing.

Also when you start to want a solid set of tests for this non trivial code then the question of code sharing becomes quite obvious to me.

As for how to do the fallback to PECL. it might indeed maybe make the most sense to refactor this around the pecl API and then include the file if the function is not defined inside the autoloader.

I don't think we want to support different versions of the standard, we're best off sticking with one throughout core. It looks like the PHPCR lib is very similar to what we already have here. I'd rather get this in and if we need to tweak a bit after then we can. I think this is a great start to build off of, but I'd like to see maybe a review or two before we RTBC.

BTW, it doesnt need to be 3rd party from the POV of Drupal. If Drupal makes whatever lib Drupal ends up creating (including tests etc) available somehow (ideally via git) we would consider adopting it.

BTW if you do use git you can even extract this code from inside a larger repo with subtree merge:
http://help.github.com/subtree-merge/

OK, lets proceed without any implementations. This is all just taste, but my preference is:

1. Remove the drupal_static() from uuid_generate(). That just saves a function_exists() which is cached by php anyway. Over-caching is not 'harmless'.
2. The method assertUuid() need not exist. Just do the assertion in testUuidHandling(). Is easier to follow that way.

Overall I think this is a great patch and it helps us get closer to UUID in D8 core. I really appreciate dixon_ taking over my work on this. I plan to backport this patch for the 7.x-1.x branch of UUID in contrib.

+++ b/includes/uuid.inc
@@ -0,0 +1,72 @@
+    if (function_exists('uuid_create')) {

This should be if (function_exists('uuid_create') && !function_exists('uuid_make')) { as Debian/Ubuntu ship the horribly broken OSSP UUID implementation as php5-uuid. This conflicts with PECL functions.

+++ b/includes/uuid.inc
@@ -0,0 +1,72 @@
+    }

For performance reasons I think we should check for COM so we have a C based implementation for Windows users too.

11 days to next Drupal core point release.

Code and tests look good to me.

I don't see why we need to load this code every request when nothing in core uses it and we'll be generating uuids very rarely. It should either be a class or have calling code require it manually for now.

catch is right. A UUID-managing class is more self contained and lazy-loadable. I personally wouldn't use statics on it and just instantiate an object, but either way a class lets us not worry about when we do or don't need this rare code.

That's the direction that core needs to take in general; we may as well start here with something new.

Having a class for this seems a bit unnecessary to me. Writing a class with only static methods is ugly in my opinion, and just instantiating things just to do one single little thing also seems odd. To me this feels like we would just force this functionality into a class just because we don't have a nice way of lazy-loading it.

If we are concerned about the include we could just remove the include in common.inc, and have the user include it themselves for now? Won't we have plenty of time to change other parts of core later on, where using classes makes more sense?

Works for me, typo for 'validtaing'.

I'm assuming there's absolutely no problem if a site starts out using the PHP implementation then installs the PECL extension?

@catch No, the generator is only cached per script execution. The value isn't stored - except via a static.

Please. If we go the class route, let's do this properly. I don't want to see switches in there.

Also, the version character looks wrong in generatePhp. The "4" should be the most significant bits (ie. the left) of time_hi_and_version.

I have no problem with switch for the check of uuid generating method, because what generate() returns is an uuid, not a cached generating method.

I like the patch in #20, or if we go with class, we should have an autoload mechanism. Currently we include mail.inc in every full bootstrap, but how frequently is it used?

So properly to me would mean this:

1. Interface
2. Procedural factory (uuid() or drupal_uuid()) - includes the logic in the current switch statement to cache the class and returns it. I don't think we need to make it configurable at all.
3. One class each for the three mechanisms.

@skwashd - I meant more is there any increased chance of collisions or other issues if you switch from one method to the other, or are they completely interoperable?

I'm sitting here at the DrupalCon London code sprint next to dixon_ reviewing his patch from #25.

This is my first patch review.

I found the patch working and the tests running successfully. After reading over the coding standards I found them to be met in the patch, except of some lines of comments exeeding 80 characters on lines 18, 84 and 85.

Cheers,
Georg

Also, I think we want cryptographically strong randomness here (ie. we want to use drupal_random_bytes(), not mt_rand()).

Another thing to consider would be using type 1 UUIDs instead of type 4 (initialized with a hash of the database credentials for the MAC address), as it would give us locally-monotonic identifiers.

@Damien: I don't remember why v4 is used, but it was discussed before. A simple argument is that v1 used MAC address (the last 48 bits of UUID).

About drupal_random_bytes, that could be ok, but we need some bit manipulating too.

@catch: they are all interoperable. The chance of collision depends on how well we generate pseudo random number.

#jcisio: I suggested to use a hash of the database credentials for the MAC address. That's probably what makes the most sense in the perspective of Drupal.

@catch gotcha. The UUIDs would remain unique which is the main consideration, they don't need to be reverse engineered.

@damz The reason mt_rand() is used instead of drupal_random_bytes() is that we need random integers not a string of random characters. There is a formula kicking around which specifies how to generate a v4 UUID.

I looked at generating version 1 UUIDs when I did the D7 port. There is no efficient, reliable cross platform method of obtaining the MAC address of the primary interface of a server. I also seem to recall the 100 nano second intervals since the start of the Gregorian calendar meant we can only support 64 bit platforms under PHP.

The PECL UUID extension uses v4 and so it makes sense for us to use something similar in our userspace implementation.

The com_create_guid method returns a v2 UUID - which is what Windows uses internally.

Given that we can't use md5() is D7, v3 is out. We could look at v5 using http://{$_SERVER['HTTP_HOST']}/[entity-type]/[id]/[revision]. To avoid collisions we could look at including a timestamp too.

I think we should aim to comply with a recognised UUID version, not introduce another Drupalism by creating our our algorythm for creating UUIDs.

Given that we can't use md5() is D7, v3 is out. We could look at v5 using http://{$_SERVER['HTTP_HOST']}/[entity-type]/[id]/[revision]. To avoid collisions we could look at including a timestamp too.

I would not recommend this. Precisely because it requires a id and revision beforehand :)

We could decide to have v5 UUIDs based on the content itself (similar to how Git generates its own identifiers), but that's for another day. I'm fine with v4 UUIDs for now.

+++ b/includes/uuid.incundefined
@@ -0,0 +1,115 @@
+   * This function first checks for the PECL alternative for generating
+   * universally unique identifiers. If that doesn't exist, then it falls back

It no longer does this now that's in the factory.

Looks good to me otherwise.

5 days to next Drupal core point release.

This is an extremely nitpicky re-roll for documentation issues. I don't know if 'a UUID' or 'an UUID' is more proper (I find lots of examples of both in Google) but I standardized on 'an' just for consistency. Otherwise this looks good to go to me.

an ID, but a UUID.

+    return sprintf('%04x%04x-%04x-%03x4-%04x-%04x%04x%04x',
[...]

As I mentioned back in #28, this is wrong. A v4 UUID is:

xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx

not:

xxxxxxxx-xxxx-xxx4-yxxx-xxxxxxxxxxxx

This version of the patch uses a new algorithm for generating the UUID, which generates RFC 4122 compliant UUID v4 strings. The new implementation relies on drupal_random_bytes() which provides better entropy than multiple calls to mt_rand(). The logic is based on Ruby's UUIDTools generate_random() method. I also looked at Python's implementation while writing this.

I also changed some of the docs. Based on a straw poll "a UUID" is how most people would say it.

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+interface UuidInterface {

Minor, but does anyone other than me like UUIDInterface better as well? (Applies also to the classes...)

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+   * Generates an universally unique identifier.

Also below:
an -> a

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+    $class = variable_get('uuid_class', 'UuidPhp');

I personally don't object on this being configurable via a variable, but above it was said that it shouldn't be.

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+      $time_low, $time_mid, ¶

Trailing whitespace.

+++ b/modules/system/system.test
@@ -2466,3 +2466,43 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+      'description' => "Test the handling of universally unique identifiers.",

Minor: Could use single quotes.

+++ b/modules/system/system.test
@@ -2466,3 +2466,43 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+    // Test the uuid_is_valid() function.
+    $test = (
+      $uuid->isValid($valid_uuid)
+      && !$uuid->isValid($invalid_uuid1)
+      && !$uuid->isValid($invalid_uuid2)
+    );
+    $this->assertTrue($test, 'UUID validation works.');

This could be split into 3 separate assertions. Would make it more readable in my opinion.
I'm not into the subject enough to know whether it makes sense to test a couple more valid and invalid UUIDs.

+++ b/modules/system/system.test
@@ -2466,3 +2466,43 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+    // Test generating an uuid.

(an -> a as above, but also:)
uuid -> UUID

+++ b/modules/system/system.test
@@ -2466,3 +2466,43 @@ class SystemIndexPhpTest extends DrupalWebTestCase {
+    // Just to demonstrate the purpose of the UUID functionality.

Could be something like "Verify uniqueness of the generated UUIDs.".

I agree Uuid* doesn't look great, but UUIDPHP or UUIDPECL look pretty bad too. The class naming convention is (sort of) PEAR/PSR-0 compliant.

I cleaned up the docs some more. I've reworked the tests to use UnitTest instead of WebTest as this is a low level library. There code paths are exercised further with this code..

I got sick of waiting for SimpleTest to run, I hope PIFR has good new for me (later) in the morning.

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+ * Interface that defines an UUID plugin.

Still one left.
(an -> a)

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+   * Generates a Universally Unique IDentifier (UUID).

Is the D capitalized on purpose? Looks a bit weird, but I guess it could be to stress where the abbreviation UUID comes from.

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+   *   A UUID, as hexadecimal string made up of 32 hex digits and 4 hyphens.

Maybe: "..., as A hexadecimal string...", I think that makes it more readable.
Also:
"hex digits" -> "hexadecimal digits"

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+/**
+ * Factory class that determines which UUID implementation to use, and uses
+ * that to generate and validate UUIDs.

This should be only one line. Either cut out the second part or move that to a dedicated paragraph.

+++ b/includes/uuid.inc
@@ -0,0 +1,121 @@
+   *   The string to test.
+   * @return

There should be an empty line between @param and @return.

Still needs some more opinions on whether or not to use a variable. (Count me as +1).

The function isValid needs to be changed to test for the v4 format, too. That is xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx (the 13th digit is 4 and the 17th digit y is [90ab]).

I don't know why we'd use a variable_get() here. There's no reason the class should be user-configurable; use a C-compiled version if available, or a user-space if not. I mean, even I have a hard time arguing against a switch statement here, and I hate switch statements. :-)

The unit test (yay for real unit tests!) looks like it's testing a couple of different things in one method. It's cleaner if each unit test method checks one thing only, so we should break up that method into a couple of methods. They should still be pretty damned fast as it's just using UnitTestCase.

I have no expertise or, really, interest in the UUID version logic so I defer to those who have looked at the UUID spec.

I'm fine with a quasi-plugin-ish approach for now until we get a more robust plugin system in place. It would still be one of the easiest quasi-plugin-ish systems we have in core to convert later. :-)

@tstoeckler I'll look at rerolling it later today. The "Universally Unique IDentifier (UUID)" convention is used through out the contrib module. I think it makes it clear to the reader what the abbreviation/acronym stands for.

@jcisio It needs to be able to validate any UUID version. If we wanted to do that we'd need logic to validate a UUID of any valid version. I think for now it is good enough to check that it is in the right format. There is only so much time one can spend reading RFC 4122 in a weekend.

@Crell Good call on the unit tests. I'll refactor them today.

I also realised after posting the patch that I don't need the initial substr(), the final substr() can just grab the bit we need.

From the discussion above, of which I understood very little, it seemed as though a userspace implementation might be preferable over a C implementation in case it had stronger uniqueness for example. If that is not the case, then a variable really doesn't make much sense.

Also this deserves a changelog.txt entry.

Rerolled based on feedback - the tests pass this time too.

Pending some low-level plugin features, I think we can have for now both the variable + a series of switch statement in case the variable is not defined.

Hi all,

I was reading out of curiosity your thread and I was wondering why you generate the UUID through PHP when MySQL(Link) is able to generate one.

Maybe we should provide a method in the MySQL driver that fallback to PHP generation if it doesn't exists ?

Ok @dixon_, as long as we are sure to generate unique UUID.

Avoiding variable_get() also means that if the phpcr folks did want to use this there'd be no need to subclass it.

I don't think we need a @todo for pluggability let alone the unified plugin system. Or if we do them @todo consider making this pluggable so it's less apologetic.

+++ b/includes/uuid.incundefined
@@ -0,0 +1,156 @@
+/**
+ * Interface that defines a UUID plugin.

Could we use backend rather than plugin? Plugin isn't really used in core yet.

+++ b/includes/uuid.incundefined
@@ -0,0 +1,156 @@
+ * that determines which UUID implementation to use, and uses that to generate

Drop 'that'?

+++ b/includes/uuid.incundefined
@@ -0,0 +1,156 @@
+   * This isn't the nicest way of doing this, but as the plugin sysytem
+   * in Drupal core is likely to change this is best way forward for now.
+   * When the plugin system is accepted, this code will be refactored to

This is too apologetic, I would remove the whole hunk.

+++ b/includes/uuid.incundefined
@@ -0,0 +1,156 @@
+
+    // Debian/Ubuntu uses the (broken) OSSP extension as their UUID ¶

Trailing space (via dreditor)

1 days to next Drupal core point release.

Rerolled per #63

+++ b/includes/uuid.inc
@@ -0,0 +1,151 @@
+    // Debian/Ubuntu uses the (broken) OSSP extension as their UUID
+    // implementation. The OSSP implementation is not compatiable the
+    // PECL functions.

I think there's a missing "with" in the second sentence.

I'm fine with this patch otherwise. +1

also " compatiable"

1) Do we have an actual use case for these to ship with? Maybe ship use with Entities?

+++ b/includes/uuid.incundefined
@@ -0,0 +1,151 @@
+  protected function determinePlugin() {
+    static $plugin;
+    if (!empty($plugin)) {
+      return $plugin;
+    }
+

2) It would be nice to be able to override which plugin is chosen. Or is that out of scope for this? Maybe something like:

return variable_get('uuid_plugin',$plugin);

3) We might also be able to move the dependency validation code for those plugins into the actual UuidInterface implementations below as static functions.

+++ b/includes/uuid.incundefined
@@ -0,0 +1,151 @@
+    if (function_exists('uuid_create') && !function_exists('uuid_make')) {
+      $plugin = 'UuidPecl';
+    }
+    // Try to use the COM implementation for Windows users.
+    elseif (function_exists('com_create_guid')) {
+      $plugin = 'UuidCom';

4) Should the UuidPhp class use uniqid()? Not sure what its benefits are. UuidPhp::generate() does look pretty solid.

5) This is awesome!

1 days to next Drupal core point release.

1) We are waiting for the entity api rearchitecture to land before we work it in there, but yes that will be the first use case.

2&3) There was a lot of discussion about this above but we decided against it.

4) No, to the best of my knowledge uniqid() does not generate standard uuids.

oops crossposted

Looks great.

1. Since the $plugin variable is always the same from Uuid object to Uuid object, we could make it static to save memory.
2. Could also move the dependencies check into the individual classes.

Could we then do something like this for the interfaces? As it stands right now, it is impossible to use anything other than PHP, PECL or the COM to generate the UUIDs. At least if we move the dependency code out, it'll help decouple it a bit.

    // Retrieve an available UUID plugin. Prefer to select the PECL or COM
    // interfaces first, if their dependencies are valid.
    // @TODO: Query the available UUID Interface implementations.
    $plugins = array('UuidPecl', 'UuidCom', 'UuidPhp');
    foreach ($plugins as $uuidplugin) {
      if ($uuidplugin::validDependencies()) {
        return $plugin = $uuidplugin;
      }
    }

/**
 * UUID implementation using the PECL extension.
 */
class UuidPecl implements UuidInterface {
  /**
   * Generate a UUID using the PECL extension.
   */
  public function generate() {
    return uuid_create(UUID_TYPE_DEFAULT);
  }

  /**
   * Checks to make sure uuid_create() and uuid_make() are available.
   */
  public static function validDependencies() {
    // Debian/Ubuntu uses the (broken) OSSP extension as their UUID
    // implementation. The OSSP implementation is not compatible with the
    // PECL functions.
    return function_exists('uuid_create') && !function_exists('uuid_make');
  }
}

A static variable in a method is shared across all instances of that object. In practice static is a global with restricted scope. (Figure THAT out. :-) ) If we really wanted to be pedantic we would move it to a static class property on the Uuid class, but I don't care enough right now to ask the patch to be rerolled for that. (If the patch does get rerolled then arguably we should switch to that.)

Rob Loach: There are no other extant compatible UUID implementations that we're aware of. I don't know that we benefit at all from having a pluggable, extensible list of implementations when there's only 3 viable implementations in the first place, and we already support all of them.

I reviewed this and it looks really good.

The only thing that is not 100% clear is why we ship with different implementations. Is it for performance or entropy reasons? I'm not sure why it _really_ matters. It could be good to clarify that with one sentence in the phpDoc.

Rerolled. Merged CHANGELOG.txt changes. Fixed a typo the determinePlugin() header doc and added an explanation of why we have multiple implementations.

The extended explanation is that the PHP userspace implementation is slow compared to doing it in C, the PECL extension only works on *nix and COM is a Windows thing. We give the user the fastest available UUID generator based on their environment.

@Crell I didn't make the static class property change as I don't think the user's environment will change between instantiations. I'll make the change if the consensus is to do it.

Forgot to fix the status

It's not that the environment would change. The net effect is the same, only one plugin object is ever created. It's just a somewhat more proper syntax. (Technically a static variable inside a method should never be used as it is just confusing and doesn't mean what you think it means, but in this case it does what we want it to anyway.)

Bah, crossposting.

I reviewed the latest version of this patch and decided to commit it to 8.x. As always, we can follow-up with incremental improvements if necessary. Right now, it is best to commit it so progress can be made with the configuration management initiative.

Nice, thanks everyone.

As a follow-up, should we implement hook_requirements to inform the user that performance can be improved by installing the PECL extension?

+++ b/includes/uuid.inc
@@ -0,0 +1,154 @@
+   * This constructor instantiates the correct UUID object.

"Constructs a UUID object."

+++ b/includes/uuid.inc
@@ -0,0 +1,154 @@
+   * Check that a string appears to be in the format of a UUID.
+   *
+   * Plugins should not implement validation, since UUIDs should be in a
+   * consistent format across all plugins.
...
+  public function isValid($uuid) {

Shouldn't this be final then?

+++ b/includes/uuid.inc
@@ -0,0 +1,154 @@
+   * @return
+   *  The class name for the optimal UUID generator.

Minor: Missing leading space (wrong indentation).

+++ b/includes/uuid.inc
@@ -0,0 +1,154 @@
+  protected function determinePlugin() {
+    static $plugin;
+    if (!empty($plugin)) {
+      return $plugin;

Why isn't this a static class property?

+++ b/includes/uuid.inc
@@ -0,0 +1,154 @@
+    $plugin = 'UuidPhp';
+
...
+    }
+    return $plugin;

Could we move the default into a final else condition for clarity/readability?

+++ b/includes/uuid.inc
@@ -0,0 +1,154 @@
+    $time_hi_and_version = base_convert(substr($hex, 12, 4), 16, 10);
+    $time_hi_and_version &= 0x0FFF;
+    $time_hi_and_version |= (4 << 12);
+
+    $clock_seq_hi_and_reserved = base_convert(substr($hex, 16, 4), 16, 10);
+    $clock_seq_hi_and_reserved &= 0x3F;
+    $clock_seq_hi_and_reserved |= 0x80;
+
+    $clock_seq_low = substr($hex, 20, 2);
+    $nodes = substr($hex, 20);
+
+    $uuid = sprintf('%s-%s-%04x-%02x%02x-%s',
+      $time_low, $time_mid,
+      $time_hi_and_version, $clock_seq_hi_and_reserved,
+      $clock_seq_low, $nodes);

These code blocks could use some more inline comments that explain what is being done and why.

-3 days to next Drupal core point release.

Here a new version of the patch with suggestions from @sun.

I've also added in modules/system/system.install a requirement to display the current uuid plugin implementation in the status report.

Subscribing.

+  static protected $plugin;
+  protected $instance;

$instance also needs a docblock.

switching tag so we're not mixing the 'needs docs' queue with other followup stuff. However if someone wants to write the change notification that could happen whenever.

Thanks for your work on the patch, Pol. :) Here's a few code style cleanup notes:

+++ includes/uuid.inc	(revision )
@@ -47,85 +48,71 @@
+   * Check that a string appears to be in the format of a UUID.

Should be "Checks whether..."

+++ includes/uuid.inc	(revision )
@@ -47,85 +48,71 @@
+   * Generate a UUID hash.

Should be "Generates a UUID hash."

+++ includes/uuid.inc	(revision )
@@ -152,3 +139,24 @@
+ * UUID implementation using the PECL extension.
...
+ * UUID implementation using the Windows internal GUID extension.

These two docblocks should begin with a third-person indicative present verb, e.g., "Creates a UUID using..."

+++ includes/uuid.inc	(revision )
@@ -152,3 +139,24 @@
\ No newline at end of file

Missing newline here at the end of the file. (Put your cursor on the last line and hit enter before rerolling.)

Note that (in addition to the changes suggested above) this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Also, be sure to set the issue to Needs Review once you submit your revised patch, so that the patch is tested by testbot and seen by reviewers.

Oh, rerolling this could be a novice task.

Rerolled the patch with suggestions from @xjm.

Thanks Pol! A couple more things here:

+++ b/core/includes/uuid.incundefined
@@ -31,14 +31,15 @@ class Uuid {
   /**
    * Holds the UUID implementation.
    */
-  protected $plugin;
+  static protected $plugin;
+  protected $instance;

I think the earlier remark about needing docblocks for these still applies as well. See the examples in the code snippet at: http://drupal.org/node/1354#classes

+++ b/core/includes/uuid.incundefined
@@ -126,6 +91,28 @@ class UuidCom implements UuidInterface {
+   * Check whether a string appears to be in the format of a UUID.

Missed one (this should be "Checks.") :)

Here we go.

There, that looks better. :)

While core may not out of the box support v3 or v5 UUIDs, the API does not cleanly handle the case where we would want them if a plugin would want to supply them. Can we at least make the API work to support them?

Do we have some follow-up issues to start using UUIds in our entities?

@moshe, this is the only one I could find: #1189942: Universal entity id?

Latest patch does not seem to apply anymore.

@mfer: I've seen your Lootils code on github. Various people would like to see UUID v5 support in core (and used by default). I'd therefore suggest to create a new issue in order to discuss whether we want to replace/improve/copy/move that library into core.

@moshe weitzman: The follow-up issue is #1637370: Add UUID support to core entity types and almost done. :)

Tagging for reroll.

This was actually a very easy manual reroll, because almost everything in the patch is already implemented in core in the directory core/lib/Drupal/Component.

As such, all that's left are these 6 lines in system.install.

Thanks for the reroll, @disasm! There were some other changes that didn't make it in though. A bit confusing because the patch in #81 was committed in #85, but then the issue was reopened for follow-ups.

I spoke to @xjm on IRC and we agreed it would be best to close this issue and create two separate follow-up issues, one for docs updates and one for code changes.

So without further ado, here are those two follow-up issues:
#1738622: Documentation cleanup for UUID API
#1738620: Display UUID generator in status, mark Uuid::isValid() final

I've also updated #1540656: [META] Entity Serialization API for web services (e.g. content staging).

Scanning through the issue again, do we need a change notification?