Can't get twig filter in macros

[damsfx]

Winter CMS Build

1.2

winter/storm                        dev-wip/1.2 91993e2
winter/wn-backend-module            dev-wip/1.2 0258f85
winter/wn-cms-module                dev-wip/1.2 a998ed7
winter/wn-system-module             dev-wip/1.2 c5295ca
twig/twig                           v3.4.1

PHP Version

8.0

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

Getting an error when using |page twig filter in a macro.

An exception has been thrown during the rendering of a template ("Undefined array key "this"").

Steps to replicate

composer create-project wintercms/winter winter12 "dev-wip/1.2 as 1.2"
php artisan winter:install
php artisan winter:env
php artisan winter:up

Then add {{ 'plugins' | page }} in default layout or home page.
Getting link to plugins page is ok.

If the same filter is call from within a macro, it fails.
Add a macro to layout and calling it :

{% macro test() %}
    {{ 'plugins' | page }}
{% endmacro test %}
{% import _self as home %}

{{ home.test() }}

Fall in an error.

Workaround

No response

[RomainMazB]

I can confirm this bug, I am using this trick on a link macro to mark the link as the currently visited one:

{% macro dropdownLink(href, label, target = '_self') %}
    <li>
        <a href="{{ href }}"
           target="{{ target }}"
           class="dark:hover:bg-jacarta-600 hover:text-accent focus:text-accent hover:bg-jacarta-50 flex content-center rounded-xl px-5 py-3 transition-colors text-lg font-display text-sm leading-normal
            {% if (''|page == href) %}text-accent-dark dark:text-white{% else %}text-jacarta-700 dark:text-jacarta-200{% endif %}
            ">
            <span class="leading-none">{{ label }}</span>
        </a>
    </li>
{% endmacro %}

This issue occurred during migration from 1.1.8 to 1.2.

[RomainMazB]

I made some investigations:
This is probably due to the PR #455 by @LukeTowers.
As I understand, in this PR, he dissociated the Twig base environment from the cms extension.

Doing so caused the CMS environment to not "overlaps" the Twig's one, making the this context key not available anymore from the original Twig's MacroNode context here.

I made a dirty test to convince myself, replacing the twig package line linked previously by this one:

->write("\$context = \$this->env->mergeGlobals(array_merge(['this' => 'test'],\n")
// And another tiny-modification after that just to close the parenthesis

Doing so make the this key found (of course another error is raised because the controller key can't be found in the this array).

I looked into the Twig documentation and found how to add a global variable. I was able to fix this issue by modifying the initTwigEnvironment method from the cms Controller class:

protected function initTwigEnvironment()
{
    $this->twig = App::make('twig.environment.cms');
    $this->twig->addGlobal('this', ['controller' => $this]);
}

Unfortunately, I am not sure if this is the most efficient way to do, and if some other global variable should be injected to prevent other related issue?

[mjauvin]

Good find @RomainMazB !

Whether or not the global var is the right fix, at least we have a good pointer on where the problem lies.

[mjauvin]

@LukeTowers I confirm this resolves the problem I was having in this discussion:

https://discord.com/channels/816852513684193281/816852817267785758/988861851582464000

[mjauvin]

@LukeTowers @bennothommo @jaxwilko can you give your thoughts about the suggested fix from @RomainMazB above?

I can submit a PR for it in the morning.

Thanks.

[bennothommo]

On the outset it looks fine, but Luke's original PR was trying to avoid too many global attributes polluting the Twig environment.

And I'm presuming that the | page filter works outside the macro, so I'm not sure why it would not work within given that they're supposed to share the same scope.

[RomainMazB]

@mjauvin, actually I agree with @bennothommo on the fact that this is probably not the right fix (I would have directly submit a PR if I was not convinced of that).

But I disagree with @bennothommo too when he says that the macro share the same scope that the page or layout.
To me there is actually no more scope since the PR #455, all the page and layout variables are filled here:
https://github.com/wintercms/winter/blob/wip/1.2/modules/cms/classes/Controller.php#L309-L317
and then passed to the template renderer by parameter here:
https://github.com/wintercms/winter/blob/wip/1.2/modules/cms/classes/Controller.php#L419-L433
which means that the page and layout receive their context through the Twig's render method parameter, not as a scope.

The macro node doesn't even go through the Controller class: because macro is a Twig native feature, the macro is just compiled "as is" without nothing else but its parameters (and some global variables that we don't want to introduce anymore).

To me a good fix would be to create a custom MacroNode inside the cms module to modify the compiler rendering. And the same way the PageNode uses the extension pageFunction method which is actually just a "proxy" to the Controller's renderPage method, the MacroNode should rely on an extension macroFunction method which would call a Controller's renderMacro method which would inject these variables.

This is more or less how the partial node works, and so does the content and component nodes (without any context for contents and the component context for the components).

[LukeTowers]

Hmm, sounds interesting @RomainMazB, as long as @bennothommo doesn't have any objections I'd be interested in seeing a PR that implements those suggestions. Sounds like we need to also expand our testing suite a bit in this area as well.

[RomainMazB]

I'll wait for Ben's feedback before working on this. I could submit a PR including tests this week.

[mjauvin]

@bennothommo can you give your thoughts on this so that we can resolve this?

Thanks!

[bennothommo]

I'm happy for you guys to proceed with a PR. Might have a couple of comments then, but will be better to test things in action.

[LukeTowers]

@damsfx @RomainMazB @mjauvin @bennothommo I believe I've come up with an acceptable fix in 5b8d189, please test and verify

[mjauvin]

Well, looks like we have a winner!

This resolve the issue with the DynamicPDF plugin as well.

[damsfx]

This issue is still present in the 1.1.9 branch, as well as #557 .

Still need to downgrade twig/twig from v2.15.1 to v2.14.3.

[LukeTowers]

@damsfx which part of the issue? There were a few different bugs at play here, what error are you getting?

[damsfx]

@damsfx which part of the issue? There were a few different bugs at play here, what error are you getting?

@LukeTowers the issue due to calling page filter in macros seems to have gone after running composer update and a cache clean.

The only one remaining is calling the trans filter {{ 'anything' | trans }}

Object of class Winter\Storm\Foundation\Application could not be converted to string
P:\_Sites\_Labs\winter\vendor\twig\twig\src\Node\Expression\CallExpression.php line 304

[LukeTowers]

@damsfx apply wintercms/storm@1b866f0 and see if that fixes it. If it does then feel free to submit a PR to the 1.1 branch backporting that temporary fix and you can use the dev-1.1 version in your composer.json to pull it in after the PR is merged.

[damsfx]

@damsfx apply wintercms/storm@1b866f0 and see if that fixes it. If it does then feel free to submit a PR to the 1.1 branch backporting that temporary fix and you can use the dev-1.1 version in your composer.json to pull it in after the PR is merged.

Done :
wintercms/storm#98