Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TwigComponent] Impossible to declare higher level component content block #844

Closed
norkunas opened this issue May 4, 2023 · 24 comments · Fixed by #920
Closed

[TwigComponent] Impossible to declare higher level component content block #844

norkunas opened this issue May 4, 2023 · 24 comments · Fixed by #920

Comments

@norkunas
Copy link
Contributor

norkunas commented May 4, 2023

So I have a component A which defines it's content block. Then I have a higher level component B which also defines it and to forward to the A component, but Twig says: The block 'content' has already been defined line 6.

{# A.html.twig #}
<{{ element }} class="{{ classes }}"{{ attributes }}>
  {%- block content -%}{%- endblock -%}
</{{ element }}>
{# B.html.twig #}
<twig:A>
  {%- block content -%}{%- endblock -%}
</twig:A>
@WebMamba
Copy link
Collaborator

WebMamba commented May 4, 2023

Humm but it can be also that content is a reserved word since the new syntax. When you use the new syntax the content block is automatically added. 🤔

@norkunas
Copy link
Contributor Author

norkunas commented May 4, 2023

So I should use block('content') ?
Edit: no this way does not work (probably some loop goes on because got 504)

@norkunas
Copy link
Contributor Author

norkunas commented May 4, 2023

Works only this way:

{# A.html.twig #}
<{{ element }} class="{{ classes }}"{{ attributes }}>
  {%- block content -%}{%- endblock -%}
</{{ element }}>
{# B.html.twig #}
{% set passthrough %}{% block content %}{% endblock %}{% endset %}
<twig:A>
  {{ passthrough }}
</twig:A>

@weaverryan
Copy link
Member

I've created a more realistic example to help me not get confused :).

{# anywhere #}
<twig:DivComponent>
    Hello!
</twig:DivComponent>
{# DivComponent.html.twig #}
<twig:GenericElement element="div">
    {% block content %}{% endblock %}
</twig:GenericElement>
{# GenericElement.html.twig #}
<{{ element }} class="{{ classes }}"{{ attributes }}>
  {%- block content -%}{%- endblock -%}
</{{ element }}>

Is that accurate?

@WebMamba In my DivComponent.html.twig, are we taking into account the possibility that the user may define normal, Twig-style blocks - e.g. {% block content %}{% endblock %} - inside the HTML syntax? I think we're not. I think we're seeing {% block content and thinking its Twig "output" code, and so we're starting the block content. So we're pre-lexing like this:

Original
<twig:GenericElement element="div">
    {% block content %}{% endblock %}
</twig:GenericElement>

After pre-lexing:
<twig:GenericElement element="div">
    {% block content %}{% block content %}{% endblock %}{% endblock %}
</twig:GenericElement>

If I'm correct, before opening the default block, we need to check to make sure that the content doesn't begin with {% block (accounting for whitespace differences). And if it does, we need to consume until the corresponding endblock and ignore all of that (taking into account the possibility of nested start {% blocks.

@norkunas
Copy link
Contributor Author

norkunas commented May 4, 2023

Also found another problem. For form I override button_widget to leverage button component but the block inside it does not work:

{% block button_widget %}
<twig:Button ...>
{{- block('button_label_content') -}}
</twig:Button>
{% endblock %}

Block "button_label_content" on template "Component/Button.html.twig" does not exist.

But with this syntax it works:

{% block button_widget %}
  {{ component('Button', {
    children: block('button_label_content'),
  }) }}
{% endblock %}

@weaverryan
Copy link
Member

Does it work with this syntax?

{% block button_widget %}
  {% component Button with {
    children: block('button_label_content'),
  } %}
{% endblock %}

@norkunas
Copy link
Contributor Author

norkunas commented May 4, 2023

Will check it tomorrow.

@norkunas
Copy link
Contributor Author

norkunas commented May 5, 2023

Does it work with this syntax?

{% block button_widget %}
  {% component Button with {
    children: block('button_label_content'),
  } %}
{% endblock %}

yes it works

@weaverryan
Copy link
Member

perfect! Then @WebMamba we just have a pre-lexing problem here of some sort.

@WebMamba
Copy link
Collaborator

WebMamba commented May 5, 2023

Ok Great! I have a better understanding of the problem now! I am checking that! 😁

@WebMamba
Copy link
Collaborator

WebMamba commented May 5, 2023

So if I have a good understanding of the problem:

<twig:foo><p>My content</p>{% block bar %}<p>Inside bar</p>{% endblock %}</twig:foo>

should turn into:

{% component 'foo' %}{% block content %}<p>My content</p>{% endblock %}{% block bar %}<p>Inside bar</p>{% endblock %}{% endcomponent %}

@weaverryan
Copy link
Member

Ok, I checked into this a bit to help with #852. @WebMamba I think it's a bit more complicated... and we actually don't have a syntax yet that makes sense for this. Here is the setup, using a realistic example:

<twig:DangerButton>
    Danger will Robinson!
</twig:DangerButton>

So, I'm passing a content block into the DangerButton component. Inside DangerButton.html.twig, I want to READ my content block and use it to SET the content block on another component.

Let's forget about the HTML syntax for a moment, and just think about how this should look using the {% component %} syntax. I believe it should be:

{# DangerButton.html.twig #}

{% component 'Button' with { type: 'danger' } %}
    {% block content %}{{ block('content') }}{% endblock %}
{% endcomponent %}

So, we create the {% block content %} and in this case, we need to use {{ block() }} to actually render it... as it makes no sense to have a {% block content %} inside of another {% block content %}.

Unfortunately, as @norkunas mentioned, this creates recursion and the page times out. However, I think that this SHOULD be allowed. It IS allowed if you do something similar in normal Twig - i.e. outside of a {% component:

Normal template - this works fine - it prints the `content` block from the parent template
<div>
    {% block content %}{{ block('content') }}{% endblock %}
</div>

So, I think the fix is 2 parts:

A) Fix the {% component %} syntax code so that {% block content %}{{ block('content') }}{% endblock %} works, by outputting the parent content block. I have no idea how difficult this may be :p

B) Fix the <twig: syntax so that it does NOT try to open the content block automatically if the content IS a block. We fixed this recently when you use <twig:block name="foo" but apparently not using the normal {% block foo %} syntax. So:

<twig:Button type="danger">
    {% block foo %}Foo{% endblock %}
</twig:Button>

This currently becomes:

{% component 'Button' with { type: 'danger' } %}
    {% block content %}{% block foo %}Foo{% endblock %}
{% endblock %}{% endcomponent %}

But it should just be:

{% component 'Button' with { type: 'danger' } %}
    {% block foo %}Foo{% endblock %}
{% endcomponent %}

@weaverryan
Copy link
Member

A bit more thinking. I believe we need to change our {% component %} syntax to more exactly match waht https://github.com/giorgiopogliani/twig-components originally did - using these "slot" variables instead of blocks. Blocks seemed like a good idea, as they're a familiar concept. However, they cause situations that just don't make sense. For example:

{# someTemplate.html.twig #}
{% block foo %}I am the foo block!{% endblock %}

{% component Alert %}
    {% block content %}
        {{ block('foo') }}
    {% endblock %}
{% endcomponent %}

It's obvious what should happen here: the text I am the foo block! should be printed inside of the content block of the component. In reality, you get this error:

Block "foo" does not exist on Alert.html.twig. The current system leverages Twigs {% embed %} syntax, and it does some serious gymnastics internally to get the block inheritance thing working. And it ultimately causes problems that should not exist. Using https://github.com/giorgiopogliani/twig-components more directly will solve these.

Probably we should deprecate our {% component %} and implement giorgiopogliani's more directly under a new syntax - e.g. {% twig_component %} (though I think we should likely not document this new Twig tag and instead just use it internally from our <twig:Component> syntax.

@WebMamba
Copy link
Collaborator

Ok, I see what you mean! Thanks for sharing your thought @weaverryan! When I was working on the fix, I feel two things:

  • first, we fight too much with Twig: I think the best solution should be to integrate the preLexer logic directly into the Twig Lexer, or (I already ask Fabien a few months ago and he doesn't want to) to make the Twig Lexer extendable and extend it.
  • second Twig block and component block are two separate things trying to hardly leave together, so having two different syntaxes is also for me the way to go. And by separating these two notions we gonna be able to give more power related to Components to the "component block"

I like this library as well https://github.com/giorgiopogliani/twig-components, but why deprecated the entire component tag, and why not just add a slot tag instead?

@weaverryan
Copy link
Member

weaverryan commented May 11, 2023

first, we fight too much with Twig: I think the best solution should be to integrate the preLexer logic directly into the Twig Lexer, or (I already ask Fabien a few months ago and he doesn't want to) to make the Twig Lexer extendable and extend it.

Yes, but that will take some time and thought. So... we should do it later ;).

I like this library as well https://github.com/giorgiopogliani/twig-components, but why deprecated the entire component tag, and why not just add a slot tag instead?

I believe that the entire way that giorgiopogliani is built is different. Even if we added a slot tag, the fact that the {{ block() }} functionality is "broken" in our current implementation - #844 (comment) - means that, I think, we should not try to keep using it. My thinking is that it's better to deprecate our existing {% component %} tag and start over. Updating the PreLexer to change to some other {% twig_component %} syntax will be trivial.

Also, see #863 for another example where I think our {% component is misbehaving.

@sneakyvv
Copy link
Contributor

sneakyvv commented May 12, 2023

It's obvious what should happen here: the text I am the foo block! should be printed inside of the content block of the component. In reality, you get this error:

Block "foo" does not exist on Alert.html.twig.

The current system leverages Twigs {% embed %} syntax, and it does some serious gymnastics internally to get the block inheritance thing working.

@weaverryan As I also commented on #863 (comment), I think you should always frame components within the "all components exist in their own isolated universe" paradigm (cfr. ux-live docs).

Given that each component renders separately, it's absolutely normal that the foo block in your example does not exist within the "Alert-universe". Moreover, even if you would alter the compiler for the ComponentNode so that it passes the blocks from the parent template to the Alert component, they would still be rendered twice, as there is no parent-child relation with embedded components. It's a separate render.

That's also why the code of @norkunas in #844 (comment) works.

{% set passthrough %}{% block content %}{% endblock %}{% endset %}
<twig:A>
  {{ passthrough }}
</twig:A>

What set does, is adding passthrough to the context that the A component will render with. This is what happens in the compiled code (some Template class):

$context["passthrough "] = <the block content>;
//...
$props = $this->extensions["Symfony\\UX\\TwigComponent\\Twig\\ComponentExtension"]->embeddedContext("A", twig_to_array([]), $context);
        $this->loadTemplate("a.html.twig", ...)->display($props);
        /

Without that passing along of that information A has no idea that it's being rendered as part of the surrounding template. It's not in the context.
That's why in #863 it's necessary to add the host to the context. Something similar should happen for the blocks. You could pass the parent scope $blocks as a second argument to display in the compiled code above, but then again, you still have the problem that the block will also be rendered where it's defined, as they don't get merged, since, again, there's no parent-child relation. These template's are not extending, they're embedding.

As for the very first example in this issue:

<twig:A>
  {%- block content -%}{%- endblock -%}
</twig:A>

That doesn't make sense to me since this effectively is: "use a block named content as the (by default also named) content block of A". So this inevitably renders to {%- block content -%}{%- block content -%}{%- endblock -%}{%- endblock -%} which is invalid, and can't, or shouldn't be fixed imo.
To be able to do what @norkunas wants to do you should indeed just store the block in a variable and use that, or simply name the block to something else. For example, starting from @weaverryan's clearer example, you could do:

{# anywhere #}
<twig:DivComponent>
    <twig:block name="genericContent">Hello!</twig:block>
</twig:DivComponent>
{# DivComponent.html.twig #}
<twig:GenericElement element="div">
    {% block genericContent %}{% endblock %}
</twig:GenericElement>
{# GenericElement.html.twig #}
<{{ element }} class="{{ classes }}"{{ attributes }}>
  {%- block genericContent -%}{%- endblock -%}
</{{ element }}>

Also not ideal, since you lose the HTML-feel a bit here, but I see no other way, since using the content block inside a <twig:component> tag is just recursion by definition.

@weaverryan
Copy link
Member

@sneakyvv I think there's a philosophical decision to be made here about the "universe" aspect. The docs state:

As a rule of thumb, each component exists in its own, isolated universe

This was meant to refer to how the components behave and re-render on the page. Inside the templates, if we're inside of Foo.html.twig, I believe that 100% of that template should behave like I'm inside the Foo template, even if I'm rendering an embedded component. That includes blocks and variables. So the desired behavior is:

AlertGreeting.html.twig

{% block greeting %}Hello!{% endblock %}
{{ dump(this) }} Dumps Foo

{% component Alert %}
    {{ dump(this) }} Dumps Foo

    {{ block('greeting') }} Prints "Hello!"
{% endcomponent %}

This is, I believe, most similar to frontend frameworks. For example, in React, the equivalent might be:

// AlertGreeting.jsx
export default class extends Component {
    greeting = "Hello!";
    
    render() {       
        return (
            <Alert>{this.greeting}</Alert>
        );
    }
}

In this case, this inside of render() refers to the AlertGreeting object. It wouldn't make sense for this to refer to Alert or for us to have access to the Alert props from inside there.

@sneakyvv
Copy link
Contributor

@weaverryan I know the universe-rule actually applies to the live components, but, how the current rendering of a component is working, you can think of the (current) behavior in the same way. Because components are embedded, and their blocks are living inside the universe of the component they're defined in, not in the universe of the component they're being embedded in.

I don't think it is even possible to override a block from outside a component, without completely reworking the syntax/compiler, since they are not being used in an extend-flow, but an embed-flow. Or am I overlooking something?
It is possible by using variables, as I and @norkunas have shown, because they are just added to the context which is then available in the embedded component.

Note the "without completely reworking the syntax/compiler", which is obviously also an option 😄

@sneakyvv
Copy link
Contributor

sneakyvv commented May 12, 2023

It is possible by using variables

But not in a general, because how would you tell if a block is supposed to extend/override a parent block, or whether it should be used as value for a block in a component?

{#  base.twig #}
...
{% block base_block %}base version{% endblock %}
...
{#  someFile.twig #}
{% extends base.twig %}

{% block base_block %}override base{% endblock %}
{% block foo_block %}I am the foo block{% endblock %}

<twig:componentA>
    <div>{{ block('foo_block') }}</div>
</twig>

Should I am the foo block be rendered in place, or inside the component's div, or both? (both is simple, just pass blocks to display() in ComponentNode::compile())

@weaverryan
Copy link
Member

Should I am the foo block be rendered in place, or inside the component's div, or both? (both is simple, just pass blocks to display() in ComponentNode::compile())

That's why I think we should stop using "blocks" for passing content into components - it's doing 2 jobs at once and is confusing and has weird questions like this #844 (comment)

I don't think it is even possible to override a block from outside a component, without completely reworking the syntax/compiler, since they are not being used in an extend-flow, but an embed-flow. Or am I overlooking something?

Exactly - so let's move away from using blocks for this! And let blocks be blocks - use a new "slot" idea like in https://github.com/giorgiopogliani/twig-components#usage

@sneakyvv
Copy link
Contributor

sneakyvv commented May 12, 2023

Completely agree, but just to be clear: even with slots it won't be possible to pass outside-slots in, am I right?
I mean slots outside of the components tags. Cause, why would you even?

@weaverryan
Copy link
Member

Completely agree, but just to be clear: even with slots it won't be possible to pass outside-slots in, am I right?
I mean slots outside of the components tags. Cause, why would you even?

Are you thinking something like this? (Syntax is up for debate - but just focus on the idea):

SomeTemplate.html.twig

{% component AlterGreeting %}
    {% slot:title %}Hello!{% endslot %}
{% endcomponent %}
AlertGreeting.html.twig

{% component Alert %}
    {% slot:content %}{{ title }}{% endslot %}
{% endcomponent %}

Or are you thinking something else? I wasn't totally sure what you meant by "I mean slots outside of the components tags". With the above, it should be easy to grab the slot from one component and "forward" it into the next component.

weaverryan added a commit that referenced this issue May 12, 2023
…andled correctly (weaverryan)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[TwigComponent] Fixing bug where traditional blocks aren't handled correctly

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Tickets       | Fixes part of #844 (comment)
| License       | MIT

Using `{% block traditional_block %}` inside of a `<twig:Component` syntax should now work. However, even though I tried to make the TwigPreLexer a bit more intelligent than just a regex parser, it's reaching its limits of complexity. Even this fix will break down if the user adds extra whitespace - e.g. `{%       block traditional_block %}`. It's likely that `TwigPreLexer` will need to be converted to an actual Lexer -> token stream -> parser type of a system. On the bright side, that would make it easier to integrate into Twig core if we ever chose to do that (the parser wouldn't convert over directly, but the lexer & tokens in theory would).

Cheers!

Commits
-------

2ca9430 [TwigComponent] Fixing bug where traditional blocks aren't handled correctly
@sneakyvv
Copy link
Contributor

Yes, that syntax is perfectly fine!
I was referring to doing something 'outside' the component like your example in #844 (comment). Forcing it like that doesn't make sense to me, while your last example is much better, for the same effect actually. In that last example everything is in the scope of the component(s). I like where this is going!

@sneakyvv
Copy link
Contributor

@weaverryan see #863 (comment) as to why I believe a new slot syntax (which equals to an include flow instead of embed) is not the solution for this PR, as it would let forgo a much bigger advantage of using embed.

symfony-splitter pushed a commit to symfony/ux-twig-component that referenced this issue Jun 23, 2023
…d components (sneakyvv)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[TwigComponent] Support passing blocks to nested embedded components

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Tickets       | Fix #844
| License       | MIT

The fix for #844, and a better solution then a manual passthrough workaround, is literally just passing the blocks from the "host Template" to the embedded Template, ~~so that the blocks are merged with the embedded template's blocks, overriding them~~ but altering their name and using a special `outerBloocks` variable to map the block name to the new name (see symfony/ux#920 (comment)).

This means that the example from symfony/ux#844 (comment) (tweaked a bit)

```twig
{# anywhere #}
{% set name = 'Fabien' %}
<twig:DivComponent>
    Hello {{ name }}!
</twig:DivComponent>
```

```twig
{# DivComponent.html.twig #}
<twig:GenericElement element="div" class="divComponent">
    {{ block(outerBlocks.content) }}
</twig:GenericElement>
```

```twig
{# GenericElement.html.twig #}
<{{ element }}{{ attributes }}>
  {%- block content -%}{%- endblock -%}
</{{ element }}>
```

Results in

```twig
<div class="divComponent">Hello Fabien!</div>
```

See the tests for more elaborate examples showing the access to context variables, multi-level overrides, etc.

Commits
-------

f4b064ca [TwigComponent] Support passing blocks to nested embedded components
web9app6 added a commit to web9app6/ux that referenced this issue Aug 1, 2023
…andled correctly (weaverryan)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[TwigComponent] Fixing bug where traditional blocks aren't handled correctly

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Tickets       | Fixes part of symfony/ux#844 (comment)
| License       | MIT

Using `{% block traditional_block %}` inside of a `<twig:Component` syntax should now work. However, even though I tried to make the TwigPreLexer a bit more intelligent than just a regex parser, it's reaching its limits of complexity. Even this fix will break down if the user adds extra whitespace - e.g. `{%       block traditional_block %}`. It's likely that `TwigPreLexer` will need to be converted to an actual Lexer -> token stream -> parser type of a system. On the bright side, that would make it easier to integrate into Twig core if we ever chose to do that (the parser wouldn't convert over directly, but the lexer & tokens in theory would).

Cheers!

Commits
-------

2ca9430a [TwigComponent] Fixing bug where traditional blocks aren't handled correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants