The wrong $types parameter is sometimes passed in to hook_node_load() [#884948]

I am working on a D7 module that implements http://api.drupal.org/api/function/hook_node_load/7 and has code that checks the $types parameter to that hook (and only runs for particular node types).

I found that on some pages of the site, the code ran, but others it didn't.

I tracked it down to the way the node module builds up the arguments for this hook. Instead of always ensuring that the $types argument is set to the correct types for this particular node_load_multiple() call, it instead keeps adding new parameters to the hook invocation. So if multiple node load calls happen on the same page, the $types parameter is always set to what it was for the first call, and there are a bunch of phantom parameters added to the hook invocation that represent what should have been set on subsequent calls....

This patch fixes it and also provides a test.

Comment	File	Size	Author
#10	hook-node-load-parameters-884948-10.patch	4.49 KB	David_Rothstein
#10
#2	hook-node-load-parameters-884948-2.patch	4.45 KB	David_Rothstein
#2
	hook-node-load-parameters.patch	4.31 KB	David_Rothstein

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pwolanin CreditAttribution: pwolanin commented 16 August 2010 at 20:12

This needs a code comment, and/or use some slightly more verbose code, like:

+   $this->hookLoadArguments = array();
    $this->hookLoadArguments[] = array_keys($typed_nodes);

or:

+   $this->hookLoadArguments = array();
-   $this->hookLoadArguments[] = array_keys($typed_nodes);
+   $this->hookLoadArguments[0] = array_keys($typed_nodes);

Comment #2

David_Rothstein CreditAttribution: David_Rothstein commented 16 August 2010 at 20:20

File	Size
hook-node-load-parameters-884948-2.patch	4.45 KB

Here it is with a code comment, plus slight code changes similar to the above recommendation (so we explicitly set the hook to have exactly one additional parameter).

Comment #3

benshell CreditAttribution: benshell commented 5 November 2010 at 00:39

Status:

Needs review

» Reviewed & tested by the community

Looks good to me. I was just debugging through the code for this problem (which I experienced with a Webform as a block) and I found that the hookLoadArguments was the issue (causing the webform module's hook_node_load function not to work correctly). This patch solved the problem.

Comment #4

Dries CreditAttribution: Dries commented 5 November 2010 at 10:51

+++ modules/node/node.module	16 Aug 2010 20:18:32 -0000
@@ -3670,7 +3670,9 @@ class NodeController extends DrupalDefau
+    $this->hookLoadArguments = array(array_keys($typed_nodes));

Any reason why we store an array in an array?

Comment #5

David_Rothstein CreditAttribution: David_Rothstein commented 5 November 2010 at 12:32

-    $this->hookLoadArguments[] = array_keys($typed_nodes);
+    // Besides the list of nodes, pass one additional argument to
+    // hook_node_load(), containing a list of node types that were loaded.
+    $this->hookLoadArguments = array(array_keys($typed_nodes));

As shown in the full diff, it's the same array-within-an-array as before; and the reason is that hookLoadArguments is an array of arguments, but the argument we are putting in it is itself an array.

Possibly we could make it a bit more clear by defining $argument = array_keys($typed_nodes) beforehand and then doing $this->hookLoadArguments = array($argument) after that...?

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 21 November 2010 at 08:54

Leaving this one for Dries.

Comment #10

David_Rothstein CreditAttribution: David_Rothstein commented 23 November 2010 at 22:49

File	Size
hook-node-load-parameters-884948-10.patch	4.49 KB

Changed the patch to use a separate $argument variable, as per #5. I'm not sure we can make it any more clear than that :) In any case, it's preexisting code; this is just fixing a bug.

Should still be RTBC as long as the tests pass.