A large number of nested quotes leads to fatal error (25 in my case), and the whole site crashes. The problem lies in recursive callbacks when processing content. So, removing the recursion may be a good solution? Patch provided.

Fatal error: Maximum function nesting level of '100' reached, aborting!

CommentFileSizeAuthor
quote-nesting-level-fix.patch2.16 KBdmitrit
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SimonLitt’s picture

Hi,
this path processes only the first tag (and child thereof).
It can also can also cause damage to the document tree.

The _quote_filter_process function should be:

function _quote_filter_process($text) {
  $quote_further_filtering = true;
  while (stristr($text, '[quote') && $quote_further_filtering) {
    $text_original = $text;
    $pattern = '#\[quote([^<\[]*?)]((?>\[(?!/?quote[^[]*?])|[^[]|(?R))*)\[/quote]#is';

    $index = 0;
    $quotes = array();
    while (preg_match($pattern, $text, $matches/*, PREG_SET_ORDER*/)) {
      $quotes[++$index] = ($matches);
      $text = $quotes[$index][2];
    }

    $quote_further_filtering = $index > 0;
    $quote = end($quotes);
    $key = key($quotes);
    while ($quote) {
      $quote_original = $quote[0];
      $quote_author = trim(drupal_substr($quote[1], 6));
      $quote_content = $quote[2];

      $quote_output = theme('quote', array('quote_content' => $quote_content, 'quote_author' => $quote_author, 'nest' => $key));
      if ($quote = prev($quotes)) {
        $key = key($quotes);
        $quote[2] = str_replace($quote_original, $quote_output, $quote[2]);
      }
      else {
        $text = str_replace($quote_original, $quote_output, $text_original);
      }
    }
  }

  return $text;
}
Honestly Illustrated’s picture

Title: Remove recursion in filter process callbacks » Recursion in _quote_filter_process causes fatal error
Status: Needs review » Needs work

Renaming title to be more clear.

I hit 26 nested comments without Fatal Error using the patch in #19 on #1448504: Undefined index: de in _quote_get_quoted_data(). However, page layout inside the quote nest was broken long before getting that many quotes nested.

Changing status because I think we need work, not review of existing work. Here's why:

I doubt that an administrator should want the users to be given an ability to make a comment of >25 other comments nested within each other, with that same data already present in the forum topic. We're making other greater problems for the whole system by allowing that as a feature of this module.

The code in #1 might patch the recursion bug and close the issue as originally defined, but this is also an opportunity to decide whether or not we need or want that behavior at all. The "[snip]" JavaScript is a clever way to allow deeply nested quotes, but why should we?

This is a lot of content data that we are duplicating in forum threads. That duplication will slow down search queries, and will artificially fatten results.

The quoting behavior of users is not likely helped by doing this, in my opinion. I believe that if a user chooses to quote a post, they are choosing to quote the post that they are going to reply directly to, and that the entire prior thread does not need to be duplicated in their response. At most, two posts should be duplicated, as evidenced by the earlier code author's decision to place the "[snip]" function at that location. Two posts is also the choice I've seen made by other forum software, where two posts in a quote is a hard limit (that's all you get).

Forum/content moderator role is given much extra work with this ability given to users. Should a moderator need to remove a content from a topic in their domain, they're now required to go through all "[snip]" instances in the topic to dig out quoted instances of the content.

There are severe layout issues when the quote tree extends beyond a few nested entries. The severity is dynamic, variable on the screen resolution and other configurations of the user agent. Suffice it to say that above two nested quotes will probably break mobile phone layout, and a few more breaks comfortable reading on the lowest resolutions we should design for.

We should look at cutting out the "[snip]" feature entirely, and limiting the quote behavior to the two most recently quoted entries in a thread of user quoting. In the future, we may possibly give a configurable variable limit for the amount of nested comments, so that the limit is not a magic number.

ivnish’s picture

Issue summary: View changes

_quote_filter_process doesn't use anymore (in the new 2.x branch)

ivnish’s picture

Status: Needs work » Closed (outdated)