[TwigComponent] Fix usage of {% embed %} with {% block %} in <twig:> components, close #952 #1397

Kocal · 2024-01-16T18:03:20Z

Q	A
Bug fix?	yes
New feature?	no
Issues	Fix #952
License	MIT

Hi everyone, this PR is an attempt to fix #952.
It fix supports for {% block X %}foo{% endblock %} and {% block X "foo"%}, from {% embed %}, in <twig:> syntax.

The following code:

<twig:Alert>
    <p>
        {% embed "my_embed.html.twig" %}
            {% block my_embed_block_1 "foo" %}
            {% block my_embed_block_2 %}bar{% endblock %}
        {% endembed %}
    </p>
</twig:Alert>

is now properly converted to:

{% component 'Alert' %}
    {% block content %}<p>
        {% embed "my_embed.html.twig" %}
            {% block my_embed_block_1 "foo" %}
            {% block my_embed_block_2 %}bar{% endblock %}
        {% endembed %}
    </p>
{% endblock %}{% endcomponent %}

Since I'm not familiar with the PreLexer, let me know if a better solution/implementation exists.

smnandre · 2024-01-16T19:16:50Z

I'm personnaly 👎

Main reasons:

I'm really not in favor of mixing syntaxes...
I don't want twig:blocks to be used outside components
TwigComponent should not be a trojan horse to change all the Twig syntax

When we'll rewrite the HTML prelexer, or the component rendering/ code generation... we don't want to deal with all Twig. As we already have not all the time to solve or improve TwigComponent / LiveComponent things.

I won't fight this in anyway of course, but really i'd rather go the other way around and forbid <twig:block /> outside TwigComponent "today"...

Kocal · 2024-01-16T21:53:55Z

So what's your alternative for smoothly migrating big code-base that uses {% embed %} with {% block %} inside, without adding a lot of works?

To me, blocks inside <twig:>' component should accept any valid Twig syntax.

smnandre · 2024-01-16T22:24:47Z

To me, blocks inside twig:' component should accept any valid Twig syntax.

On this, i 100% agree. Even if "philosophically" i'm having doubt about this embed-in-template situation.
I really agree (or we should announce this "non-compatibility").

But this may/should not be done by writing more code about specific template tags (again, not criticising your work on this, far from it..) but i lost so many hours trying to debug things in Twig/Live components, that i'm 100% certain if we start to code specific things about every tag construct of twig, there is a big maintanability problem...

So i really do not thing we should have any code concerning "embed" in this repository. Or we should also have to handle "macro", "with"..... and "use".

That beeing said, what are the alternatives right now ? I don't know :( Can this by fixing exisiting block handling ?

Kocal · 2024-01-17T07:14:22Z

But this may/should not be done by writing more code about specific template tags (again, not criticising your work on this, far from it..) but i lost so many hours trying to debug things in Twig/Live components, that i'm 100% certain if we start to code specific things about every tag construct of twig, there is a big maintanability problem...

So i really do not thing we should have any code concerning "embed" in this repository. Or we should also have to handle "macro", "with"..... and "use".

Yeah I understand your point too, maybe my approach is too naive and can be optimized for others Twig tokens like macro, with...

That beeing said, what are the alternatives right now ? I don't know :( Can this by fixing exisiting block handling ?

TBH I don't know, all I can say during my debug phase is that the following code (without blocks in embed):

<twig:Alert>
    <p>
        {% embed "my_embed.html.twig" with { foo: 'bar' } %}{% endembed %}
    </p>
</twig:Alert>

is nicely converted to

{% component 'Alert' %}
    {% block content %}<p>
        {% embed "my_embed.html.twig" with { foo: 'bar' } %}{% endembed %}
    </p>
{% endblock %}{% endcomponent %}

But this code:

<twig:Alert>
    <p>
        {% embed "my_embed.html.twig" %}
            {% block my_embed_block_1 "foo" %}
            {% block my_embed_block_2 %}bar{% endblock %}
        {% endembed %}
    </p>
</twig:Alert>

raises error Expected closing tag "</twig:Alert>" not found at line 8, and generates the following output (before raising the error):

{% component 'Alert' %}
    {% block content %}<p>
        {% embed "my_embed.html.twig" %}
            {% endblock %}{% block my_embed_block_1 "foo" %}
            {% block my_embed_block_2 %}bar{% endblock %}
        {% endembed %}
    </p>
</twig:Alert>

You can see a {% endblock %} between {% embed "my_embed.html.twig" %} and {% block my_embed_block_1 "foo" %}, it has been added by the line

if (!empty($this->currentComponents) && $this->currentComponents[\count($this->currentComponents) - 1]['hasDefaultBlock']) {
    $output .= '{% endblock %}';

    $this->currentComponents[\count($this->currentComponents) - 1]['hasDefaultBlock'] = false;
}

weaverryan · 2024-01-17T17:53:12Z

Yo @Kocal!

We should definitely fix this. But, to be clear, I think there are 2 separate issues. I think you fixed them both at once, but I want to clarity.

Issue 1: blocks without endblock:

<twig:Terminal bottomPadding="145">
    composer require symfony/asset-mapper symfony/stimulus-bundle
    {% block my_embed_block_1 "foo" %}
</twig:Terminal>

This errors - we're simply not processing the {% block anything "content" %} properly.

Issue 2: the embed issue where we think blocks that are inside of the embed are blocks for the component.

smnandre · 2024-01-17T19:31:24Z

I think fixing the block thing in the Prelexer would fix both, no ?

ux/src/TwigComponent/src/Twig/TwigPreLexer.php

Line 86 in a2418ef

$output .= $this->consumeUntil('%}');

In all manner (and not related to this PR) we have to refacto/improve the PreLexer soon, as we do not handle a lot of minor / edge cases

smnandre · 2024-01-17T19:32:44Z

@Kocal .. do you think you could extract this part in your code and try without any "embed" handling at first ? Or you do think both need to be fixed at once ?

Kocal · 2024-01-19T19:14:29Z

Hi everyone 👋, thanks for your suggestions, I'll try them ASAP!

Cheers from the mountains 😛

weaverryan

I'm ok with the 2 things in on PR as long as we can get test cases to cover both individually :).

View from the mountain looks crazy to me. I ski, but on hills - never have been on a real mountain in winter!

weaverryan · 2024-01-19T21:02:54Z

src/TwigComponent/src/Twig/TwigPreLexer.php

+                            Lexer::REGEX_NAME,
+                            '',
+                            trim(mb_substr($this->input, $prevPosition, $this->position - $prevPosition)))
+                        ;


Instead of keeping calculating the content between the positions, can we use the return value of $this->consumeUntil('%}');?

Also, in the test, you have

{% block my_embed_block_1 "foo" %}

I can understand that, after the preg_replace(), the "" would remain. And so isBlockSelfClosing would be true. But isn't this a valid syntax?

{% block my_embed_block_1 fooVariable %}

In this case, everything would be replaced, the result would be an empty string, and isBlockSelfClosing would be false, right? Could we, instead, (A) grab the content between {% block and %}, trim() the whitespace, then set isBlockSelfClosing to true if there are ANY non-whitespace characters left?

But isn't this a valid syntax?

{% block my_embed_block_1 fooVariable %}

