Because metatag tokens for an entity are evaluated all at once instead of each as needed, you can easily generate an infinite loop by including a metatag token in a node field (and a node token in a metatag field). Of course, this would happen if the two fields referenced each other. However this also happens if the two fields do not reference each other and should be resolvable. The reason this happens is that the metatag_tokens() function tries to resolve all metatag token values, not just the items passed to it in the $tokens array.
Here are steps to reproduce the problem:
- Make sure the following modules are installed and enabled:
- Token
- Token Filter
- Metatag (and dependencies)
- Apply the patch in comment #1 of issue #2051407: metatag_token_generate_array() doesn't take language into account.
- Go to Configuration > Text Formats > Full HTML > Configure.
- Under "Enabled Filters" check the "Replace tokens" option and click Save configuration.
- Add a new Basic Page node.
- Enter data for the required fields.
- In the body field, enter "[node:metatag:keywords]". Make sure the text format is set to "Full HTML".
- Make sure the Meta tags Description field remains set to the default value of "[node:summary]".
- Enter "test" or some other string for the Meta tags Keywords field.
- Save your node.
- This will result in an infinite loop that exhausts all memory resources and whitescreens Drupal.
The basic flow of the code is this:
- The function metatag_tokens() gets called with a $tokens array containing "[metatag:keywords]".
- This function calls metatag_token_generate_array().
- Instead of just resolving the value of "[metatag:keywords]", the metatag_token_generate_array() function tries to resolve all the metatags associated with the passed-in entity as well as all default metatag values. This includes the Meta tag description field.
- The Meta tag description is set to the ["node:summary]" token.
- In order to get the value "[node:summary'], the node's body field must be resolved. The node body has the "[node:metatag:keywords]" token in it.
- In order to resolve the value of "[node:metatag:keywords]" the metatag_tokens() function get's called, which calls metatag_token_generate_array().
- And so on until PHP runs out of memory.
I am not familiar enough with tokens nor metatags to suggest a fix. I'm also not sure how likely a use case this is. Is someone really going to include a metatag token in a normal node field? I did look at how a few other modules implement the hook_tokens() function, and it seems like they only try to resolve the value(s) passed in the $tokens array. If the metatag_tokens() module did the same, I think this infinite loop would be avoided.
Comment | File | Size | Author |
---|---|---|---|
#24 | 2198669-recursion-example.txt | 19.92 KB | deviantintegral |
#23 | metatag-fixed-entity-metatag-tokens-recussion-issue-2198669-22.patch | 3.42 KB | D2ev |
Comments
Comment #1
illeace CreditAttribution: illeace commentedComment #2
illeace CreditAttribution: illeace commentedComment #3
DamienMcKennaComment #4
DamienMcKennaComment #5
boon4376 CreditAttribution: boon4376 commentedI also encountered this issue with Beta 9
I was attempting to use [node:metatag:description] in the open graph description and twitter description so that I would not have to enter the tag 3 times. It caused any pages using that configuration to fail.
Comment #6
boon4376 CreditAttribution: boon4376 commentedJust an update - still experiencing this issue in RC 2
Comment #7
D2ev CreditAttribution: D2ev commentedI have same scenario where we are using node metatag tokens like [node:metatag:] in other metatag fields which leads to infinite loop. I have fixed this issue by replace node-metatag-token to it's actual node field value to avoid infinite recursion. Patch attached
Same can be fixed in 'Token Filter' module. I am creating patch for 'Token Filter' module which has dependency on above patch.
Comment #8
D2ev CreditAttribution: D2ev commentedComment #9
DamienMcKennaThanks for the patch, we just need to update it to work with all entities rather than just nodes.
Comment #10
D2ev CreditAttribution: D2ev commentedYes, I have updated patch accordingly and test it for 'node' and 'taxonomy_term' entity type.
patch attached.
Comment #11
deviantintegral CreditAttribution: deviantintegral commentedarray_slice() here?
There's a return right after this, so it looks like the concatenation is a no-op? Should we just return str_replace() directly?
Just to confirm - this hits if you have a field without subtokens?
From the docblocks, it feels like metatag_token_entitymetatagtoken_replace() is the function to call, but it's actually _process_metatag(). Would it be clearer to have one function marked as internal with an underscore?
Does this regular expression exist anywhere else already? In includes/token.inc? Elsewhere in the module? This seems like a good candidate for a constant or a new function to return $data_tokens.
Do we need the question mark? The dot-star should already match "zero or more characters".
How about using array_slice() here?
We should define $recursion as a new array above the foreach.
This could also use a comment about what $recursion is tracking.
$replaced_value is probably a clearer variable name here.
I haven't tested this patch at all yet. There are also some code style and comment issues, but I've skipped those until we get the actual code comments addressed.
Comment #12
D2ev CreditAttribution: D2ev commentedAndrew - Thank you for your review. I have done modification to patch -
1. Changed to array_slice().
2. Changed to return str_replace() directly.
3. Condition execute If the field don't have any tokens in it.
4. Changed _metatag_token_process_metatag() function to private or internal.
5. Used token_scan() instead of my custom preg_match_all().
6. Changed to array_slice() now.
7. Defined $recursion.
8. Changed the variable name.
I haven't tested yet. patch attached.
Comment #13
deviantintegral CreditAttribution: deviantintegral commentedNice, this is much better!
I *think* what you have as an internal function is backwards.
Since you're calling metatag_token_process_metatag() from other code outside of the recursion function, that should probably be the "public" function. That would make metata_token_entitymetatagtoken_replace() the internal function - which would forgive the number of function parameters a little.
These parameters (and the function return) need documentation.
Comment #14
D2ev CreditAttribution: D2ev commentedAndrew - Following your points
2. I thought of using metata_token_entitymetatagtoken_replace() public function. Which can be access from outside when we want to get metatag replacement for a entity-metatag-token '$token' of token type '$token_type' with entity metatags '$metatags'. Let me know your suggestion on this.
3. Added _metatag_token_process_metatag() function documentation.
patch attached.
Comment #15
D2ev CreditAttribution: D2ev commentedComment #16
DamienMcKennaA small thing, but shouldn't metatag_token_entitymetatagtoken_replace() be able to reside in the tokens.inc file?
Comment #17
D2ev CreditAttribution: D2ev commentedDamienMcKenna - I think context of metatag_token_entitymetatagtoken_replace() function is with metatag module as it check entitymetatagtokens which is available only when metatag module enabled. Other modules that have dependecy on metatag can use the function. What are your thoughts?
Comment #18
DamienMcKenna@D2ev: I was thinking that metatag_token_entitymetatagtoken_replace() should be in the tokens.inc file because it's only used by the tokens functions and isn't relevant to the rest of the module.
Comment #19
D2ev CreditAttribution: D2ev commentedOk, I moved it to token.inc file.
patch attached.
Comment #20
D2ev CreditAttribution: D2ev commentedFor token_filter module patch attached here - #2370975: Module token_filter is not working with metatag module.
Comment #21
deviantintegral CreditAttribution: deviantintegral commentedThis is pretty close to RTBC. Most of the below are docs fixes, so they could be treated as followups if this fix needs to be merged ASAP.
It's good to try to have a one line summary wrapped to 80 characters. How about:
"Loop through metatags to avoid recursion on entity tokens."
Missing space after comma.
But, I think we can clarify this line. How about "The entity token type, such as 'node' or 'term'.
Typo on Return.
But instead, how about "The metatags array with entity tokens replaced." ?
replaced, not replace.
Let's remove the token example text to get this line under 80 characters.
"An array of entity..."
"A token to be replaced."
The entity token type, such as 'node' or 'term'.
"An array of tokens to be replaced."
"An array of tokens to search for."
"The replaced value of $token." (to make it clear that it's from the function parameter)
Let's flip the $replace_tokens and $search_tokens parameters? We normally talk about such things like "search and replace", so let's follow that order.
Comment #22
D2ev CreditAttribution: D2ev commentedThanks Andrew. Updated the patch documention.
Comment #23
D2ev CreditAttribution: D2ev commentedComment #24
deviantintegral CreditAttribution: deviantintegral commentedI just went through the testing steps in the original issue description and it still WSOD's on me. I've attached the error trace.
Comment #25
deviantintegral CreditAttribution: deviantintegral commentedComment #26
D2ev CreditAttribution: D2ev commentedAndrew if you are testing with Token Filter module then ensure that this patch is applied - #2370975: Module token_filter is not working with metatag module.
Let me know if you still find this issue.
Comment #27
D2ev CreditAttribution: D2ev commentedComment #29
illeace CreditAttribution: illeace commentedI applied this patch as well as the patch to token_filter mentioned in #26 above. When both patches are applied, the bug as written is fixed. Tokens are properly replaced in both the body field and the metadata fields.
Comment #30
deviantintegral CreditAttribution: deviantintegral commentedActually... turns out the site I was using never had the Token Filter module. But, this patch still fixed the recursion issue. +1 on the RTBC.
Comment #31
Dave Reid@deviantintegral: Strange, your backtrace in #24 clearly shows that Token filter was available and enabled.
Comment #32
deviantintegral CreditAttribution: deviantintegral commentedI tested on a stock D7 site with token_filter to follow the original issue, but this patch also passed on a client site that did not have token_filter at all.
Comment #33
DamienMcKennaCommitted. Thank you both for your work on this!