content_token_values() switches build mode to token (and doesn't switch it back) [#1458806]

Hi,
I found the "cause" for this bug in CCK, but it did happen after a token upgrade to 6.x-1.18. So I suppose something in previous token versions did protect from this behavior.

content_token_values() does change the build mode of the object that it receives as an argument to be "token".
If that node is cached (which is usually the case), and after the procedure it is rendered, the build mode will still be on "token", instead of what it used to be (such as, "full" or "teaser" or whatever).

The bug happens to me with a particular combination of modules, but I think the design flaw is something more general. Even if you cannot reproduce, you should still be worried.

In general, this is a design flaw in Drupal core: The same cached node object is used for a variety of procedures, which all have free write access on the node's properties. This is a wide-open door for nasty side effects.

-------

My own quick fix was to patch content_token_values() (in cck/includes/content.token.inc), to remember the old build mode and restore it afterwards.

Quick fix:
What I would suggest for token is to do the same in token module, in whatever function that calls module_invoke_all('token_values'). Remember the original build mode, and restore it afterwards.

Real fix:
Clone the node object, before it is passed into hook_token_values(). This might have other consequences, why it would be better to postpone into a major version upgrade of token.

Comments

Comment #1

donquixote CreditAttribution: donquixote commented 27 February 2012 at 18:48

I just had the same bug strike again, in a different way.
The "quick fix" was not enough. The only solution was to clone the node.
I did that in content_token_values(), but probably it is ok or preferable to do that in token instead. After all, content_token_values() is not the only thing that can mess with the node object.

More information:
content_token_values renders the node in build mode "token", and overwrites some of the field labels. Later, display suite renders the node again (in build mode "full"), but with the overridden field labels from build mode "token".
(even though token build mode was not configured to render with display suite.. whatever)
As a little experiment, I set one of the field labels to "Description (token)" in token build mode config, and voila, that exact label showed up in the full view of the node.
Btw, I have a staging site that is almost identical, and there I was not able to reproduce the problem. Seems this depends on the order of things being processed.

My conclusion:
- This bug is not easy to reproduce.
- Instead of trying to reproduce, improve the flawed design of "everyone messes with the one node instance". This logical issue is quite obvious, even without the bug showing up in practice.