Indeed. (https://twig.symfony.com/doc/3.x/tags/extends.html#block-shortcuts)

{% block title page_title|title %}

(but pretty sure this is not the hardest problem we may theoretically have while mixing twig/html :)

Instead of keeping calculating the content between the positions, can we use the return value of $this->consumeUntil('%}');?

Hum you're right, I will give it a try.

Also, in the test, you have

{% block my_embed_block_1 "foo" %}

I can understand that, after the preg_replace(), the "" would remain. And so isBlockSelfClosing would be true. But isn't this a valid syntax?

This is not true, when running the regex /[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*/A on my_embed_block "foo", only the my_embed_block will be removed from the string, "foo" will still be there.

See https://regex101.com/r/LVIP6p/1.

{% block my_embed_block_1 fooVariable %}

In this case, everything would be replaced, the result would be an empty string, and isBlockSelfClosing would be false, right? Could we, instead, (A) grab the content between {% block and %}, trim() the whitespace, then set isBlockSelfClosing to true if there are ANY non-whitespace characters left?

Nope, fooVariable will still be there after preg_replace, see https://regex101.com/r/FU3VmO/1.

weaverryan · 2024-01-19T21:03:31Z

src/TwigComponent/tests/Unit/TwigPreLexerTest.php

+            <twig:Alert>
+                <p>
+                    {% embed "my_embed.html.twig" %}
+                        {% block my_embed_block_1 "foo" %}


I'd love to see this tested independently, both with a string - like '"foo"- and also a variable like{% block my_block fooVar %}`

…components, close symfony#952

Kocal · 2024-01-20T14:03:19Z

I've applied your suggestions :)

do you think you could extract this part in your code and try without any "embed" handling at first ? Or you do think both need to be fixed at once ?

@smnandre unfortunately removing the $inTwigEmbed logic makes the 4 tests "embeds + blocks" fail :/ But maybe I'm missing something

smnandre · 2024-01-20T16:00:07Z

I looked a bit and it's almost perfect. But i think @weaverryan was right, and we are solving two problems at once that deserve independant solutions.

Here is a new test that fails (and probably not because of your code, it was just not tested before i guess)

yield 'component_where_entire_default_block_is_twig_embed_with_endblock_name' => [
            <<<EOF
            <twig:Alert>
                <p>
                    {% embed "my_embed.html.twig" %}
                        {% block my_embed_block %}{% endblock my_embed_block %}
                    {% endembed %}
                </p>
            </twig:Alert>
            EOF,
            <<<EOF
            {% component 'Alert' %}
                {% block content %}<p>
                    {% embed "my_embed.html.twig" %}
                        {% block my_embed_block %}{% endblock my_embed_block %}
                    {% endembed %}
                </p>
            {% endblock %}{% endcomponent %}
            EOF,
        ];

Trying to solve it, and thinking about what i suggested about the embed isolation, i realized.... we should never be in that situation in the first place :)

A current component cannot have any impact on children embed content... So i think when we detect an embed we should call the prelexer (as it's done in the block parsing)

But it was already there, so let's merge your fix and adress all this after :)

weaverryan · 2024-01-29T15:26:49Z

Thanks for your work on this Hugo!

Kocal · 2024-01-29T16:18:32Z

You're welcome :)

weaverryan reviewed Jan 19, 2024

View reviewed changes

[TwigComponent] Fix usage of {% embed %} with {% block %} in <twig:> …

0a86384

…components, close symfony#952

Kocal force-pushed the fix/GH-952 branch from 06a5bf5 to 0a86384 Compare January 20, 2024 14:02

weaverryan merged commit 6b2a80f into symfony:2.x Jan 29, 2024
34 checks passed

Kocal deleted the fix/GH-952 branch January 29, 2024 16:18

smnandre mentioned this pull request Apr 22, 2024

[TwigComponent] composer install fails in combination with twig/twig:3.9.X #1764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigComponent] Fix usage of {% embed %} with {% block %} in <twig:> components, close #952 #1397

[TwigComponent] Fix usage of {% embed %} with {% block %} in <twig:> components, close #952 #1397

Kocal commented Jan 16, 2024 •

edited

Loading

smnandre commented Jan 16, 2024

Kocal commented Jan 16, 2024

smnandre commented Jan 16, 2024

Kocal commented Jan 17, 2024 •

edited

Loading

weaverryan commented Jan 17, 2024

smnandre commented Jan 17, 2024

smnandre commented Jan 17, 2024

Kocal commented Jan 19, 2024

weaverryan left a comment

weaverryan Jan 19, 2024

smnandre Jan 19, 2024

Kocal Jan 20, 2024

weaverryan Jan 19, 2024

Kocal commented Jan 20, 2024 •

edited

Loading

smnandre commented Jan 20, 2024

weaverryan commented Jan 29, 2024

Kocal commented Jan 29, 2024

[TwigComponent] Fix usage of {% embed %} with {% block %} in <twig:> components, close #952 #1397

[TwigComponent] Fix usage of {% embed %} with {% block %} in <twig:> components, close #952 #1397

Conversation

Kocal commented Jan 16, 2024 • edited Loading

smnandre commented Jan 16, 2024

Kocal commented Jan 16, 2024

smnandre commented Jan 16, 2024

Kocal commented Jan 17, 2024 • edited Loading

weaverryan commented Jan 17, 2024

smnandre commented Jan 17, 2024

smnandre commented Jan 17, 2024

Kocal commented Jan 19, 2024

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Jan 19, 2024

Choose a reason for hiding this comment

smnandre Jan 19, 2024

Choose a reason for hiding this comment

Kocal Jan 20, 2024

Choose a reason for hiding this comment

weaverryan Jan 19, 2024

Choose a reason for hiding this comment

Kocal commented Jan 20, 2024 • edited Loading

smnandre commented Jan 20, 2024

weaverryan commented Jan 29, 2024

Kocal commented Jan 29, 2024

Kocal commented Jan 16, 2024 •

edited

Loading

Kocal commented Jan 17, 2024 •

edited

Loading

Kocal commented Jan 20, 2024 •

edited

Loading