Problem/Motivation

A Twig node visitor enhances a template when an SDC component is found (modules/sdc/src/Twig/ComponentNodeVisitor.php).
The node visitor replaces 'display_start' and 'display_end' with its nodes, ignoring that other modules may have added something to them.

In particular, the Tracer module (used by WebProfiler) is adding some code to trace the template render time.

Steps to reproduce

* install a fresh copy of Drupal 10.1.*
* enable the SDC module, define a component, and add it to a page
* navigate to the page that contains the SDC component, everything work as expected
* download and enable WebProfiler (that in turn downloads and enables Tracer)
* navigate to the page that contains the SDC component
* the page broke with an error like this: Error: Call to a member function leave() on null in __TwigTemplate_355dee1e190651c61ec8a29afeb99829->doDisplay() (line 120 of /tmp/twig/64b904e4040f3_bocconi_ds:bds-header_bF4EXNuRh-ABv7SRE3u1qRweA/gyEYJRegggYskrr-GXJO_VBdHtvR8cGZlDzRANParpA.php).

Proposed resolution

Append information SDC needs to 'display_start' and 'display_end' Twig nodes instead of replacing them.

Remaining tasks

Review the MR

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

Issue fork drupal-3375843

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

lussoluca created an issue. See original summary.

lussoluca opened merge request !4420

dieterholvoet’s picture

dieterholvoet commented
Component: theme system » single directory components
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Issue tags: +stable blocker
Parent issue: » #3345922: Single Directory Components module roadmap: the path to beta and stable

IMO (at a first glance) this is also a stable blocker.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Version: 10.1.x-dev » 11.x-dev

Changed base branch.

e0ipso opened merge request !4867

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Re-rolled against 11.x and fixed some phpcs issues to allow tests to run. Thanks @lussoluca and @DieterHolvoet!!

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Hmmm, for some reason the GitLab UI to generate a new branch from 11.x did not work as I was expecting. I updated it manually.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Active » Needs review
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

All tests are passing. This confirms the static code review.

Next step before RTBC is for @lussoluca to confirm that the current MR (on the updated 11.x branch) works along with the Tracer module.

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented
Status: Needs review » Reviewed & tested by the community

I confirm that everything still works as expected

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Confirming the RTBC based on my code review, the green tests, and Luca's manual testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review

Is this something we could test in core? Maybe we could write a test module that does something similar to Tracer to make sure that we don't break this.

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented

I can work on it.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Leaving assigned to @lussoluca.

Moving to NW per #13

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented
Status: Needs work » Needs review

Test added, let me know if it's ok

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Only rebased to run the test-only run and tests failed as expected

There was 1 error:
1) Drupal\Tests\sdc\Kernel\ComponentNodeVisitorTest::testOtherVisitorsCanModifyTwigNodes
PHPUnit\Framework\Exception: Unavailable module: 'sdc_other_node_visitor'. If this module needs to be downloaded separately, annotate the test class with '@requires module sdc_other_node_visitor'.
/builds/issue/drupal-3375843/core/tests/Drupal/KernelTests/KernelTestBase.php:537
/builds/issue/drupal-3375843/core/tests/Drupal/KernelTests/KernelTestBase.php:387
/builds/issue/drupal-3375843/core/tests/Drupal/KernelTests/KernelTestBase.php:261
/builds/issue/drupal-3375843/core/modules/sdc/tests/src/Kernel/ComponentKernelTestBase.php:56
/builds/issue/drupal-3375843/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
--
There was 1 risky test:
1) Drupal\Tests\sdc\Kernel\ComponentNodeVisitorTest::testOtherVisitorsCanModifyTwigNodes
This test did not perform any assertions
/builds/issue/drupal-3375843/core/tests/Drupal/Tests/Listeners/DrupalListener.php:65
/builds/issue/drupal-3375843/vendor/phpunit/phpunit/src/Framework/TestResult.php:452
/builds/issue/drupal-3375843/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3375843/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3375843/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3375843/vendor/phpunit/phpunit/src/TextUI/Command.php:97
ERRORS!
Tests: 1, Assertions: 0, Errors: 1, Risky: 1.

Looking at the change it makes sense and does seem to solve the issue.
Tested on Umami which has SDC enabled with a disclaimer component.

  • lauriii committed a6aabb7f on 11.x 
    Issue #3375843 by lussoluca, e0ipso, smustgrave, DieterHolvoet: Allow...

  • lauriii committed ca19b50d on 10.2.x 
    Issue #3375843 by lussoluca, e0ipso, smustgrave, DieterHolvoet: Allow...

lauriii closed merge request !4867

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Crediting @DieterHolvoet for working on #3376785: Error: Call to a member function leave() on null which I believe is a duplicate.

Committed a6aabb7 and pushed to 11.x. Also cherry-picked to 10.2.x and 10.1.x. Thanks!

  • lauriii committed 818ad553 on 10.1.x 
    Issue #3375843 by lussoluca, e0ipso, smustgrave, DieterHolvoet: Allow...

Status: Fixed » Closed (fixed)

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