API functions

This is a good and useful patch, it should really go in. I've rolled a new version with a modification. All table and field names are now escaped with db_escape_table() to ensure they are safe to be embedded in the SQL queries. While these are not user-supplied values, this is still a good precaution against accidental future SQL injection vulnerabilities.

I also added some code to uuid_set_uuid() which checks to see if the $serial_id actually exists in the base table. This prevents accidentally orphaned records from being inserted into the _uuid table.

Patch looks good, thanks!

I'm just wondering if there are places in UUID module we can change to use those new set of functions.

Also, aren't there a better name to use instead of "serial id"?

That said, I support the patch, I just want to make sure we get that in right.

I'm sure there are places throughout UUID that could benefit from this, not sure if you want to do them in another patch or not though.

I'm pretty neutral towards the name "serial id", I think its fine.

Yes, if there are places that benefit from this, it should be made in the same patch as code cleanup.

Any news on this? I'd like to commit this patch before rolling out a stable release.

Thanks!

I have a mostly finished patch that rolls this functionality throughout UUID, however I have been finding it difficult to find the time to finish and test it. If I can't get to it this weekend I'll post what I have and maybe someone else can finish it off.

Sounds great, thanks!

I tried to contact heyrocker by irc but I didn't remember that he changed his tz, and I've been doing this too late in the day, so I decided to move on this by my own. I've done some advances in heyrocker's approach, but I guess we will need additional changes, let me explain.

I've change API signatures and 1st argument to 'bundle', because this argument is not the table name at all. So now $bundle replaces $table, and bundle is any of 'node', 'users', 'node_revisions', 'comments' and so, and bundle id is 'nid', 'vid', 'cid' for their respective tables.

I've changed API function names to reflect that and conform a more browseable API now.
uuid_uuid_get(): get the UUID of an object based on its Bundle ID.
uuid_uuid_get('node', 'nid', $node->nid) will get uuid of this node nid.

uuid_uuid_set(): set the UUID of an object based on its Bundle ID.
uuid_uuid_set('node', 'nid', $node->nid, $node->uuid) will set uuid for this node nid.

I've included the delete API function:
uuid_uuid_delete(): delete the UUID of an object based on its Bundle ID.
uuid_uuid_delete('node', 'nid', $node->nid) will delete the entry of this node nid.

Now serial_id no longer exists and has been replaced with $id or 'bundle', so the updated functions are:

uuid_bundle_id_get(): get the bundle ID of an object based on its UUID.
uuid_bundle_id_get('node', $node->uuid) will return $node->nid.
uuid_bundle_id_get('node_revisions', $node->revision_uuid) will return $node->vid.

uuid_bundle_id_delete(): delete the bundle ID of an object based on its UUID.
uuid_bundle_id_delete('node', $node->uuid) will delete the entry of this uuid.

With this new API we can go for #935998: Normalize the UUIDs into one table also.. but..

I've gone through the code and do all replacements I was able to do, but I've a found a pair of cases that this API approach is unable to cover:

Problem: All bundles (now, previously all tables) are keyed by a single element, node -> nid, node_revisions -> vid, and so on.. so now it is not possible to remove node_revisions using node->nid, because the API is only using vid to track the uuid value. The issue #935998: Normalize the UUIDs into one table is also affected by this issue, but using the keyed array approach we can have more than just one bundle ID column.

Solution: use an array of keyed elements as filter and values instead of single elements. e.g. the following call will use more than one key to set and get the uuid value for the node_revisions:

  /*
   * Saving an uuid
   */ 

  // Previous API approach.
  uuid_uuid_set('node_revisions', 'vid', $node->vid, $node->uuid);

  // New approach using keyed arrays:
  uuid_uuid_set('node_revisions', 
    array(
      'nid' => $node->nid,
      'vid' => $node->vid,
    ),
    $node->uuid
  );


  /*
   * Loading an uuid
   */ 

  // Previous API approach.
  uuid_uuid_get('node_revisions', 'vid', $node->vid);

  // New approach using keyed arrays:
  uuid_uuid_get('node_revisions', 
    array(
      'nid' => $node->nid,
      'vid' => $node->vid,
    )
  );


  /*
   * Deleting a single revision uuid
   */ 

  // Previous API approach.
  uuid_uuid_delete('node_revisions', 'vid', $node->vid);

  // New approach using keyed arrays:
  uuid_uuid_delete('node_revisions', 
    array(
      'vid' => $node->vid,
    )
  );


  /*
   * Deleting a node, so removing all revisions.
   */ 

  // Previous API approach.
  // There is no way, you are only able to delete the latest node's vid entry.
  uuid_uuid_delete('node_revisions', 'vid', $node->vid);

  // New approach using keyed arrays:
  uuid_uuid_delete('node_revisions', 
    array(
      'nid' => $node->nid,
    )
  );

The new API should get the keyed array and replace all those fields with AND for SELECT and DELETE queries, and VALUES in INSERT.

The other problem I've face is that taxonomy 'bundle' looks a little bit too complex for the first sight, so I need a second pass for this to be changed using the previous API approach.

Coments on the new API approach? do you think we need to change this and use keyed arrays?

The patch I've uploaded is still using the old approach, I just made some advances to heyrocker's work and included the delete api functions. I haven't fixed any error in the code nor logic, and the functionality testcase submitted to the simpletest issue returns the same number of errors as the current CVS code.

After a quick review of includes/cache.inc I think we can get this working (and event extended) the same way Drupal's cache does, by using a combined $id when required (the case of nodes is the first I can see) with key = '$node->nid:$node->vid'. This way the argument does not need to be an array, and we still do keep track of the node's nid when working with revisions. To handle the situation when all entries of the same 'nid' should be deleted, it would be as simple as mimic the way cache delete works using a LIKE condition for the $cid, and every looks perfect to get a single uuid tracking table ready.

Do you think it is the way to go?

I think it's fine for the functions to accept both scalar (string/numeric, one key) or a keyed array (more than one key). This will be similar to drupal_write_record() third parameter $update.

+ * Base bundle of the object. Currently, one of node, revision_revisions,

I think you mean node_revisions instead of revision_revisions. Please fix this in all API functions.

oh, your are right, recidive, they were inherited fomr the previous patch. I'll changed them, but I think it is important to choose which is the best approach:

Accepting arrays as input means that query can have different number of fields, and this might difficult the step of going to a single database that must have all these fields (I'm not sure how many of them would be required or minimum for now). Going the 'cache' style and using a unique identifier is my pref. option, that way "node:12:17" or "user:1" can lately become the uuid's bundle ID, and that is enough to clearly identify the relation 'fields' (even more than one): a) DELETE FROM {uuid} WHERE bid LIKE 'mycustommodule:%' or b) DELETE FROM {uuid} WHERE bid LIKE 'node:12:%' are two ussage examples of how to remove several entries in a query, a) all uuids from a custom module, and b) all uuids for the node->nid = 12.

Recidive, lets make room for other opinions also, I'm sure there are a lot of great people around this issue queue that would have something to point about these two approaches.

subscribed

In D7 bundles are sub types of entities, where as in @ilo's proposal it seems that bundle is being used to reference an entity. I think we should try to keep the language around entities and bundles consistent across core and contrib - even in D6.

function in patch no longer exists uuid_uuid()

this is not documented anywhere, except in other module issue queues?

Drupal 6 core is no longer supported. We are no longer supporting 6.x-1.x versions of this module. I am closing this issue as won't fix.