Issue status update for http://drupal.org/node/24336 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Jaza Reported by: Jaza Updated by: Jaza -Status: active +Status: patch Attachment: http://drupal.org/files/issues/taxonomy.module.revision_delete_fix.patch (493 bytes) Thanks for the clarification, Moshe. I wasn't sure whether or not I was taking the right approach. Basically, I saw two ways to look at it: 1. when node_save() is called and is pased an existing node, it relies on the assumption that the current path is node/x/edit. Therefore, it is the responsibility of node_save() to check that this is true before invoking hook_nodeapi(). 2. when hook_nodeapi() is called with the 'update' value, it makes no guarantee that the current path is node/x/edit. Therefore, if any implementations of hook_nodeapi() rely on this assumption, they should make the check themselves, since they may be called by functions such as node_save(). It was really just a matter of interpretation (and guesswork): if node_save() makes an assumption, then it needs to make the check; but if it makes no assumption, then functions that may be called by it (through hook_nodeapi()) need to make the check. I decided that option 1 sounded best, and so I put the check in node_save(). The attached patch moves the check to taxonomy_nodeapi(). Could you clarify the issue further, please? Should all implementations of hook_nodeapi implement a similar check, if they rely on the path being node/x/edit? I think I understand the reasons for this design decision now, but not entirely sure. Thanks. Jaza Previous comments: ------------------------------------------------------------------------ June 5, 2005 - 22:54 : Jaza Attachment: http://drupal.org/files/issues/node.module.revision_delete_fix.patch (595 bytes) This bug is caused by the revisions for a node being managed on the node/x/revisions page, rather than the node/x/edit page. When a revision is deleted, it calls node_save(). But node_save() assumes that it is being called from the node editing page. It grabs all the submitted values for a node, and then invokes the _nodeapi() (and _update()) hook to see what extra values it needs to grab. Over in taxonomy.module, taxonomy_nodeapi() calls taxonomy_node_save(), which tries to grab the taxonomy mappings for the node. But since none were submitted (because it's not the node editing form), it clears the old values, and then inserts the newly submitted values (i.e. NOTHING) into the database. This bug addresses the issue, by only invoking the hooks if the previous page was node/x/edit. I don't know how the core node fields manage to get saved, since they're not submitted either. But anyway, the main thing is that core fields don't seem to be affected by this bug. And with the patch, taxonomy mappings aren't affected either. This patch probably addresses the same problem for fields that other modules define through _nodeapi() - haven't checked this, though. ------------------------------------------------------------------------ June 6, 2005 - 03:51 : moshe weitzman please don't add special cases for a particular page. fix the calling code instead. in this case, fix node/x/revisions or, less likely, fix taxonomy.

Issue status update for http://drupal.org/node/24336 Project: Drupal Version: cvs Component: taxonomy.module Category: bug reports Priority: normal Assigned to: Jaza Reported by: Jaza Updated by: cidenton Status: patch I've been running into this problem as well, in a relational flexinode field I am writing. I would turn the problem statement on its head - the problem is not that taxonomy_node_save() is improperly trying to save the taxonomy mappings for the node, but that taxonomy.module does not use hook_load() or hook_nodeapi() to put them on the node when it is loaded. Should that be the target of the patch? In general it seems like taxonomy.module needs a fuller hook implementation. Its hook_nodeapi implementation has the built-in assumption that someone else is doing its work at load and form time. It seems awkward that every node module has to include taxonomy-specific checks in its hook_form() implementation: if (function_exists('taxonomy_node_form')) { $output .= implode('', taxonomy_node_form('page', $node)); } I am very new to Drupal; is there an architectural reason that taxonomy.module does not have a more complete hook_nodeapi() implementation, or is it historical? Regards, Claude cidenton Previous comments: ------------------------------------------------------------------------ June 5, 2005 - 08:54 : Jaza Attachment: http://drupal.org/files/issues/node.module.revision_delete_fix.patch (595 bytes) This bug is caused by the revisions for a node being managed on the node/x/revisions page, rather than the node/x/edit page. When a revision is deleted, it calls node_save(). But node_save() assumes that it is being called from the node editing page. It grabs all the submitted values for a node, and then invokes the _nodeapi() (and _update()) hook to see what extra values it needs to grab. Over in taxonomy.module, taxonomy_nodeapi() calls taxonomy_node_save(), which tries to grab the taxonomy mappings for the node. But since none were submitted (because it's not the node editing form), it clears the old values, and then inserts the newly submitted values (i.e. NOTHING) into the database. This bug addresses the issue, by only invoking the hooks if the previous page was node/x/edit. I don't know how the core node fields manage to get saved, since they're not submitted either. But anyway, the main thing is that core fields don't seem to be affected by this bug. And with the patch, taxonomy mappings aren't affected either. This patch probably addresses the same problem for fields that other modules define through _nodeapi() - haven't checked this, though. ------------------------------------------------------------------------ June 5, 2005 - 13:51 : moshe weitzman please don't add special cases for a particular page. fix the calling code instead. in this case, fix node/x/revisions or, less likely, fix taxonomy. ------------------------------------------------------------------------ June 5, 2005 - 21:47 : Jaza Attachment: http://drupal.org/files/issues/taxonomy.module.revision_delete_fix.patch (493 bytes) Thanks for the clarification, Moshe. I wasn't sure whether or not I was taking the right approach. Basically, I saw two ways to look at it: 1. when node_save() is called and is pased an existing node, it relies on the assumption that the current path is node/x/edit. Therefore, it is the responsibility of node_save() to check that this is true before invoking hook_nodeapi(). 2. when hook_nodeapi() is called with the 'update' value, it makes no guarantee that the current path is node/x/edit. Therefore, if any implementations of hook_nodeapi() rely on this assumption, they should make the check themselves, since they may be called by functions such as node_save(). It was really just a matter of interpretation (and guesswork): if node_save() makes an assumption, then it needs to make the check; but if it makes no assumption, then functions that may be called by it (through hook_nodeapi()) need to make the check. I decided that option 1 sounded best, and so I put the check in node_save(). The attached patch moves the check to taxonomy_nodeapi(). Could you clarify the issue further, please? Should all implementations of hook_nodeapi implement a similar check, if they rely on the path being node/x/edit? I think I understand the reasons for this design decision now, but not entirely sure. Thanks. ------------------------------------------------------------------------ June 21, 2005 - 14:40 : Dries Patch still looks like a special case to me. Encoding the URL is hairy. The blogapi.module has the same problem so we really need a generic fix here.