- Issue created by @niklan
- Status changed to Active
8 months ago 6:41pm 30 July 2024 - π·πΊ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.
- π·πΊRussia niklan Russia, Perm
Added a failtest which highlights the issue as well: https://git.drupalcode.org/issue/drupal-3464719/-/jobs/2292129#L3250
- π«π·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>
- Status changed to Needs review
8 months ago 4:45pm 5 August 2024 - π·πΊ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.
- π·πΊ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
8 months ago 10:07pm 11 August 2024 - πΊπΈ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
8 months ago 2:30pm 12 August 2024 - π·πΊRussia niklan Russia, Perm
Updated summary and simplified it a little, since there is a failtest and a possible solution exists.
- πΊπΈUnited States smustgrave
smustgrave β changed the visibility of the branch 3464719-failtest to hidden.
- Status changed to RTBC
7 months ago 2:10pm 30 August 2024 - πΊπΈ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
7 months ago 7:59am 11 September 2024 - π¬π§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.
- 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.