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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

illeace’s picture

Issue summary: View changes
illeace’s picture

Issue summary: View changes
DamienMcKenna’s picture

DamienMcKenna’s picture

boon4376’s picture

I 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.

boon4376’s picture

Just an update - still experiencing this issue in RC 2

D2ev’s picture

I 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.

D2ev’s picture

Status: Active » Needs review
DamienMcKenna’s picture

Status: Needs review » Needs work

Thanks for the patch, we just need to update it to work with all entities rather than just nodes.

D2ev’s picture

Status: Needs work » Needs review
FileSize
4.27 KB

Yes, I have updated patch accordingly and test it for 'node' and 'taxonomy_term' entity type.

patch attached.

deviantintegral’s picture

  1. +++ b/metatag.module
    @@ -2298,3 +2298,60 @@ function metatag_is_error_page() {
    +        unset($metatag_parts[0]);
    

    array_slice() here?

  2. +++ b/metatag.module
    @@ -2298,3 +2298,60 @@ function metatag_is_error_page() {
    +    $metatag_rep_value .= str_replace($search_tokens, $replace_tokens, $token);
    

    There's a return right after this, so it looks like the concatenation is a no-op? Should we just return str_replace() directly?

  3. +++ b/metatag.module
    @@ -2298,3 +2298,60 @@ function metatag_is_error_page() {
    +  else {
    

    Just to confirm - this hits if you have a field without subtokens?

  4. +++ b/metatag.tokens.inc
    @@ -104,6 +104,8 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +    $metatags = metatag_token_process_metatag($metatags, $token_type);
    

    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?

  5. +++ b/metatag.tokens.inc
    @@ -116,3 +118,32 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +    preg_match_all("^\[(.*?)\]^", $data['value'], $data_tokens, PREG_PATTERN_ORDER);
    

    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".

  6. +++ b/metatag.tokens.inc
    @@ -116,3 +118,32 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +          unset($metatag_parts[0]);
    

    How about using array_slice() here?

  7. +++ b/metatag.tokens.inc
    @@ -116,3 +118,32 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +          $recursion[$entity_field] = array($entity_field);
    

    We should define $recursion as a new array above the foreach.

    This could also use a comment about what $recursion is tracking.

  8. +++ b/metatag.tokens.inc
    @@ -116,3 +118,32 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +          $metatag_rep_value = metatag_token_entitymetatagtoken_replace($metatags, $metatags[$entity_field]['value'], $token_type, $recursion);
    

    $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.

D2ev’s picture

Andrew - 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.

deviantintegral’s picture

  1. +++ b/metatag.module
    @@ -2298,3 +2298,54 @@ function metatag_is_error_page() {
    +  $data_tokens = token_scan($token);
    

    Nice, this is much better!

  2. +++ b/metatag.tokens.inc
    @@ -104,6 +104,8 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +    $metatags = _metatag_token_process_metatag($metatags, $token_type);
    

    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.

  3. +++ b/metatag.tokens.inc
    @@ -116,3 +118,34 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +function _metatag_token_process_metatag($metatags, $token_type) {
    

    These parameters (and the function return) need documentation.

D2ev’s picture

Andrew - 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.

DamienMcKenna’s picture

A small thing, but shouldn't metatag_token_entitymetatagtoken_replace() be able to reside in the tokens.inc file?

D2ev’s picture

DamienMcKenna - 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?

DamienMcKenna’s picture

@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.

D2ev’s picture

Ok, I moved it to token.inc file.

patch attached.

D2ev’s picture

For token_filter module patch attached here - #2370975: Module token_filter is not working with metatag module.

deviantintegral’s picture

This 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.

  1. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + * Loop through metatags to replace entity-metatag-token
    

    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."

  2. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   Entity token type. e.g, 'node','term'.
    

    Missing space after comma.

    But, I think we can clarify this line. How about "The entity token type, such as 'node' or 'term'.

  3. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   Retrun entity metatags array.
    

    Typo on Return.

    But instead, how about "The metatags array with entity tokens replaced." ?

  4. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +          // that need to be replace too.
    

    replaced, not replace.

  5. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + * Replace entity metatag token([<entity_token_type>:metatag:<xyz>]) with actual entity metatag field value.
    

    Let's remove the token example text to get this line under 80 characters.

  6. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   A list entity metatag tokens.
    

    "An array of entity..."

  7. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   A token need to be search and replace.
    

    "A token to be replaced."

  8. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   Entity token type. e.g, 'node','term'.
    

    The entity token type, such as 'node' or 'term'.

  9. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   A list of tokens need to replace.
    

    "An array of tokens to be replaced."

  10. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   A list of token searched.
    

    "An array of tokens to search for."

  11. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    + *   The replaced value of token.
    

    "The replaced value of $token." (to make it clear that it's from the function parameter)

  12. +++ b/metatag.tokens.inc
    @@ -116,3 +118,76 @@ function metatag_token_generate_array($entity, $entity_type, $bundle) {
    +function metatag_token_entitymetatagtoken_replace($metatags, $token, $token_type, $replace_tokens = array(), $search_tokens = array()) {
    

    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.

D2ev’s picture

Thanks Andrew. Updated the patch documention.

D2ev’s picture

deviantintegral’s picture

I just went through the testing steps in the original issue description and it still WSOD's on me. I've attached the error trace.

deviantintegral’s picture

Status: Needs review » Needs work
D2ev’s picture

Andrew 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.

D2ev’s picture

Status: Needs work » Needs review

illeace’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

deviantintegral’s picture

Actually... 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.

Dave Reid’s picture

@deviantintegral: Strange, your backtrace in #24 clearly shows that Token filter was available and enabled.

deviantintegral’s picture

I 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.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thank you both for your work on this!

  • DamienMcKenna committed 55b5398 on 7.x-1.x authored by D2ev
    Issue #2198669 by D2ev: Using metatag tokens can easily cause an...

Status: Fixed » Closed (fixed)

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