So this is a bit of an odd problem. The call to content_view() in token_cck.inc in function content_token_values() can cause some really weird behavior. If token_replace() is called within a hook_nodeapi() function, and $op == 'view', token module causes any changes made to $node->content to be erased.

Steps to reproduce:
1. Install token.module and custom_breadcrumbs.module (a module which calls token_replace() in $hook_nodeapi())
2. Create a module with a hook_nodeapi();

function testmodule_nodeapi(&$node, $op, $teaser, $page) {
  if ($op == 'view') {
    $field_name = 'field_testfield';
    $node->content[$field_name]['#value'] = 'a new value'
  }
}

Where 'field_testfield' is any field provided by CCK.

3. Go to a node page with your field, where you would expect that it would be displayed as 'a new value'. Instead, the default CCK display for that field will be displayed. Disable the custom_breadcrumbs() module or comment out the content_view() in token_cck.inc. The replacement works fine.

This behavior is caused by code within content_view():

function content_view(&$node, $teaser = FALSE, $page = FALSE) {
  if ($node->in_preview) {
    _content_widget_invoke('process form values', $node);
  }
  $content = _content_field_view($node, $teaser, $page);
  $node->content = array_merge((array) $node->content, $content);
}

As you can see, the $node->content variable is replaced with the CCK modifications on every call to content_view. I'm not sure how it's modifying the original node, since the $object variable passed into token_replace() isn't passed by reference.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eaton’s picture

This is a PHP5 issue -- variables are passed by reference automatically, we should totally be doing a clone on that before working with it. thanks!

quicksketch’s picture

Priority: Normal » Critical
FileSize
1.02 KB

Hiya fellas. This problem still hasn't been fixed. It's been pointed out to me that it's also breaking CCK Slideshow now. I found a much easier way to reproduce the problem also, so it's more clear what's happening (if that's necessary).

To reproduce:
- Install token and CCK.
- Open up token.module and take a look at token_get_values(). Note that $object is NOT passed in by reference.
- Scroll down to where the token hook is invoked:

    $tmp_tokens = module_invoke_all('token_values', $type, $object, $options);

- Add this line after the hook to change the body.

    $tmp_tokens = module_invoke_all('token_values', $type, $object, $options);
    $object->content['body']['#value'] = 'Token module has pwned your content!';

Now go look at your site, and you'll see that token module has inadvertently changed the body of every node on your site, even though $object was not passed in by reference.

Anyway, this causes serious problems when modules have already changed the content of a node once, then the token lookup is performed on CCK fields and the field values in the node are wiped and replaced a second time.

This patch makes a copy of the $object so this sort of behavior doesn't occur.

quicksketch’s picture

Status: Active » Needs review
eaton’s picture

Thanks for the heads up on this -- I think a lot of the initial testing for the module was done on PHP4. This is definitely necessary, and I'll stick it in this week when I take a spin through the queue. I think, though, we might want to use drupal_clone() -- I think that might be easier than wrapping things in array/object casts. Am I missing something?

quicksketch’s picture

Ah, drupal_clone may indeed be the better way to go. I hadn't found clone() in PHP because it's a language construct and not included in the function reference.

greggles’s picture

I just marked http://drupal.org/node/162911 as a duplicate of this. The patch looks good to me aside from the drupal_clone idea.

moshe weitzman’s picture

this appears to be causing headaches in the views_bookmark issue queue as well. a similar issue bit karen in the date.module queue - http://drupal.org/node/189093#comment-671798

jbrown’s picture

FileSize
751 bytes
89.96 KB
27.92 KB

I have been experiencing this bug through custom_links and my op_video module.

token1.png shows that the teaser is set to "preview image", but token2.png shows the teaser being rendered as full "Inline player"

With the supplied patch this bug does not occur.

This patch uses drupal_clone()

greggles’s picture

Status: Needs review » Patch (to be ported)

I just committed a combination of the two patches to the DRUPAL-5 branch. Assuming this works well for folks I think it's about time that we did another tag and release.

Thanks everyone for your help on this. I can imagine that this was tough to hunt down and your help is very appreciated.

greggles’s picture

Status: Patch (to be ported) » Fixed

and now committed to DRUPAL-6--1 as well.

quicksketch’s picture

Thanks Greg, very much appreciated!

quicksketch’s picture

Status: Fixed » Needs review
FileSize
715 bytes

We need to wrap the drupal_clone() in an IF statement in token_get_values(), otherwise all calls to token_get_values($string, 'global') causes and fatal PHP error when Drupal tries to clone NULL

Fatal error: __clone method called on non-object in /Users/nate/Sites/drupal5/includes/common.inc on line 1385
greggles’s picture

Status: Needs review » Fixed

Thanks, quicksketch - applied to both DRUPAL-5-1 and DRUPAL-6--1.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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