[drupal-devel] [bug] Deleting revision also deletes a node's taxonomy mappings

[cidenton]

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.