When I run TruncateHTML on an HTML fragment that has > 256 children, I get an error "Maximum function nesting level reached" on TruncateHTML::removeProceedingNodes(). The reason is that removeProceedingNodes() goes one level of recursion deeper for every next sibling (not child, sibling!) under a parent element. Usually this is not a problem but if you have elements with many children, the call stack grows too large.

I will attach a patch that appears to solve this issue.

In order to replicate this, XDebug has to be enabled and the configuration should include the xdebug.max_nesting_level setting.

CommentFileSizeAuthor
#20 smart_trim_applied_patch.png69.36 KBankondrat4
#14 max_function_nesting-3013628-13.patch1.6 KBpcate
#12 max_function_nesting-3013628-12.patch1.49 KBultimike
#7 maximum_function_nesting-3013628-D8-7.patch1.2 KBdimilias
#7 test_only.patch491 bytesdimilias
#2 smart_trim-3013628_1.patch733 bytessbesselsen

Issue fork smart_trim-3013628

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sbesselsen created an issue. See original summary.

sbesselsen’s picture

sbesselsen commented
StatusFileSize
new smart_trim-3013628_1.patch733 bytes

Here's my patch:

pcate’s picture

pcate commented

Patch fixed the issue for me.

pcate’s picture

pcate commented
Status: Active » Reviewed & tested by the community
markie’s picture

markie commented

Can I get a use case or sample HTML for testing?

markie’s picture

markie commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Still looking for a use case to test locally.

dimilias’s picture

dimilias commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new test_only.patch491 bytes
new maximum_function_nesting-3013628-D8-7.patch1.2 KB

I ran across this today. Indeed the patch solves the case for me.
I provided a test that proves the failure.

I see that locally though, for some reason the rest of the test cases fail as well though they work on the website. Let's see how it will behave here.

dimilias’s picture

dimilias commented

Could we have a test suite run here?

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

Setting this to minor since this is more of an environment configuration problem which is unlikely to happen in production environments. In order to replicate this the following conditions need to be met:

  1. XDebug must be enabled.
  2. The xdebug.max_nesting_level option must be set.

Still this is worthwhile to fix to make sure the module works also in development environments with XDebug enabled, and also to reduce the memory footprint in production environments.

The patch looks good to me. I tried to run the tests locally, but as expected this can only be reproduced with XDebug enabled and configured to limit recursion. As mentioned by @idimopoulos above the current test suite is failing, but fixing this is out of scope of this issue, it is being handled in #3143589: Tests are failing.

Here is the result with only the test applied:

$ ./vendor/bin/phpunit web/modules/contrib/smart_trim/
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.6
Configuration: /home/pieter/v/joinup-dev/phpunit.xml

Testing web/modules/contrib/smart_trim/
FFF.....F                                                           9 / 9 (100%)

Time: 203 ms, Memory: 6.00MB

There were 4 failures:

1) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #0 ('A test string', 5, '…', 'A tes…')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'A tes…'
+'A…'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

2) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #1 ('“I like funky quotes”', 5, '', '“I li')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I li'
+'“I'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

3) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #2 ('“I <em>really, really</em> li...uotes”', 14, '', '“I <em>really, rea</em>')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I <em>really, rea</em>'
+'“I <em>really</em>'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

4) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateWords with data set #4 ('“I <em>really, really</em> li...uotes”', 2, '', '“I <em>really,</em>')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I <em>really,</em>'
+'“I <em>really</em>'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:70

FAILURES!
Tests: 9, Assertions: 9, Failures: 4.

Other deprecation notices (2)

  1x: The "Symfony\Component\Debug\DebugClassLoader" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\DebugClassLoader" instead.

  1x: The "Symfony\Component\Debug\ErrorHandler" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\ErrorHandler" instead.

Here is the output with only the test and XDebug enabled and set to a recursion level of 32:

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.6 with Xdebug 2.9.4
Configuration: /home/pieter/v/joinup-dev/phpunit.xml

Testing web/modules/contrib/smart_trim/
FFFE....F                                                           9 / 9 (100%)

Time: 634 ms, Memory: 8.00MB

There was 1 error:

1) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #3 ('Maximum nesting level protect...d</h4>', 3, '', 'Max')
Error: Maximum function nesting level of '32' reached, aborting!

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:260
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:264
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:183
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/src/Truncate/TruncateHTML.php:130
/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

--

There were 4 failures:

1) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #0 ('A test string', 5, '…', 'A tes…')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'A tes…'
+'A…'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

2) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #1 ('“I like funky quotes”', 5, '', '“I li')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I li'
+'“I'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

3) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #2 ('“I <em>really, really</em> li...uotes”', 14, '', '“I <em>really, rea</em>')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I <em>really, rea</em>'
+'“I <em>really</em>'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

4) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateWords with data set #4 ('“I <em>really, really</em> li...uotes”', 2, '', '“I <em>really,</em>')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I <em>really,</em>'
+'“I <em>really</em>'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:70

ERRORS!
Tests: 9, Assertions: 8, Errors: 1, Failures: 4.

Other deprecation notices (2)

  1x: The "Symfony\Component\Debug\DebugClassLoader" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\DebugClassLoader" instead.

  1x: The "Symfony\Component\Debug\ErrorHandler" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\ErrorHandler" instead.

And here with patch + test:

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.6 with Xdebug 2.9.4
Configuration: /home/pieter/v/joinup-dev/phpunit.xml

Testing web/modules/contrib/smart_trim/
FFF.....F                                                           9 / 9 (100%)

Time: 635 ms, Memory: 8.00MB

There were 4 failures:

1) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #0 ('A test string', 5, '…', 'A tes…')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'A tes…'
+'A…'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

2) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #1 ('“I like funky quotes”', 5, '', '“I li')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I li'
+'“I'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

3) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateChars with data set #2 ('“I <em>really, really</em> li...uotes”', 14, '', '“I <em>really, rea</em>')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I <em>really, rea</em>'
+'“I <em>really</em>'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:26

4) Drupal\Tests\smart_trim\Unit\TruncateHTMLTest::testTruncateWords with data set #4 ('“I <em>really, really</em> li...uotes”', 2, '', '“I <em>really,</em>')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'“I <em>really,</em>'
+'“I <em>really</em>'

/home/pieter/v/joinup-dev/web/modules/contrib/smart_trim/tests/src/Unit/TruncateHTMLTest.php:70

FAILURES!
Tests: 9, Assertions: 9, Failures: 4.

Other deprecation notices (2)

  1x: The "Symfony\Component\Debug\DebugClassLoader" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\DebugClassLoader" instead.

  1x: The "Symfony\Component\Debug\ErrorHandler" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\ErrorHandler" instead.

So the test correctly detects that the issue is fixed, even though it is only reproducible using XDebug.

Fixing the other tests is out of scope, so this is RTBC.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Title: Maximum function nesting level reached for elements with many children » Maximum function nesting level reached for elements with many children on environments with XDebug
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Issue summary: View changes

Updating issue summary to clarify that this can only be replicated using XDebug.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Version: 8.x-1.x-dev » 2.0.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new max_function_nesting-3013628-12.patch1.49 KB

I've re-rolled this against the 2.0.x branch with a couple of changes:

1. With the existing unit tests fixed #3222068: Unit tests failing, I've added an additional (very similar) test for this for truncating by words (as well as characters).
2. I'm pretty sure the loop in removeProceedingNodes() needs to be inside the existing if ($nextnode !== NULL) { - if not, the existing (now fixed) tests fail.

-mike

pcate’s picture

pcate commented

The patch doesn't apply for me with the stable 2.0.0 version of the module.

pcate’s picture

pcate commented
StatusFileSize
new max_function_nesting-3013628-13.patch1.6 KB

Updated patch attached for stable 2.0.0 version. It is based on #12.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Version: 2.0.x-dev » 2.1.x-dev
Status: Needs review » Needs work

Let's convert this to an issue fork against 2.1.x and see how it goes. Seems like a reasonable change (and it has an associated test!)

-mike

ultimike opened merge request !70

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Status: Needs work » Needs review

I converted the patch into an MR (tests passing) - I also tweaked the test data a little.

Needs a review or two.

-mike

markie’s picture

markie commented

MR makes sense but are there testing instructions for it? Does the pipeline check xdebug? What limits the test to just 3 instances of the test string?

Not enough to say "needs work" but am curious..

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented

My understanding of this issue is if there are bucketloads of HTML elements in the field that Smart Trim is operating on, when using xdebug, the xdebug.max_nesting_level limit can be hit.

For manual testing, you'd need data to be Smart Trimmed that contains a bunch of HTML - so much that the xdebug.max_nesting_level it hit (as well as xdebug enabled, obviously).

The Gitlab pipeline does not run with xdebug enabled as far as I know.

-mike

ankondrat4’s picture

ankondrat4 commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new smart_trim_applied_patch.png69.36 KB

Hello.

MR was applied as expected. +1 RTBC.

  • markie committed 64ac0d13 on 2.1.x authored by ultimike 
    Issue #3013628 by ultimike, idimopoulos, PCate, sbesselsen: Maximum...
markie’s picture

markie commented
Status: Reviewed & tested by the community » Fixed

Merging this in. Thanks

Status: Fixed » Closed (fixed)

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