Here is a first baby step towards UUID support in core. This patch adds uuid.inc with tests for validation and generation of UUIDs.

We are intentionally waiting for the Entity API to be completed before we make use of this API in core.

Everything is lazy-loadable by providing a factory class that takes care of generating and validating UUIDs. The implementation used for generating UUIDs is pluggable through an interface, thus also lazy-loadable.

For now the implementation is auto-detected. It first checks for an available C implementation with fallback on a PHP implementation. We agreed on intentionally avoiding a variable_get()-type of pluggablity while waiting for a real plugin system in core. It will probably be a small task to convert it later. This decision was based on:

* We can then use real unit tests (DrupalUnitTestCase)
* The three implementation that it auto-detects on will cover 99.9% of all use-cases

Most of the code and tests are taken from the reboot branch of the UUID module, that dixon_ been working on, with help from various people including skwashd, davereid, heyrocker and more.

Files: 
CommentFileSizeAuthor
#108 drupal-uuid_in_core-1252486-97.patch582 bytesdisasm
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#97 0001-125486-Low-level-UUID-API-in-core.patch5.45 KBPol
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#94 0002-125486-Low-level-UUID-API-in-core.patch5.4 KBPol
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#88 1252486-uuid-inc-88.patch5.36 KBPol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1252486-uuid-inc-88.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#81 1252486-uuid-inc-81.patch7.15 KBskwashd
PASSED: [[SimpleTest]]: [MySQL] 32,919 pass(es). View
#75 uuid.inc_.txt4.39 KBRobLoach
#70 uuid-1252486-68.patch6.53 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 32,915 pass(es). View
#68 uuid-1252486-68.patch0 bytesdixon_
PASSED: [[SimpleTest]]: [MySQL] 32,890 pass(es). View
#64 uuid-1252486-64.patch6.52 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es). View
#55 1252486-uuid-inc-55.patch7.29 KBskwashd
PASSED: [[SimpleTest]]: [MySQL] 32,865 pass(es). View
#47 1252486-uuid-inc-47.patch5.52 KBskwashd
FAILED: [[SimpleTest]]: [MySQL] 32,856 pass(es), 0 fail(s), and 1 exception(es). View
#45 1252486-uuid-inc.patch4.99 KBskwashd
PASSED: [[SimpleTest]]: [MySQL] 32,857 pass(es). View
#41 1252486-uuid-inc-5.patch4.4 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 32,864 pass(es). View
#40 1252486-uuid-inc-5.patch4.39 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 33,229 pass(es). View
#38 1252486-uuid-inc-4.patch4.59 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 33,224 pass(es). View
#25 1252486-uuid-inc-3.patch4.24 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 33,234 pass(es). View
#20 1252486-uuid-inc-2.patch4.34 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 33,647 pass(es). View
#6 1252486-uuid-inc-1.patch4.4 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 33,645 pass(es). View
uuid-inc.patch3.78 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 33,649 pass(es). View

Comments

dixon_’s picture

Issue tags: +Configuration system

Adding CMI tag.

TwoD’s picture

Status: Needs review » Needs work
+++ 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.

heyrocker’s picture

Subscribe

lsmith77’s picture

Status: Needs work » Needs review

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

moshe weitzman’s picture

Should use DrupalUnitTestCase, no?

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

dixon_’s picture

FileSize
4.4 KB
PASSED: [[SimpleTest]]: [MySQL] 33,645 pass(es). View

Here is a new patch with extended tests and improved docs from TwoD's review. Now also using DrupalUnitTestCase as moshe pointed out.

@lsmith77: We'll see where we end up. But for now, it seems a bit overkill to add a dedicated library for these three functions.

dixon_’s picture

Moshe, regarding the one implementation you'd like to see. I discussed this with heyrocker, and we said that we're better off waiting for Entity API to be completed and entity_save() making it into core.

But that said, it's a very small patch to add UUID's and some tests to e.g. node.module if we want to have something working while waiting for Entity API.

Dave Reid’s picture

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';

jherencia’s picture

Sub...

Crell’s picture

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.

Dave Reid’s picture

@Crell Yeah that absolutely makes sense.

dixon_’s picture

So, should we strip out the conditional PECL/PHP callback check then, and go with PHP as default and make uuid.inc swappable, as Dave mentioned?

Crell’s picture

#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. :-)

Dave Reid’s picture

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.

lsmith77’s picture

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.

heyrocker’s picture

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.

lsmith77’s picture

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/

moshe weitzman’s picture

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.
skwashd’s picture

Status: Needs review » Needs work

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.

dixon_’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
PASSED: [[SimpleTest]]: [MySQL] 33,647 pass(es). View

Here is a new version that should meet all reviews up to now.

I've stripped out drupal_static(), added check for OSSP clash, added a COM implementation for Windows users and updated the tests to not add assertUuid() but to use a simpler more straight forward assertion.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Code and tests look good to me.

catch’s picture

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.

Crell’s picture

Status: Reviewed & tested by the community » Needs work

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.

fabsor’s picture

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?

dixon_’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
PASSED: [[SimpleTest]]: [MySQL] 33,234 pass(es). View

Although I agree with fabsor that it might be overkill to use a class, we also need to adopt more lazy-loading in core, as said above. And if creating a class is what we need to do, then let's do it.

So, to keep it simple for now, and to move forward, I've just wrapped the functions in a class.

Later, when we have a plugin framework in core, it probably makes sense to make this a proper interface since we have three implementations of a possible UUID interface: PECL, COM, PHP.

But let's keep it simple for now to move forward. This patch has the best of both worlds -- it's dead simple and Drupal is lazy-loading the class.

catch’s picture

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?

skwashd’s picture

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

Damien Tournoud’s picture

Status: Needs review » Needs work

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.

jcisio’s picture

Status: Needs work » Needs review

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?

catch’s picture

Status: Needs review » Needs work

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?

g.oechsler’s picture

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

Damien Tournoud’s picture

Status: Needs work » Needs review

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.

jcisio’s picture

Status: Needs review » Needs work

@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.

dixon_’s picture

Status: Needs work » Needs review

I'll come back with a "proper" patch very soon.

Damien Tournoud’s picture

#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.

skwashd’s picture

@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.

Damien Tournoud’s picture

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.

dixon_’s picture

FileSize
4.59 KB
PASSED: [[SimpleTest]]: [MySQL] 33,224 pass(es). View

Ok, so here is a more proper patch that implements a very simple pluggable pattern, that somewhat was discussed by Crell here #1239644: Define basic conventions for converting things into classes.

The basics is this: In uuid.inc we have an interface, a factory class and three very basic implementations of that interface. The factory class decides what implementation to use based on variable_get('uuid_class').

So, to sum up. This is very similar to how the cache "plugin" works, except that the factory is a class instead of procedural functions. This way we can lazy-load the factory class too.

catch’s picture

Status: Needs review » Needs work
+++ 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.

dixon_’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
PASSED: [[SimpleTest]]: [MySQL] 33,229 pass(es). View
heyrocker’s picture

Issue tags: -Configuration system
FileSize
4.4 KB
PASSED: [[SimpleTest]]: [MySQL] 32,864 pass(es). View

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.

tstoeckler’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system

an ID, but a UUID.

Damien Tournoud’s picture

+    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
skwashd’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
PASSED: [[SimpleTest]]: [MySQL] 32,857 pass(es). View

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.

tstoeckler’s picture

Status: Needs review » Needs work
+++ 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.".

skwashd’s picture

Status: Needs work » Needs review
FileSize
5.52 KB
FAILED: [[SimpleTest]]: [MySQL] 32,856 pass(es), 0 fail(s), and 1 exception(es). View

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.

tstoeckler’s picture

Status: Needs review » Needs work
+++ 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).

jcisio’s picture

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]).

Crell’s picture

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. :-)

skwashd’s picture

@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.

tstoeckler’s picture

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.

catch’s picture

Issue tags: +Needs change record

Also this deserves a changelog.txt entry.

dixon_’s picture

The only way to make it pluggable with other implementations other than those three in core is to use variable_get(). But if we are happy with that, we can of course use a switch statement again. But then, the system isn't really "pluggable" in it's real sense... IMO, it's more important to make is pluggable "for real", isn't it?

When we moved to variable_get() we also had to move to DrupalWebTestCase, since variable_get() depends on the database. This is why the tests are breaking in #47.

skwashd’s picture

FileSize
7.29 KB
PASSED: [[SimpleTest]]: [MySQL] 32,865 pass(es). View

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

Damien Tournoud’s picture

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.

Damien Tournoud’s picture

Status: Needs work » Needs review
dixon_’s picture

I think the patch in #55 is very good for now. Let's move forward? :)

Avoiding variable_get() makes sense for now, because:

  • We will have a better plugin framework in place later
  • We can use DrupalUnitTestCase
  • Those three implementations in core will cover 99% of all use cases needed
Pol’s picture

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 ?

dixon_’s picture

Relying on the database to generate UUID doesn't make sense when it's both faster and less hassle to generate it in C code with PHP fallback.

Pol’s picture

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

catch’s picture

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.

catch’s picture

Status: Needs review » Needs work
+++ 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.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
6.52 KB
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es). View

Rerolled per #63

Crell’s picture

Status: Needs review » Needs work
+++ 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

catch’s picture

also " compatiable"

RobLoach’s picture

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.

dixon_’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 32,890 pass(es). View

@Rob Loach We just agreed on skipping variable_get() in wait for a real plugin system. And uniqid() doesn't generate real UUIDs, based on the UUID standard.

Here is a patch that fixed the spelling mistakes in that comment, noted by Crell and catch.

dixon_’s picture

Status: Needs work » Needs review
dixon_’s picture

FileSize
6.53 KB
PASSED: [[SimpleTest]]: [MySQL] 32,915 pass(es). View

Hmm, that was not a very useful patch... Here's a new one.

heyrocker’s picture

Status: Needs review » Needs work

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.

heyrocker’s picture

Status: Needs work » Needs review

oops crossposted

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks great.

dixon_’s picture

I updated the issue summary to reflect our recent discussion.

RobLoach’s picture

FileSize
4.39 KB
  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.
dixon_’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/uuid.inc
@@ -0,0 +1,151 @@
+  protected function determinePlugin() {
+    static $plugin;
+    if (!empty($plugin)) {
+      return $plugin;
+    }

The $plugin detection is already statically cached in determinePlugin().

1 days to next Drupal core point release.

dixon_’s picture

Status: Needs work » Reviewed & tested by the community

Ops.

RobLoach’s picture

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');
  }
}
Crell’s picture

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.

Dries’s picture

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.

skwashd’s picture

FileSize
7.15 KB
PASSED: [[SimpleTest]]: [MySQL] 32,919 pass(es). View

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.

skwashd’s picture

Status: Reviewed & tested by the community » Needs review

Forgot to fix the status

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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.)

Crell’s picture

Status: Reviewed & tested by the community » Needs review

Bah, crossposting.

Dries’s picture

Status: Needs review » Fixed

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.

marcvangend’s picture

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?

sun’s picture

Status: Fixed » Needs work
+++ 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.

Pol’s picture

FileSize
5.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1252486-uuid-inc-88.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

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.

colan’s picture

Subscribing.

catch’s picture

+  static protected $plugin;
+  protected $instance;

$instance also needs a docblock.

catch’s picture

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.

xjm’s picture

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.

xjm’s picture

Issue tags: +Novice

Oh, rerolling this could be a novice task.

Pol’s picture

FileSize
5.4 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Rerolled the patch with suggestions from @xjm.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Novice
xjm’s picture

Status: Needs review » Needs work

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.") :)

Pol’s picture

FileSize
5.45 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Here we go.

xjm’s picture

Status: Needs work » Needs review

There, that looks better. :)

Status: Needs review » Needs work

The last submitted patch, 0001-125486-Low-level-UUID-API-in-core.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
mfer’s picture

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?

moshe weitzman’s picture

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

amateescu’s picture

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

dixon_’s picture

Assigned: dixon_ » Unassigned

This issue is now a part of a meta issue, to track follow ups on the UUID implementation and some other things: #1540656: [META] Entity Serialization API for web services (e.g. content staging)

sun’s picture

Status: Needs review » Needs work

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. :)

sun’s picture

Issue tags: +UUID
Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

disasm’s picture

Status: Needs work » Needs review
FileSize
582 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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.

Status: Needs review » Needs work

The last submitted patch, drupal-uuid_in_core-1252486-97.patch, failed testing.

Cottser’s picture

Status: Needs work » Fixed
Issue tags: -Needs reroll

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).

Cottser’s picture

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

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.