Implementation of UUID for taxonomy

Hi, thanks for your patch.

Unfortunately, I think your patch was taken against old HEAD code. I've just synced HEAD with DRUPAL-6 branch.

Some of your changes already exist in DRUPAL-6 (and now in HEAD too) like the ability to set which node types should have UUID auto-generated.

The uuid_node.inc and uuid_user.inc don't exist since long time ago.

Can you elaborate on your one-table table approach to store UUIDs for all entities? What it buy us?

I'd appreciate if you cold split your patch by functionality, since there are other issues for some of what you are proposing here, see #502622: UUID() is a non-deterministic function and breaks with replication and #324712: Support Token module. You can concentrate this issue maybe in the database stuff. This is not critical though, since I can as well commit your entire patch. But things like UUID generation need more thought IMO, see discussion in that issue.

Please make another patch so I can review better.

Thanks again.

-  $types = node_get_types('names');
-  $form['content']['uuid_automatic_for_nodes'] = array(
-    '#type' => 'checkboxes',
-    '#title' => t('Content Types With Automatic UUIDs'),
-    '#default_value' => variable_get('uuid_automatic_for_nodes', array()),
-    '#options' => $types,
-    '#description' => t('Selected content types to have UUIDs automatically generated.'),
-    '#required' => FALSE,
-  );
+  
+  if ($options = node_get_types('names')) {
+    $form['node']['uuid_automatic_for_nodes'] = array(
+      '#type' => 'checkboxes',
+      '#title' => t('Content types with automatic UUIDs'),
+      '#default_value' => variable_get('uuid_automatic_for_nodes', array()),
+      '#options' => $options,
+      '#description' => t('Selected content types to have UUIDs automatically generated.'),
+      '#required' => FALSE,
+    );
+  } else {
+    $form['node']['uuid_automatic_for_nodes'] = array(
+      '#type' => 'item',
+      '#value' => t('There is currently no content type defined.'),
+    );
+  }

Your changes above does essentially the same as the current code, you can strip that out.

-    '#title' => t('Automatic UUIDs For Users?'),
+    '#title' => t('Automatic UUIDs for users?'),

Please avoid that kind of changes as they do nothing besides making it hard to read the patch.

+  $form['taxonomy'] = array(
     '#type' => 'fieldset',
-    '#title' => t('General Settings'),
-  );
-  $form['settings']['sync'] = array(
-    '#type' => 'submit',
-    '#value' => t('Create Missing UUIDs'),
-    '#submit' => array('uuid_sync'),
-    '#weight' => 10,
-  );
-  $form['settings']['rebuild_help'] = array(
-    '#value' => '<div class="description">' . t("If UUID settings have changed recently some content or users may be missing their UUIDs.") . '</div>',
-    '#weight' => 11,
+    '#title' => t('Taxonomy UUID Settings'),
   );

You are removing important code here, that's probably not what you want.

The uuid_sync() is too much hardcoded for nodes, users and taxonomy, maybe it's time to make this a pluggable system by adding some hooks for other modules to extend functionality.

 /**
  * Implementation of hook_uninstall().
  */
 function uuid_uninstall() {
-  variable_del('uuid_automatic_for_nodes');
-  variable_del('uuid_automatic_for_users');
-
-  // Remove tables.
   drupal_uninstall_schema('uuid');
 }

You are removing code that delete module setting on unistallation instead of adding code to remove your new settings.

Can you explain why your 1-table-only approach for storing UUIDs is better than a table for each entity? This can have performance impact e.g. when you have 1M users and 1M nodes, the query will need to search 2M items instead of 1M.

Also, it would help if you can remove the UUID generation stuff from your patch as this is being developed in the other issue.

Tip, taking a patch is not just a matter of replacing module files with your files and taking the patch, what you need to do instead is to take a patch against the code you used as base for doing your changes, and selectively, manually apply to the newer code, then take a patch from it. Also a patch that touches every part of the code is less likely to get commited, since one can agree with part of your code only, and the other part will hold this back. Try to split your patches by functionality. I know this can be tedious, but it's a good way to learn how to make good patches.

Thanks for you contributions!

Hello, patch looks ok.

I'm just a bit concerned about using %s placeholder in table and column names in those two queries:

+  if (array_key_exists($type, $types) && in_array($array['vid'], variable_get('uuid_automatic_for_taxonomy', array()))) {
+    switch ($op) {
+      case 'insert':
+        $uuid = (isset($array['uuid']) && uuid_is_valid($array['uuid'])) ? $array['uuid'] : uuid_uuid();
+        db_query("INSERT INTO {%s} (%s, uuid) VALUES (%d, '%s')",
+          $types[$type]['table'], $types[$type]['colname'], $array[$types[$type]['colname']], $uuid);
+        break;
+      case 'delete':
+        db_query('DELETE FROM {%s} WHERE %s = %d',
+          $types[$type]['table'], $types[$type]['colname'], $array[$types[$type]['colname']]);
+        break;
+    }
+  }

I think it's safer to just concatenate table name/column name instead e.g. changing from {%s} to {' . $types[$type]['table'] . '}.

Also, don't you think 'colname' should be renamed to 'field' just to be inline with schema?

Hi, the patch looks good.

It's missing update function to create the uuid_vocabulary and uuid_term_data tables.

I'd like to commit this but don't have time to test this now.

I'm moving this to needs review in the hope to get some test.

@B-Prod, can you work with some one else to test this for us?

Thank you.

What about views argument support? Will this be ever implemented?

It's a bit sad, that this patch never was commited.

@ayalon can you test patch in #10 and see if it still applies and work as expected? If so, please move issue status to RTBC and I'll do a final review and commit.

Thanks.

Ok here are my test results:

1. Applied patch without an error to the latest version
2. Activated UUID for a certain vocabulary
3. Saved the settings
4. Updated missing entries
5. Checked the db for UUID -> Everything is there as expected, tables created, entries created

Now I wanted to deploy these UUID with the deploy module
1. Activated the module on the deploy server
2. Deployed a whole vocabulary
3. Checked the term UUID and the vocabulary UUID and -> HURRAY! They were the same!

My vote for commiting this patch!

Commited to Drupal 6 dev branch. Thanks!

In addition to this patch I also made a views integration:

It's now possible to pass a taxonomy UUID and a vocabulary UUID as a parameter.

It's also possible to sort and to display the values of taxonomy uuid and vocabulary uuid.

@ayalon: could you open a new issue for your patch in #15?

Thanks!