Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I submit this patch strictly from a recent experience I had when I was trying to make use of the $reset parameter. In my case, I was writing an import script where lines of CSV were being parsed from a file. Each line of CSV had the potential to alter the taxonomy of an existing node. With the data we were getting for this import script, multiple lines in the data were adding to the taxonomy of an existing node. Keeping this in mind, I would call node_load($nid, NULL, TRUE), where the 'TRUE' in the call was the $reset parameter (this loaded node would be saved at the end of the loop). Unfortunately, I discovered, the node_invoke_nodeapi() and node_invoke() functions did not pass the $reset parameter to the node hooks. In my case, this was a problem, because taxonomy_nodeapi() relies on (in a helper function) a php static array when loading the taxonomy for a node. Of course, if the taxonomy of a node has changed during the execution of a script, a later loading would only return the first array of taxonomy terms loaded for the node, in spite of the $reset parameter being set to TRUE.
Of course, for this patch to be useful, other core modules will need to make use of the $revision and $reset parameters in their node hooks. I am willing to submit patches to this end if this one is accepted.
I also argue that this is the way to go just to make the loading function consistent across the hooks.
-Ankur
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | node_reset.patch | 2.34 KB | R.Muilwijk |
| #5 | node_load_7.patch | 2.32 KB | ankur |
| node.module_51.patch | 962 bytes | ankur | |
Comments
Comment #1
ankur CreditAttribution: ankur commentedAny thoughts on this? I'm just wondering what might be the point of having a $reset paramter to node_load() if that parameter doesn't get passed via node_invoke() and node_invoke_nodeapi(). The point of resetting when loading a node is that things in the node have changed within the lifetime of a single running of a script (or page serve). Currently, the $reset parameter to node_load() only resets the static $nodes array within node_load().
There would be much more consistency (and usefulness for a $reset param) if the hook_load()s and the hook_nodeapi()s (for $op == 'load') were able to receive their own reset parameter and became consistent w/ the node_load() by maintaining their own static arrays that respect the $reset parameter.... and the $revision parameter (which I haven't mentioned at all in this comment).
-Ankur
Comment #2
StevenPatzComment #3
ankur CreditAttribution: ankur commentedActually, if you look at the code for node_load(), passing the $revision param thru to the hooks is not necessary because, by the time to node_invoke() and node_invoke_nodeapi() happen, $node->vid will be set. So, patch needs to be rewritten to only pass along the $reset parameter.
-Ankur
Comment #4
drummYep, don't need $revision.
The code for handling this in the taxonomy module would be a good addition to the patch.
Comment #5
ankur CreditAttribution: ankur commentedHere's an updated patch that removes the redundant revision parameter and also includes the changes to the taxonomy module.
-Ankur
Comment #6
catchThis still applies with a big offset, bumping to the D7 queue.
Comment #7
ankur CreditAttribution: ankur commentedWhat?! I figured this patch would've seen more review/comments and gotten more consideration, seeing as to how the original revision of this patch was submitted just two weeks after the Drupal 5 release. Oh well. I guess there aren't that many people out there that are writing import scripts.
This patch is nothing major and is pretty straight forward. Maybe it's one of those petty things oriented on small details that gets swept under the rug in favor of things more worthy of a press-release. Oh well.
-Ankur
Comment #8
StevenPatzhardly critical.
Comment #9
R.Muilwijk CreditAttribution: R.Muilwijk commentedIn your patch you reset the complete static. I don't think this should be done cause the reset is always called with a node and there for you know what node to delete.
I made up a new patch which only resets that node. Also I ran the tests which didn't give me any problems.
Comment #10
ankur CreditAttribution: ankur commentedI think it's a good idea myself to only reset the internal cache for the nid on which the 'load' op is being called. However, if we're going to have that convention here, then we should probably also make it consistent with what's going on in the node_load() function defined in node.module. That function also maintains a static variable that is an array of cached nodes. Currently, that array is entirely reset everytime node_load() is called with a the reset parameter set to true.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #12
ankur CreditAttribution: ankur commentedComment #13
marcingy CreditAttribution: marcingy commentedThis is a task in my opinion, moving to d8.
Comment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedWonder if this is a valid task for node in D10?