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

Macros defined in outside scope are not considered used by UnusedMacro when used inside a macro #180

Open
sustmi opened this issue Jul 2, 2021 · 1 comment
Labels

Comments

@sustmi
Copy link

sustmi commented Jul 2, 2021

Issue #27 says:

The body of a macro is a fully isolated scope. So an import done inside a macro must be checked for usage only inside the macro. And an import done outside the macro must look for usages outside the macro too (finding a usage inside the macro is not valid, as it cannot reference the macro).

However, the current Twig documentation says:

Imported macros are always local to the current template. It means that macros are available in all blocks and other macros defined in the current template, but they are not available in included templates or child templates; you need to explicitly re-import macros in each template.

https://twig.symfony.com/doc/3.x/tags/macro.html#macros-scoping

This means that the following code is valid use of foo macro:

{% import "foo.html.twig" as foo %}
{% macro bar() %}
    {{ foo.stuff() }}
{% endmacro %}

However, the UnusedMacro thinks that foo macro is not used and issues the following violation:

l.1 c.29 : ERROR Unused macro import "foo".

Additional notes:

I found this comment in the Scope class:

/**
 * When isolated, a scope won't be explored when looking for name usages.
 */
public function isolate()
{
    $this->isolated = true;
}

* When isolated, a scope won't be explored when looking for name usages.

However this should not be true when looking for usages of macros.

Steps to reproduce

I created a failing test case (that should be passing) here: https://github.com/sustmi/twigcs/blob/6965bad2494b811c54030eed8a7b3682beb8851c/tests/Twig3FunctionalTest.php#L258

  1. git clone https://github.com/sustmi/twigcs/
  2. cd twigcs
  3. git checkout unused-macro-issue-180
  4. composer install
  5. phpunit tests/Twig3FunctionalTest.php
sustmi added a commit to sustmi/twigcs that referenced this issue Jul 2, 2021
@sustmi
Copy link
Author

sustmi commented Jul 2, 2021

There is one noteworthy behavior.

One would think that in this code:

{% block body %}
    {% import "foo.html.twig" as foo %}
    {% macro bar() %}
        {{ foo.stuff() }}
    {% endmacro %}

    {{ _self.bar() }}
{% endblock %} 

the foo macro would be accessible from bar macro context, but Twig runtime says that foo macro does not exist.

I think that the devil lies in detail. The Twig documentation says (emphasis is mine):

When calling import or from from a block tag, the imported macros are only defined in the current block and they override macros defined at the template level with the same names.

This probably means that macro imported inside block tag is only defined in the block itself and not in sub-scopes. Or in another words: the visibility of imported macro in block or macro tags depends on whether the import is done in top-level scope or inside block scope. Not very intuitive I would say.

If I import the macro outside the body block, the code works:

{% import "foo.html.twig" as foo %}

{% block body %}
    {% macro bar() %}
        {{ foo.stuff() }}
    {% endmacro %}

    {{ _self.bar() }}
{% endblock %} 

However, that does not explain why this code works:

{% block body %}
    {% import "foo.html.twig" as foo %}
    {% set arr = [1,2] %}
    {% for item in arr %}
        {{ foo.stuff() }}
    {% endfor %}
{% endblock %}

because for loop also has its own scope (see the closing note here: https://twig.symfony.com/doc/3.x/tags/set.html) but the foo macro is accessible in this case.

I guess that for scope is not the same as macro or block scope because when I substitute the for loop with block, this code does not work:

{% block body %}
    {% import "foo.html.twig" as foo %}
    {% block bar %}
        {{ foo.stuff() }}
    {% endblock %}
{% endblock %} 

while the following code works (ie. the behavior is the same as with macro tag inside block):

{% import "foo.html.twig" as foo %}

{% block body %}
    {% block bar %}
        {{ foo.stuff() }}
    {% endblock %}
{% endblock %} 

@OwlyCode OwlyCode added the bug label Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants