There is an issue with the patch committed here: http://drupal.org/node/895622#comment-4034336

In hook_nodeapi($op='update'), the $node object is not fully loaded. As a result, editing a node will result in a new UUID be created & saved for it. You can read more about $node not being fully loaded w/$op='update' starting here: http://drupal.org/update/modules/5/6#comment-1087468

The attached patch loads the node at the beginning of the "case 'update':" in hook_nodeapi().

CommentFileSizeAuthor
#13 uuid_1065364_6.patch3.05 KBryan_courtnage
#8 uuid_1065364_5.patch1005 bytesryan_courtnage
#6 uuid_1065364_4.patch860 bytesryan_courtnage
#4 uuid_1065364_3.patch1.73 KBryan_courtnage
#3 uuid_1065364_2.patch1.78 KBryan_courtnage
uuid_new_uuid_on_update.patch699 bytesryan_courtnage

Comments

Status: Needs review » Needs work

The last submitted patch, uuid_new_uuid_on_update.patch, failed testing.

q0rban’s picture

q0rban commented

subscribe

ryan_courtnage’s picture

ryan_courtnage commented
StatusFileSize
new uuid_1065364_2.patch1.78 KB

Sorry, I hadn't run the tests before uploading. Made a stupid mistake by loading into $node, as I was clobbering any changes from the node edit. I'm now loading into a new variable.

Everything seems to behave the way it should to me. All of the tests pass, expect for 3 that deal with programmatically changing the uuid when calling node_save(). I'm not sure I understand the use-case. It could be that the tests themselves should be updated. q0rban, I may need your help with this.

ryan_courtnage’s picture

ryan_courtnage commented
Status: Needs work » Needs review
StatusFileSize
new uuid_1065364_3.patch1.73 KB

A minor tweak. Still 3 fails.

Status: Needs review » Needs work

The last submitted patch, uuid_1065364_3.patch, failed testing.

ryan_courtnage’s picture

ryan_courtnage commented
Status: Needs work » Needs review
StatusFileSize
new uuid_1065364_4.patch860 bytes

Okay, I've got it now. Not sure why I didn't take this approach to begin with..

@q0rban, your tests are absolutely fantastic. Without them, one of my previous patches could have slipped through. Thank You!

q0rban’s picture

q0rban commented
Status: Needs review » Needs work

Great, I'm glad it helped, although most of the credit for those tests go to mike ryan and ezra-g I believe. I only expanded what they had. :)

I think this could be simplified to not run a complete node_load:

$uuid_information = uuid_node_load($node);
if ($uuid_information['uuid'] && empty($node->uuid)) {
  $node->uuid = $uuid_information['uuid'];
}
if ($uuid_information['revision_uuid'] && empty($node->revision_uuid)) {
  $node->revision_uuid = $uuid_information['revision_uuid'];
}
ryan_courtnage’s picture

ryan_courtnage commented
Status: Needs work » Needs review
StatusFileSize
new uuid_1065364_5.patch1005 bytes

I smacked myself on the forehead after posting that comment and then looking at the CVS history for the tests. Doh! :P Either way, great tests. Attached is a modified patch that doesn't node_load().

q0rban’s picture

q0rban commented

Looks great, Ryan! The only thing I would change is to add some commenting above it to highlight why this is necessary. Probably the description from your original comment would work well, and a reference to this issue. :)

recidive’s picture

recidive commented
Status: Needs review » Needs work

Hi @ryan_courtnage, thanks for the patch. It looks good.

Please add the comments and I'll commit it.

q0rban’s picture

q0rban commented

It would also be nice to have a test for this bug, but I'm not sure how you would even generate a test for this. It must be something that occurs only when editing a node via the UI.

ryan_courtnage’s picture

ryan_courtnage commented

I was thinking about this myself. I imagine that submitting the node edit form with a drupalPost would suffice. I'll try some things this evening.

ryan_courtnage’s picture

ryan_courtnage commented
Status: Needs work » Needs review
StatusFileSize
new uuid_1065364_6.patch3.05 KB

Okay, here it is with the requested comments, as well as a web test that will fail nicely without the change, and will pass with it.

recidive’s picture

recidive commented
Status: Needs review » Fixed

Committed! Thanks!

Status: Fixed » Closed (fixed)

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

aschiwi’s picture

aschiwi commented
Status: Closed (fixed) » Active

I have a question about this. The patch works, but I'm wondering why I get uuid_node_revision entries in the first place. I have automatic uuid creation turned off for all content types, I'm only using it to get terms into a site using a Feature. After running update #6005 I now have 41,883 entries in the uuid_node_revision table. I also have entries in uuid_node, but I think I shouldn't have entries in either of these tables?

apotek’s picture

apotek commented
Status: Active » Closed (fixed)

@aschiwi Please create a new issue for the problem you are having above. They may well be related, but this issue, and the patch that was created for it, have solved *this* issue, and it's better if we close this and open another one for the issue you have identified. Keeps things much cleaner that way.