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

Macro arguments using leading/tailing underscore (BC introduced in v3.15) #4452

Closed
kampalex opened this issue Nov 19, 2024 · 4 comments · Fixed by #4475
Closed

Macro arguments using leading/tailing underscore (BC introduced in v3.15) #4452

kampalex opened this issue Nov 19, 2024 · 4 comments · Fixed by #4475

Comments

@kampalex
Copy link

Hi. I discovered a side effect after upgrading to the latest version, related to this line in PR 4391.
For me, it's a BC, which may affect others as well.
The usage of macro arguments having leading/tailing underscores worked well until 3.15.

Example:

{% macro testing(_message) %}
The message: '{{ _message }}'.
{% endmacro %}
{{ _self.testing('Hello World!') }}

Output:
3.14: The message: 'Hello World!'.
3.15: The message: ''.

Compiled template:
In the referred line of the PR, underscores are trimmed off from arguments as they are assigned onto $context. _message becomes message, but the original _message is requested in the escape-method later. That's an error.

[...]
class __TwigTemplate_[...] extends Template
[...]
    // line 1
    public function macro_testing($_message = null, ...$varargs): string|Markup
    {
        $macros = $this->macros;
        $context = [
            "message" => $_message,
            "varargs" => $varargs,
        ] + $this->env->getGlobals();

        $blocks = [];

        return ('' === $tmp = \Twig\Extension\CoreExtension::captureOutput((function () use (&$context, $macros, $blocks) {
            // line 2
            yield "The message: '";
            yield $this->env->getRuntime('Twig\Runtime\EscaperRuntime')->escape(($context["_message"] ?? null), "html", null, true);
            yield "'.
";
            yield from [];
        })())) ? '' : new Markup($tmp, $this->env->getCharset());
    }

I'm curious how I should see the code change. If this is the intention, maybe a trigger_deprecation should be called while evaluating the macro arguments.
I just wanted to let you know - the affected templates will be fixed on my side.

@kampalex kampalex changed the title Macro arguments having leading/tailing underscore (BC introduced in v3.15) Macro arguments using leading/tailing underscore (BC introduced in v3.15) Nov 19, 2024
@stof
Copy link
Member

stof commented Nov 19, 2024

@fabpot what was the reason to trim _ in the names ?

@kampalex
Copy link
Author

In v3.14.2...v3.15.0 diff:

File doc/coding_standards.rst:
Line 91 | Old: * Use lower cased and underscored variable names:
Line 91 | New: * Use snake case for all variable names (provided by the application and created in templates):
[....]
Added: * Use snake case for all function/filter/test names:

....Okay! This explanation has clarified my issue.

The compiled template looks much cleaner without the __ wrap per macro argument:
3.14: public function macro_testing($___message__ = null, ...$__varargs__)
3.15: public function macro_testing($_message = null, ...$varargs): string|Markup
According to the doc changes, _message as macro argument is deprecated.

I think the reason for trim is RESERVED_NAMES which are wrapped using '_'.

@mathieu-dumoutier
Copy link

Hello. I have an error too when I upgraded 3.14.2 to 3.15

An exception has been thrown during the rendering of a template ("Warning: Undefined variable $_v0"). on each rendered of easyadmin's crud routes (4.15.1)

Like the missing variable is $_v0 with an underscore, I think is may be in relation with this issue. It seems to appear with |trans filter.

Capture d’écran 2024-11-25 à 08 23 21

@fabpot
Copy link
Contributor

fabpot commented Nov 25, 2024

#4475 should fix this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants