symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
837 stars 307 forks source link

lint:twig error, possibly with twig component #1306

Closed tacman closed 10 months ago

tacman commented 11 months ago

I've opened an issue at https://github.com/symfony/symfony/issues/52778, but posting here because I think it may have to do with symfony/ux-twig-component.

I wonder if I should actually post this to twigphp/twig?

I'm pretty sure it's a bug that was recently introduced, as I think the template referenced used to work. @kbond , can you take a quick peek and at least tell me where to post this issue?

smnandre commented 11 months ago

I'm not sure, but are you using <twig:block in a non-embeded context ?

<twig:block is not equivalent to the {% block %} tag, it's only usable inside a embeddable Twig Component.

The block tag may be used in include / extends context for example, <twig:block cannot

If i missunderstood your usage please tell me :)

tacman commented 11 months ago

Hmm, I've been using it as a replacement for {% block %}, but perhaps it was coincidental that it used to work.

I suspect that if I cleaned up the mess of blocks (obviously, I was in the middle of migrating to the new style), I could get it to work. But certainly the lint:twig command should point me to a hint as to where the problem is.

smnandre commented 11 months ago

That is not related to the lint:twig command but to our implementation of the TwigComponent custom PreLexer.

I must admit that, personnaly, have a lot to do before i spend time on improving this particular case, but let's see next weeks/months if this becomes a common problem

Thank you for finding / submitting all those issues: it really helps improving the UX components !

tacman commented 11 months ago

I fixed it with

        if (strlen($this->input) >= $this->position) {
            return false;
        }

And got the error message I was expecting. Okay to submit a PR?

smnandre commented 11 months ago

PR are always welcomed :)

smnandre commented 10 months ago

But i need to insist, for you to not be surprised if (when?) that happens...

The <twig:block > syntax is currently uniquely supported inside embedded components.

It "works" by chance for you there, but this is not something intented and could stop to work at any time

smnandre commented 10 months ago

In fact, if you replace your and replace it by {% block gridTable %} ... no more errors :)

And i think I understand why and ... what need to be fixed because it could have other consequences

So i'm really sorry but you found a bug that... when corrected, may also stop accepting this outside-component behaviour :/

tacman commented 10 months ago

@smnandre

In my templates that extend base.html.twig, I'm able to use either of these (but not both at the same time!) to define the title block.

{% block title %}Officials{% endblock %}
<twig:block name="title">Officials</twig:block>

I just like the syntax better, I think it looks cleaner. But thanks for the heads-up that it might not work. I have a bundle that totally depends on these twig blocks that appears to have broken in the latest dev version, based on this off-book behavior. Uh oh!

tacman commented 10 months ago

Argh. Here's the use case that is now failing with the dev branch of ux. I have a component that renders a table. Each table column can be rendered within its own block, matching the name of the variable.

    {% component grid with {
        data: data,
        columns: [
        'id',
        {name: 'birthday', sortable: true},
    } %}

        <twig:block name="birthday">
            {{ row.birthday|date('Y-m-d') }}
        </twig:block>
    {% endcomponent %}

Within the component, it renders the value by default, but if a block (within the component) exists, it uses that block to render the value. It works great.

There are other issues, but this seems like it should work, since it's a twig:block within a component.

I

tacman commented 10 months ago

There's some change in the dev branch that handles parsing blocks differently.

Here's another example.

<twig:grid :data="data" tableId="example" :columns="columns" useDatatables="false">        
    <twig:block name="id">            {{ row.id }}        </twig:block>    
</twig:grid>

My component passes "row" to the block to be rendered.

https://github.com/survos/SurvosApiGridBundle/blob/main/templates/components/grid.html.twig#L41-L56

 {% set blockParams = {field_name: c.name, row: row, column: c, c:c } %}

                            {% with blockParams %}
                                {{ block(templateName) }}
                            {% endwith %}

I realize that this is an edge case, defining blocks that are effectively templates, but it's worked great! In fact, these same templates can be compiled to javascript (via twigjs) and used in dynamic datatables. It's actually really cool, to define dynamic rendering for datatables in twig.

I love this ability. Please consider allowing it. Thanks.

tacman commented 10 months ago

Sorry if this is redundant, but I'm starting to panic. The following works with the current release of ux-twig-component, but the twig:grid fails with the dev branch:

    <h3>using component</h3>
    {% component 'grid' with {
        data: data,
        columns: columns
    } %}
        {% block title %}
            <i>{{ row.title }}</i>
        {% endblock %}
    {% endcomponent %}

    <h3>using twig:grid</h3>
    <twig:grid :data="data" tableId="example" :columns="columns">
        <twig:block name="title">
            <i>{{ row.title|default('throws a compile error without default') }}</i>
        </twig:block>
    </twig:grid>
smnandre commented 10 months ago

I'm gonna fix the block bug to start, and we'll see where it goens from here.

There is something wrong with the Lexer

<twig:block name="foo">
    {#  {% block a %} #}
</twig:block>

This should throw an exception, not a strlen error.

tacman commented 10 months ago

[removed, irrelevant]

smnandre commented 10 months ago

I don't know how you use the block tag outside the template you linked, but the only way {% if block(templateName) is defined %} is if the block is defined in the template or a parent, right ?

https://twig.symfony.com/doc/3.x/functions/block.html

smnandre commented 10 months ago

Brainstorming: how about a property on the twig block that defers compiling? Something like the html template tag (https://www.w3schools.com/tags/tag_template.asp)

I don't love that idea, but all my applications use this bundle, so I'm hoping it continues to work! I'm a huge fan of twig components.

Not sure to understand how that'd work, and how this is related to twig components ?

tacman commented 10 months ago

The block outside of the component was just a nicer looking syntax, I can live without it.

But I don't think the Lexer should check for undefined variables within the component blocks, because the component itself may be defining the variables to render. Perhaps this will go back to the previous behavior after you fix the block bug.

tacman commented 10 months ago

whew, I can stop panicking now. I'm sure you'll fix this, as the behavior I'm describing is what you describe in the documentation, and currently isn't parsing:

<twig:SuccessAlert>
    <twig:block name="alert_message">
        I can override the alert_message block and access the {{ message }} too!
    </twig:block>
</twig:SuccessAlert>

image

smnandre commented 10 months ago

Is there a variable "message" if you dump it ligne 23 ?

If you dump it ligne 24 after the opening tag ?

smnandre commented 10 months ago

But I don't think the Lexer should check for undefined variables within the component blocks

It does not.. so this is something else (not saying there isnt a bug somewhere ;) )

tacman commented 10 months ago

Is there a variable "message" if you dump it ligne 23 ?

If you dump it ligne 24 after the opening tag ?

No -- message is defined in the component and passed to the template from there.

smnandre commented 10 months ago

Wich component ? I try to understand :)

SuccessAlert has a message property that what you mean ? Where is it defined ?

tacman commented 10 months ago

This fails in the dev version, but not in the live version.

    <twig:alert message="abc" dismissible="true">
        <twig:block name="alert_message">
            I can override the alert_message block and access the {{ message }} too!
        </twig:block>
    </twig:alert>

The component html template is at https://github.com/survos/BootstrapBundle/blob/main/templates/components/alert.html.twig

It's a very simple component -- but there's definitely an issue where the twig compiler is compiling the blocks and throwing errors when missing a variable, which doesn't make sense because the variable is coming from the component code. At the point of compiling the block, it can't possibly have access to the context (I think that's the right word.)

smnandre commented 10 months ago

The error you see it at runtime, not at compile time.

Where is your block "alert_message" in your alert template ?

You'd need one if you want to use a block like this inside a embedded component

tacman commented 10 months ago

I was just cutting and pasting from the example. It fails with the example on the ux twig component documentation page, even though SuccessAlert doesn't even exist. That's why I think it's failing at compile time.

In the release version, it compiles then throws an error that the component doesn't exist, as it should.

smnandre commented 10 months ago

I'd like to help you, but i need a concrete, reproductible exemple of non-working behaviour.

Not the lint:twig, as i said i'm fixing it, but you said

It's a very simple component -- but there's definitely an issue where the twig compiler is compiling the blocks and throwing errors when missing a variable

And i just cannot help you if i cannot reproduce / understand the situation.

tacman commented 10 months ago

Sure, I'll wait until you fix the block bug, just in case it's related. If it still exists, I'll create the SuccessAlert component that you describe in the documentation and see if I can reproduce the error using that.

On Wed, Nov 29, 2023 at 12:44 PM Simon @.***> wrote:

I'd like to help you, but i need a concrete, reproductible exemple of non-working behaviour.

Not the lint:twig, as i said i'm fixing it, but you said

It's a very simple component -- but there's definitely an issue where the twig compiler is compiling the blocks and throwing errors when missing a variable

And i just cannot help you if i cannot reproduce / understand the situation.

— Reply to this email directly, view it on GitHub https://github.com/symfony/ux/issues/1306#issuecomment-1832411555, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEXIQJTGDAFAN4P663UL2TYG5X7LAVCNFSM6AAAAAA76KTSVGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZSGQYTCNJVGU . You are receiving this because you authored the thread.Message ID: @.***>

tacman commented 10 months ago

OK, starting in the same directory as the ux repo.

symfony new bug --webapp --version=6.4 && cd bug
composer require symfony/ux-twig-component symfony/asset-mapper 
composer req survos/bootstrap-bundle
echo "import 'bootstrap/dist/css/bootstrap.min.css'" >> assets/app.js

bin/console make:controller App
cat > templates/app/index.html.twig <<'END'
{% extends 'base.html.twig' %}
{% block body %}
<twig:alert message="hello" dismissible="true">
        <twig:block name="alert_message">
            I can override the alert_message block and access the {{ message }} too!
        </twig:block>
    </twig:alert>
{% endblock %}

END

symfony server:start -d
symfony open:local --path=/app

As expected image

Now use the dev ux

cd ../ux
./link ../bug
cd ../bug
symfony open:local --path=/app

The bootstrap stuff isn't really necessary, but since it's an alert, it's easier to see that it's working. Regardless, it doesn't even compile using the dev version of ux-twig-component

image

tacman commented 10 months ago

All that work putting together a beautiful bug report, but it took so long that you fixed it! THANKS! Looks like it's working now. I'll continue to test.

tacman commented 10 months ago

Yep, it's working as expected within my bundle :-)

FWIW, the ability to define blocks outside of the twig component is working again, even though I realize there's no guarantee it will continue to work, so I won't use it.

{% block body %}
{#    {% block title %}YY{% endblock %}#}
    <twig:block name="title">XX</twig:block>
    {{ block('body_content') }}
{% endblock %}
smnandre commented 10 months ago

It is sadly not as expected.

Can only work if you have a declared block "alert_message" defined in the alert template.
smnandre commented 10 months ago

Yep, it's working as expected within my bundle :-)

FWIW, the ability to define blocks outside of the twig component is working again, even though I realize there's no guarantee it will continue to work, so I won't use it.

{% block body %}
{#    {% block title %}YY{% endblock %}#}
    <twig:block name="title">XX</twig:block>
    {{ block('body_content') }}
{% endblock %}

I really have to insist, you should not consider it for the future, as it works "by chance" as i said, and is not the expected behaviour. Why don't you use the include / embed tags for this ? Is there something specific you have with the twig component that you do not with twig ?

tacman commented 10 months ago

In fact, I brought that exact issue up a few months ago: https://github.com/symfony/symfony/discussions/51755. I hate twig macros, I can't imagine why I'd want to clunky twig macro when I can have an elegant twig component. Embeds make sense, but components are again more elegant.

I also like rendering blocks with a 'with' tag, that doesn't seem to be very common though.

Certainly I will not intentionally define blocks outside the component scope that way, as you've encouraged. Thanks for the heads-up.

I still struggle with an elegant solution for page markup, none of the current solutions are an obvious "right" way to do it. A topic for another thread.

Thanks again for your work on this. I'm sorry I panicked, and wish I had identified a clear bug report much earlier.

smnandre commented 10 months ago

The "with" is on my TODO list but i cannot guarantee you anything before a couple of months, as i have other things on my plates right now, but i agree with you !

I understand your needs (i think) but you are 2 years in the future i think ^^

Thank you again for all the bug reports ;)

tacman commented 10 months ago

@kbond and I talked about "scaffolding" in another thread. I like the term.

Bundles are not supposed to be full-fledged apps, but for a theme bundle, it's practically one. With AssetMapper, though, finally a theme with front-end assets can be installed as a bundle without committing to a whole framework (like EasyAdmin).

I have a menu component that works with bootstrap and makes creating sites much easier. Now I'm playing around with an entire page component, that works like embed (navbar, sidebar, page header, page content, footer). I'm sure there's a better approach, page layout is tricky. I often find a theme that I like somewhere, all HTML and CSS, then spend a lot of time trying to convert it to twig.

tacman commented 10 months ago

but you are 2 years in the future i think ^^

Turns out that 2 years from today will likely be the release of Symfony 8! :-)