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

Twig 2.11 is a breaking change - and still it is a minor release #3090

Open
ericmorand opened this issue Jul 22, 2019 · 12 comments
Open

Twig 2.11 is a breaking change - and still it is a minor release #3090

ericmorand opened this issue Jul 22, 2019 · 12 comments
Labels

Comments

@ericmorand
Copy link
Contributor

ericmorand commented Jul 22, 2019

The documentation explictly says so:

Before Twig 2.11, it was possible to use macros imported in a block in a "sub-block". When upgrading to 2.11, you need to either move the import in the global scope or reimport the macros explicitly in the "sub-blocks".

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

A template written pre-2.11 is not guaranteed to render to the same result - or even compile successfully - using 2.11. This is a breaking change.

@stof
Copy link
Member

stof commented Jul 22, 2019

Can you share a reproducer (using https://twigfiddle.com/ for instance) ?

@ericmorand
Copy link
Contributor Author

Here is a fiddle that works with Twig 2.10.0 but fails with Twig 2.11.0:

https://twigfiddle.com/ea4eqj

It implements exactly what is in the doc:

Before Twig 2.11, it was possible to use macros imported in a block in a "sub-block". When upgrading to 2.11, you need to either move the import in the global scope or reimport the macros explicitly in the "sub-blocks".

@simshaun
Copy link
Contributor

simshaun commented Aug 5, 2019

Just ran into this. My use case that broke on 2.11 (dumbed down):
https://twigfiddle.com/1oy6q4

{% block body_html -%}
  {% import 'macros.twig' as macros %}
 
  {% block header_wrap %}
    {{ macros.spacer(16) }}
    Header
    {{ macros.spacer(32) }}
  {% endblock %}
  
  {% block footer_wrap %}
    {{ macros.spacer(32) }}
    Footer
    {{ macros.spacer(16) }}
  {% endblock %}
{%- endblock %}

@fabpot fabpot added the Bug label Aug 7, 2019
@althaus
Copy link
Contributor

althaus commented Sep 3, 2019

I've just had an issue with this, too:

{# macros.html.twig #}
{% macro foo %} ... {% endmacro %}
{# base.html.twig #}
{% import 'macros.html.twig' as m %}
{# page.html.twig #}
{% extends 'base.html.twig' %}
{% import _self as m %}

{{ m.foo }}

fails with Twig 2.11.3 complainig about Macro "foo" is not defined in template "page.html.twig".

@stof
Copy link
Member

stof commented Sep 3, 2019

@althaus your own case was never meant to work, as page.html.twig does not contain a foo macro that you could import.

@ericmorand
Copy link
Contributor Author

@stof, his example (reworked to add a block that he probably forgot to paste here) works with Twig 2.10:

https://twigfiddle.com/asu8dr

Not with Twig 2.11.

That it was never meant to work is debatable. page.html.twig extends from a template that imports some macros. Is there something in the doc that says that macros are not available to child templates?
I can't find anything about this in the extends documentation.

@althaus
Copy link
Contributor

althaus commented Sep 4, 2019

@ericmorand Yes, I tried to reduce our acutal code to show the problen. Thanks for creating the working fiddle!

@stof: Don't know if this was meant to work, but it worked for roughly the last 5 years.

@stof
Copy link
Member

stof commented Sep 4, 2019

@ericmorand the doc for macros are saying:

The macros can then be called at will in the current template:
https://twig.symfony.com/doc/2.x/tags/macro.html#importing-macros

And there is another note explaining that a bit later in the doc (and this note was already there at least in Twig 2.9, so it is not something added to describe the changes done in 2.11):

Importing macros using import or from is local to the current file. The imported macros are not available in included templates or child templates; you need to explicitely re-import macros in each file.
https://twig.symfony.com/doc/2.x/tags/macro.html#macros-scoping

@stof
Copy link
Member

stof commented Sep 4, 2019

I understood the issue reported by @althaus. He relied on a side-effect of the internal implementation of macros, where the m import in the parent would stay in the context in the child, and the fact that there is a macro import with the same name in the child means the child also considers m.foo as a macro call (and so compiles it to the proper code).
Note that this case is highly confusing, as any macro defined in the child (and so imported by the {% import _self as m %}) would not work (as m gets replaced entirely by the parent import). This was never considered as supported, and the only way to keep that working would be to never change the internal implementation of macros (and to never fix any of the macro issues, as the fact that _self macros would not be usable after being imported would rightfully be considered as a bug by others for instance).

@ericmorand
Copy link
Contributor Author

@stof, thanks for the clarification with the doc.

About the issue itself, it raises the same kind of debate that #3091 raised: if the implementation is faulty - and pre 2.11 macro support was faulty, shouldn't the existing project relying on this faulty implementation be considered as rightful?

This is what you wrote for #3091:

Existing projects care about the implementation, not about what the doc says or not, regarding what breaks them.

That's exactly what is happening there with @althaus - and many others - project: it cares about the implementation, not the doc. What makes this situation different from #3091 ?

@althaus
Copy link
Contributor

althaus commented Sep 5, 2019

@stof We started with Twig 1.15.0... so dunno how clear the doc has been at that time. ;-)

So I've basically have to check all our templates and verify that they are properly importing any macros from any parent on their own? So a template is a black box regarding the resolving of macros?

That sounds like fun... :(

Findus23 added a commit to matomo-org/matomo that referenced this issue Feb 16, 2020
@adampippin-cmg
Copy link

Wanted to throw another use case in here that this change breaks, since it's a little different: Apparently macros can no longer be passed as arguments to functions/extensions.

Simplified example of what I was doing previously:

$twig_environment->addFunction(\Twig\TwigFunction('my_function', function($macros) { /* do stuff with the macros */ }));
{% import ('my/macros.twig') as plugin %}
{{ my_function(plugin) }}

This now results in null being passed to the custom function. Adding needs_context and examining the context it seems like the macros are no longer present anywhere.

I understand this may or may not have not been a supported use case (though that's not really clear to me -- this is access within the same block), but it was certainly unexpected from a minor, supposedly backwards-compatible, update.

Unfortunately I can't simply roll back as others have been doing because I need to support PHP 7.4.

sgiehl added a commit to matomo-org/matomo that referenced this issue Jun 4, 2020
* proof of concept of Twig 3 upgrade

* some for if template fixes

* potentially fix RenderTokenParser

* comment out RenderTokenParser

* clearCompiledTemplates() using unlinkRecursive()

* macro imported in block and used in subblock is not valid

twigphp/Twig#3090

* more template fixes

* remove non existing clearTemplateCache()

* add missing parameter to unlinkRecursive

* Use custom MethodCallExpression to fix RenderTokenParser

* increase minimum php version to 7.2.5

* submodule update

* fix twig loop filter

* updates expected UI files

* fix twig loop filter

* fix twig loop filter

* fix neutral evolution check

* fix macro usage

* convert some conditions to filters

* fix macro include

* remove debug code as default logging is good enough

* submodule updates

Co-authored-by: sgiehl <stefan@matomo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants