$node is passed to PHP snippet by reference, and that can cause node data being unintentionally corrupted by PHP snippet evaluation, for ex. by calling code like $term = array_shift($node->taxonomy);
There should be either an option to specify if the node should be passed by ref, or cloned; or it should be always cloned; or at least the behavior should be documented and there should be warning for not using such code like the mentioned above.

CommentFileSizeAuthor
#6 714450_clone_fix.diff1.01 KBMGN

Comments

MGN’s picture

MGN commented
Title: Node data can be corrupted by PHP snippet evaluation » Improve documentation to emphasize the potential dangers of using PHP snippets
Category: feature » task

Generally, this advanced option should only be used by people who understand php. There is a warning on the form page indicating it is possible to break your site if it is used incorrectly. Still, I can work on improving the documentation in the readme file.

MGN’s picture

MGN commented
Component: Miscellaneous » Documentation
marrch_caat’s picture

marrch_caat commented

You're right, the PHP snippets shouldn't be used by someone who doesn't know PHP. But it's not obvious that the $node is passed to the snippet ByRef - and it's not a really common way, and many similar snippets in Drupal modules receive only clone of data to prevent it's modification. The purpose of the snippet id only construction of breadcrumbs data, not some actions affecting node itself. So I suppose that if there is some need to allow the snippet modifiing $node data, it should at least be made absolutely clear that the data is passed ByRef. IMHO.

MGN’s picture

MGN commented

I agree, and thinking about further the $node variable shouldn't be passed by reference. Do you know of an instance in the code where this is occurring? I thought it was only be passed by value...

marrch_caat’s picture

marrch_caat commented

Well, I wasn't really correct using the word "byref". But it's an object, and when it's passed by value - it just can't be destroyed or replaced with a new one, but its fields can be modified or even destroyed anyway. To prevent that, it's good practice to use clone() or Drupal wrapper drupal_clone() function, and when it's about visibility or similar PHP snippets - it's usually done so. That's what I meant...

MGN’s picture

MGN commented
Title: Improve documentation to emphasize the potential dangers of using PHP snippets » Node data can be corrupted by PHP snippet evaluation
Component: Documentation » Code
Assigned: Unassigned » MGN
Category: task » bug
Status: Active » Needs review
StatusFileSize
new 714450_clone_fix.diff1.01 KB

Ok, I think I see what you are getting at. I think the attached patch addresses this problem, can you test it out and see if it behaves as you would expect?

Thanks

marrch_caat’s picture

marrch_caat commented

Yes, the patch is working as I supposed.

MGN’s picture

MGN commented
Status: Needs review » Fixed

Thanks. Committed to 6.x-2.x-dev for further testing. http://drupal.org/node/714450

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.