Twig embed leads to recursion and InvalidComponentException

Created on 30 July 2024, about 1 year ago

Problem/Motivation

I faced a very weird bug with SDC. If one component embeds another, it leads to an exception. It is very hard to track down, because it ends up in a wrongly compiled Twig PHP template which basically leads to a recursion.

Steps to reproduce

I prepared a very simple example module β†’ , by just enabling it on the 11.x branch (10.3 as well). The website goes into WSOD and an exception occurs.

NOTICE: PHP message: Uncaught PHP Exception Drupal\Core\Render\Component\Exception\InvalidComponentException: "[placeholder] The property placeholder is required" at /var/www/html/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php line 203
2024-07-30T15:19:02.149398187Z 192.168.192.7 -  30/Jul/2024:15:19:01 +0000 "GET /index.php" 500

But as you can see, the placeholder is provided:

  $variables['page_top']['example'] = [
    '#type' => 'component',
    '#component' => 'example:foo',
    '#props' => [
      'placeholder' => 'foo',
    ],
  ];

And if you try to debug it, you will find that it works the first time, but fails on the second. But this second call is triggered by a wrong Twig Template.

If you find the compiled template and open it, you will find out that it tries to load wrong template:

$this->parent = $this->loadTemplate("example:bar", "example:foo", 1);

Basically, it tries to load itself again, but since it is embedded without any context, it fails because the placeholder is not set.

If you remove the only keyword ({% embed 'example:bar' %}{% endembed %}) it works. If you replace it with include ({% include 'example:bar' only %}), it also works. It only fails with the example I provided ({% embed 'example:bar' only %}{% endembed %}).

I have found this bug with a more complex example, but it highlights the issue better. It is represented in my example as a foo component:

  1. The component Twig template β†’ .
  2. How Twig sees that template source β†’ . Exatly the same.
  3. What Twig actually renders β†’ . You can even see under the comment for line 14 that it tries to render itself (search-bar) instead of a button, which is on that line.

Also, it doesn't matter how this component is rendered, whether by a render element or directly from Twig.

πŸ› Bug report
Status

Needs review

Version

11.0 πŸ”₯

Component
single-directory componentsΒ  β†’

Last updated 6 days ago

Created by

πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @niklan
  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm
  • Status changed to Active about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    @Niklan mean to open an MR?

  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    I'm not sure what I should add/change in MR at this point. I can't understand where this bug comes from. It seems to me that it is potentially a bug in Twig itself. I'm not familiar with Twig's internal code, but I think I should try to find out where the template is compiled to find out why it compiles it with a wrong template reference which leads to a recursion.

    I just don't know where to approach this task. It's definitely not a ComponentValidator problem. Everything hints and leads to Twig. Basically, that 'example:foo' component tried to be rendered twice, while it is a single call. And the second call comes from a wrong Twig compiled template, which is a little hard to follow and understand, so I want to try to find out where and how these templates are built in the first place.

    Maybe I'll try to make a failtest later.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    If you’re on slack #components or #ui_patterns someone may be able to help.

  • Merge request !9001#3464719 "Failtest" β†’ (Open) created by niklan
  • Pipeline finished with Failed
    about 1 year ago
    Total: 535s
    #239670
  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    Added a failtest which highlights the issue as well: https://git.drupalcode.org/issue/drupal-3464719/-/jobs/2292129#L3250

  • Pipeline finished with Failed
    about 1 year ago
    #239685
  • πŸ‡«πŸ‡·France pdureau Paris

    If you’re on slack #components or #ui_patterns someone may be able to help.

    Not #ui_patterns but #ui_suite_initiative ;)

  • πŸ‡«πŸ‡·France pdureau Paris

    If the issue is not specific to SDC (which is resolving template path from component ID: {% embed 'sdc_test:no-props' only %} ) and can be reproduce with normal Twig "embed" (calling directly a template path), the fix must be proposed directly at https://github.com/twigphp/Twig

  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    Did a quick and dirty test with just Twig, and it works fine.

    script.php:

    <?php
    
    use Twig\Environment;
    use Twig\Loader\FilesystemLoader;
    
    $loader = new FilesystemLoader(__DIR__ . '/templates');
    $twig = new Environment($loader, [
      'cache' => __DIR__ . '/cache',
    ]);
    echo $twig->render('foo.twig');
    

    ./templates/foo.twig:

    <div class="wrapper">
      {% embed 'bar.twig' with {} only %}{% endembed %}
    </div>
    

    ./templates/bar.twig:

    Hello from Bar!

    Output:

    $ drush scr var/scripts/twig/test.php 
    <div class="wrapper">
      Hello from Bar!
    </div>
    
  • Merge request !9089Resolve #3464719 "Possible fix" β†’ (Closed) created by niklan
  • Status changed to Needs review 12 months ago
  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    I'm almost given up, but found a major clue. It seems that \Drupal\Core\Template\ComponentNodeVisitor::leaveNode should exit if the node has a parent.

    It seems like when you render a node with a parent set, this filter validate_component_props receives a wrong context, which leads to an exception during validation.

    I've tested locally, and everything seems to be working fine now.

  • Pipeline finished with Success
    12 months ago
    Total: 634s
    #244731
  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    Well, the pipeline confirms that it fixes a problem and doesn't break anything else (at least what has been tested). On my project, everything works as expected now as well. Honestly, for me, it is hard to explain why this helps.

    Just an assumption from debugging: It looks like such components are represented twice in the Twig Node Tree, one of them is some kind of container (wrapper), it doesn't have a payload with variables, but \Drupal\Core\Template\ComponentNodeVisitor::leaveNode still adds a validation function to it, which leads to an exception.

  • Status changed to Needs work 12 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Hello could the issue summary be completed to include the proposed solution section of the default template please

    Thanks

  • Status changed to Needs review 12 months ago
  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    Updated summary and simplified it a little, since there is a failtest and a possible solution exists.

  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    smustgrave β†’ changed the visibility of the branch 3464719-failtest to hidden.

  • Status changed to RTBC 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Thanks! Removing issue summary tag

      1) Drupal\KernelTests\Components\ComponentRenderTest::testWrapperComponent
        Twig\Error\RuntimeError: An exception has been thrown during the rendering
        of a template ("[text] The property text is required").

    Shows test coverage

    Didn't see anything in the code that needed tweak.

  • Status changed to Needs review 11 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Reading the code I'm wondering if this change will lead to things not happening that should. What about the attach_library call? Going to ping @e0ipso to have a look as SDC maintainer.

  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    False positive

  • e0ipso Can Picafort

    Can we check weather the fix in πŸ› SDC incorrectly throws an exception about embedded slots Needs work also fixes this?

  • πŸ‡·πŸ‡ΊRussia niklan Russia, Perm

    Tested locally and it seems to have resolved the issue as well.

  • Status changed to Postponed: needs info 12 days ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Going to close this one as a duplicate and everyone seemed to help so checking all credit.

Production build 0.71.5 2024