I posted about this to the devel list:

http://lists.drupal.org/pipermail/development/2006-September/019956.html

To summarize, under PHP5 the node object returned from node_load may not the
same during all stages of the page view. I'd attribute this to PHP 5's behavior where all objects are passed by reference. Assuming the behavior under PHP4 is the correct/desired behavior, then a solution seems to be to use drupal_clone in node_load to insure we preserve a pristine copy of the node object.

patch attached for Drupal 5.0 (HEAD), same patch to follow for 4.7.

for those who want to see the effect of the patch, here's a simple test. Create a node with a short body (e.g. "hello world!"). Find the nid. Create another node using PHP mode and put in the code below, substituting your nid for 12:

for ($i=1; $i < 5; $i++) {
$anode = node_load(12);
$anode->body .= " $i";
print $anode->body. "<br>";
}

on a an unpatched site under PHP5, the output will be:

hello word! 1
hello word! 1 2
hello word! 1 2 3
hello word! 1 2 3 4

on a site under PHP4 or with the patch, the output will be:

hello word! 1
hello word! 2
hello word! 3
hello word! 4
CommentFileSizeAuthor
#17 node_load_php5_fix2_5x-dev.txt1.02 KBpwolanin
#15 drupal_node_load_warnings.patch.txt763 bytesfago
#14 node_load_php5_fix2_47.txt769 bytespwolanin
#1 node_load_php5_fix1_47.diff.txt825 bytespwolanin
node_load_php5_fix1.diff.txt824 bytespwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
node_load_php5_fix1_47.diff.txt825 bytes

patch for 4.7

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

I marked http://drupal.org/node/88503 as a duplicate of this issue

Arto’s picture

Arto CreditAttribution: Arto commented

+1 as this seems to me, after some consideration, exactly the correct way to fix the issue on PHP5, and it doesn't have a negative impact on PHP4, either.

BTW, for anyone needing a refresher on PHP5 object references and the history of the drupal_clone() function, read Steven's 2004 writeup at http://www.acko.net/node/54.

Arto’s picture

Arto CreditAttribution: Arto commented
Status: Needs review » Reviewed & tested by the community

Verified that both the 4.7 and 5.0 patches still apply cleanly against the respective CVS versions, so marking this as ready to be committed.

Dries’s picture

Dries CreditAttribution: Dries commented

Aren't we cloning too much now? How about we clone when we take the node out of the cache? Looks like with this patch, we clone then node before we put it in the cache, and just before we take it out of the cache. I might be mistaken though.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dries- Yes, I really think you have to do both- otherwise, the the copy in the cache may be altered via the reference intially returned by node_load.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

FYI, if you leave off the 2nd drupal_clone at line 510, the output of the script above is:

hello word! 1
hello word! 1 2
hello word! 1 3
hello word! 1 4

If you leave off the 1st drupal_clone at line 469 the output is:

hello word! 1
hello word! 2
hello word! 2 3
hello word! 2 3 4

of course, you could put the 2nd drupal_clone at the return at line 513, but then you always incur the function call, rather than only when the node is cachable.

Arto’s picture

Arto CreditAttribution: Arto commented

Dries: as Peter said, both clone calls are definitely necessary. I initially had the same concern but upon further consideration came to realize the patch is exactly right, including the fact that the second clone happens on the conditional case instead of the default case.

I believe this is ready to be committed.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: x.y.z » 4.7.4
Status: Fixed » Reviewed & tested by the community

changing version to 4.7, RTBC to catch killes' attention

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

applied

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

I've just upgraded my 4.7 installations. Now I get some warnings on "node/34" pages, where the nid is not valid!

# warning: __clone method called on non-object in .../includes/common.inc on line 1213.
# warning: __clone method called on non-object in .../includes/common.inc on line 1213.

I'm using php 5.2.0, so drupal_clone uses the clone() function which fails for boolean values (the node object is false!), when node_load() tries to cache the node.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Fixed » Needs work

Hmmm, I think I see the problem. $cachable may be TRUE, even if $node is NULL. I'm not sure about elsewhere in the core code, but in node_module there check for correctness of the returned object seem to take the form: if ($node->nid) OR if ($node->vid). This is bad in any case, since they should be using isset(). Some similarly bad code in poll module: if ($node = node_load($nid))

So- 3 possible ways to deal with this off the top of my head.

1) put a check for non-objects in drupal_clone (maybe a bad hack)
2) change node_load further to only clone non-NULL (or is_object) values.
3) change node_load so that calls with an invalid nid/vid result in saving an object with $node->nid == 0, $node->vid == 0.

Sorry for missing this in the original patch.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
node_load_php5_fix2_47.txt769 bytes

Here's an attempt at a revised patch- not tested yet (I'm at work and shouldn't be doing this now). Note also the explicit setting of $cachable, for ALL_E compliance.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Version: 4.7.4 » 5.x-dev
FileSize
drupal_node_load_warnings.patch.txt763 bytes

your patch fixes the problem!

I assume your patch is for HEAD so I change the version.
Further I've attached your patch for drupal 4.7.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

my patch was for 4.7, but for HEAD would be the same except for an offset and the fact that the module is in a directory.

I'm glad to hear that this patch fixes the problem, but the question still remains whether it's the optimal solution.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
node_load_php5_fix2_5x-dev.txt1.02 KB

Hmm, apparently the 4-7-CVS tarball didn't have the fix1 patch from above yet, so actually my patch wasn't good.

Your patch should work for 4.7.x (though next time use the -p flag for diff).

Attached patch is for 5.x

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

tested patch from #17 with latest update of 5.x-dev on both PHP 5.1.2 and PHP 4.4.1 and seems to work as expected (no surprise). I'm inclined to stick with this approach, since it represents the smallest change.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Priority: Normal » Critical

I think this must be fixed before RC

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Fixed

Committed to HEAD.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: 5.x-dev » 4.7.x-dev

needs to be backported too

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Fixed » Reviewed & tested by the community
RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Status: Reviewed & tested by the community » Patch (to be ported)

Yeah, just ran across this is 4.7. Good call, changed status to the new "to be ported".

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Patch (to be ported) » Fixed

"to be ported" means to me: The patch should get into 4.7, but it doesn't apply due to soem changes between 4..7 and 5. This patch applied just fine, so "rtbc" would have been better.

applied

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)