content_view() not recursive safe

mfredrickson - March 20, 2007 - 02:40
Project:Token
Version:5.x-1.6
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

I don't know whether to file this bug here or in CCK - but the content_view function is not recursion safe. The result is an infinite loop if a CCK field modules uses token values during the view phase.

The execution goes like this:

content_view($node) calls the formatter, formatter calls token_replace($str, 'node', $node), content_token_values() calls content_view() ... and we have a never ending cycle.

I found this bug while trying to integrate token.module replacement into viewfield. I'll see if a solution doesn't present itself, but I otherwise don't have a suggestion just yet.

-M

#1

eaton - March 20, 2007 - 05:07

I'll put in a recursion check tonight that prevents token from looping that way -- it's a danger if enough things use tokens that the module itself should take care of it. Thanks!

#2

eaton - March 20, 2007 - 11:35
Status:active» needs review

Mark, I've committed a simple fix to the dev branch of Token. It allows tokens to go two layers deep, but no deeper. Do you think this is sufficient?

--Jeff

#3

mfredrickson - April 13, 2007 - 21:51

I'm not seeing your fix in 1.x-dev or HEAD. Should I be looking elsewhere?

-M

#5

quicksketch - April 16, 2007 - 06:19

I ran into this same problem while implementing token support into link module. The current recursion check doesn't prove to be effective in preventing the problem, as the actual depth is never actually checked before the module_invoke_all('token_values', $type, $object).

I added the check in such as this:

<?php
 
elseif ($depth < 3) {
   
$tmp_tokens = module_invoke_all('token_values', $type, $object);
   
$tokens[$type][$id] = $tmp_tokens;
  }
?>

It prevents the loop but the tokens were never replaced at all.

I think the real problem here is the call to content_view() in token_cck.inc. Perhaps we should remove it entirely and live with the 'view' version of each cck field unpopulated?

#6

eaton - April 16, 2007 - 12:14

We can't really do that -- Although I can't believe I left the depth check out. Heh.

The problem is that the 'cleaned' versions of each field, the ones that are display-safe, are often built by the formatters. And the formatters don't do anything until the view call is made. I'll check that recursion check again. Heh. Thanks :)

#7

mfredrickson - May 29, 2007 - 02:42
Status:needs review» fixed

Looks good from viewfield's perspective. I'm marking closed.

#8

Anthony Hersey - May 31, 2007 - 21:42

@mfrederickson - Which version of token.inc are you testing against? Neither the latest build nor the 1.7 release work for me with the latest DRUPAL-5 version of viewfield.module. I'm getting an infinite loop in the token_get_values() function. As near as I can tell it's from this:

<?php
 
elseif ($depth = 1) {
   
$depth++;
  }
  ...
 
$depth--;
?>

Which never gets to the else block. I changed that to == and it seems to be working fine. Patch attached.

AttachmentSize
token-inc.txt 488 bytes

#9

Anthony Hersey - May 31, 2007 - 23:27

And to follow up on this, this will not work if there are multiple things calling token on the same page - for example a teaser list wherein each teaser has a viewfield exposed.

I suppose depth could be a keyed array off of $type and $object but I don't think that could reliably handle this case, either.

#10

Anthony Hersey - June 1, 2007 - 16:29
Status:fixed» needs review

Attached is a patch that keys depth off of the $id also used for the token cache in the function (I moved the declaration up). This will allow multiple CCK items in the same request to use tokens as long as they are unique.

AttachmentSize
token-inc.2.txt 1.63 KB

#11

Anthony Hersey - June 1, 2007 - 16:44

The patch in #10 didn't key by both $type and $id, this one does.

AttachmentSize
token-inc.3.txt 1.74 KB

#12

Anthony Hersey - June 4, 2007 - 17:07

Another day, another patch. This one is simpler, and allows for multiple modules (custom_pagers, pathauto, etc) to use token in the same request without content_view using up the depth counter. It just sets a boolean flag when it starts generating for one call, and clears it out again when done. I did a test of nested viewfields, and they seemed to work correctly. Still not positive this is the best approach, but ti satisfies my current use cases.

Sorry for all the rapid posting here, but this is a feature very much needed for my sanity.

AttachmentSize
token-inc.4.txt 1.49 KB

#13

mfredrickson - July 13, 2007 - 16:52
Status:needs review» reviewed & tested by the community

I like stromsweeper's "mutex" solution in #12.

@Eaton: Can you commit this and roll a new token release? I have a viewfield release (with some sweet features, including token support) that I'd like to push out to the masses.

#14

greggles - July 26, 2007 - 22:48

This same bug was messing up pathauto in certain circumstances.

The patch in #12 solves it for me.

@Eaton - I'm planning to commit this tomorrow unless you object. I won't make a new branch so at least it will be easy to roll back.

#15

drewish - August 1, 2007 - 20:08

i marked http://drupal.org/node/160671 as a duplicate of this.

#16

greggles - August 14, 2007 - 22:02
Status:reviewed & tested by the community» patch (to be ported)

This is now fixed for DRUPAL-5 - needs to be ported

#17

RobRoy - August 30, 2007 - 04:34

Fixed the problem for me with link.module too FYI, nice catch!

#18

greggles - October 30, 2007 - 15:24
Status:patch (to be ported)» fixed

Ported in eaton's work this past weekend.

#19

Anonymous - November 13, 2007 - 15:32
Status:fixed» closed

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

#20

alimc29 - February 1, 2008 - 16:19

Does anyone know if this patch has been incorporated into later upgrades of this module?

#21

greggles - February 1, 2008 - 16:41

@alimc29 - yes, it should be in all current versions of token now (i.e. 5.x-1.10, 6.x-*). If you are getting similar behavior to this, please post a new (and detailed) bug report.

Thanks.

 
 

Drupal is a registered trademark of Dries Buytaert.