@diosmosis

As you might know the reason for RenderTokenParser better: It seems like all classes still exist and accept the same variables. But when compiling I get the following error:

Argument 1 passed to twig_call_macro() must be an instance of Twig\Template, null given, called in /home/lukas/public_html/matomo/tmp/templates_c/d5/d5170b71a6f342f1d816a92543eb41445131dba1eeb3f5ae17688ebe6efe479c.php on line 65
in /home/lukas/public_html/matomo/vendor/twig/twig/src/Extension/CoreExtension.php line 1128            

with the compiled template being:

    // line 4
    public function block_topcontrols($context, array $blocks = [])
    {
        $macros = $this->macros;
        // line 5
        echo "    ";
        $this->loadTemplate("@CoreHome/_siteSelectHeader.twig", "@CoreHome/getDefaultIndexView.twig", 5)->display($context);
        // line 6
        echo "    ";
        $this->loadTemplate("@CoreHome/_periodSelect.twig", "@CoreHome/getDefaultIndexView.twig", 6)->display($context);
        // line 7
        echo "    ";
        echo call_user_func_array($this->env->getFunction('postEvent')->getCallable(), ["Template.nextToCalendar"]);
        echo "
    ";
        // line 8
        $this->loadTemplate(twig_call_macro($macros["dashboardSettingsControl"], "getTemplateFile", [], 8, $context, $this->getSourceContext()), "@CoreHome/getDefaultIndexView.twig", 8)->display(twig_array_merge($context,                                twig_call_macro($macros["dashboardSettingsControl"], "getTemplateVars", [], 8, $context, $this->getSourceContext())));
        // line 9
        echo "    ";
        $this->loadTemplate("@CoreHome/_headerMessage.twig", "@CoreHome/getDefaultIndexView.twig", 9)->display($context);
    }

and $macro being empty.

To fix the remaining tests, this patch is needed for CustomAlerts, but it is not compatible with Twig <2.9 so it would break before this is merged:

diff --git a/templates/macros.twig b/templates/macros.twig
index 48f88fe..7b6adcd 100644
--- a/templates/macros.twig
+++ b/templates/macros.twig
@@ -1,5 +1,5 @@
 {% macro metricChangeDescription(alert) %}
-{% spaceless %}
+{% apply spaceless %}
     {%- if alert.metric_condition == 'less_than' %}
         {{ 'CustomAlerts_ValueIsLessThan'|translate(alert.metric_matched, alert.value_new) }}
     {% elseif alert.metric_condition == 'greater_than' %}
@@ -13,5 +13,5 @@
     {% elseif alert.metric_condition == 'percentage_increase_more_than' %}
         {{ 'CustomAlerts_ValuePercentageIncreasedMoreThan'|translate(alert.metric_matched, alert.value_old|default('-'), alert.value_new) }}
     {% endif -%}
-{% endspaceless %}
+{% endapply %}
 {% endmacro %}
\ No newline at end of file

@Findus23 I compared your template w/ the template on twig 2, seems instead of twig_call_macro($macros['dashboardSettingsControl'], getTemplateFile()) it was set to $context['dashboardSettingsControl']->getTemplateFile(). Looks like there was some sort of change to MethodCallExpression. Unfortunately I don't know if it can be fixed, looks like something fundamental changed.

RenderTokenParser was written before adopting angularjs, it was trying to solve the same problem of grouping rendering & behavioral logic together in units (like components/directives). It's not needed and can be removed.

Does this help?

@Findus23 are you still working on that one, or should someone take over?

@sgiehl I don't think I am able to rewrite or remove the RenderTokenParser and the rest should be done (apart from the things mentioned in the first comment) so someone can take over.

Tried to dig a bit deeper into the RenderTokenParser. The implementation of the used MethodCallExpression has changed in a way that it doesn't work for our usage anymore. I've now simply copied the old MethodCallExpression from twig 1.x and used that instead. That seems to fix the problem. Not sure if there would be a smarter solution, but don't want to waste more time digging into Twig...

@diosmosis If you think it's useful to completely remove the RenderTokenParser and the stuff around it, feel free to create a follow-up issue. Guess it makes sense to try to merge that as soon as possible so we maybe find remaining problems if there are any not covered by ui tests.

Will go though the failing UI tests now and fix them...

@diosmosis If you think it's useful to completely remove the RenderTokenParser and the stuff around it, feel free to create a follow-up issue.

I can take a look, but I'm not sure exactly when.

BTW @Findus23 @sgiehl is there a big benefit from Twig 3 or any particular issue it solves? If it takes any more time we might otherwise rather do this in Matomo 5 or so considering it wasn't in the original Matomo 4 milestone.

If we merge this, we'd likely also need to implement matomo-org/matomo-for-wordpress#233 potentially as I'd expect most WordPress plugins that use twig still use an older version and to avoid some compatibility issues. Also we'd need to see re the PHP 7.2.5 requirement increase as in WP we defined 7.2 but that might be fine.

Be curious to learn what benefit Twig 3 brings and be great to know how long Twig 2 receives updates.

At the moment we are not using Twig 2 (otherwise the update would be far simpler), but Twig 1.

Benefits I can think of at the moments:

• better performance
• less weird edge-cases (there were a few unexpected behaviours that were deprecated in the major releases)
• people don't need to learn how the outdated twig version differers if they want to write a Matomo

The changes are not that huge and I think with the changes by @sgiehl it is 95% finished.
Also most people will be using Matomo 4.x using PHP 8 which will be a huge release with many breaking changes so I wouldn't be surprised if the older twig versions will not support it. And that would bring us into the impossible situation of either not supporting the main PHP version for years, backporting and maintaining Twig ourselves or having a release that breaks all plugins.
Unfortunatly there are no official EOL dates for Twig 1 and 2 so this is just a guess by me.

But now with Matomo 4 it would be a good chance to easily make a breaking chance when the next one will only come in 3 or 4 years.

Sounds good. Will need to see how this impacts WordPress. Quite annoying re the PHP 7.2.5. Also bit scared of these global things where we need to adjust the twig files for many plugins as it costs us a lot of time and the same for developers should they need to migrate twig files (although not many developers use it to a big extend anyway it be mostly us).

Eg adjusting all our plugins and especially premium features and cloud and account management code might take quite a long time as there will be a lot of testing needing to be done etc. In Premium Features a lot of the code should be in angular though.

@sgiehl can you estimate how much work is left to be done here for core plus our open source plugins?

@tsteur core and submodule plugins should work already. There shouldn't be much work to do for the other plugins I think. It's mostly only changing if conditions in for loops into filters, and converting the spaceless tag if it was used. And some imports might need to be moved as the context seems to have changed in some cases.

Imho someone else should do a review now. Afterwards we can merge and should then see in the UI tests of our other plugins if something need to be changed. But will also do a search through the other plugins once this is merged and update them if needed...

Will do a review now.

We can look into the premium features and cloud later. I can do the migration for most plugins. @sgiehl be good to summarise in a new section in https://github.com/matomo-org/matomo/wiki/Matomo-4.0.0-release-notes what changes in Twig 3 and how to change it. I suppose there are a few common things. This will make it easier for us and for developers. Like some examples like the ones below which we can then put into a migration guide.

{spaceless} -> {array spaceless}
plugins if plugin.uninstallable and plugin.activated -> plugins|filter(plugin => plugin.uninstallable and plugin.activated)
divisibleby(2) -> divisible by(2)

Be good to not migrate the other plugins for now but focus on Matomo 4 issues.

BTW: Adjusting the templates shouldn't be complex in most cases, so once we are in the first beta releases I could create PRs for all non-premium plugins and some major third-party plugins.

@Findus23 it's mostly just the testing for our many non-core plugins as not everything is covered by UI tests etc. If we give 3rd party developers some examples on how to migrate it like we did in https://developer.matomo.org/guides/migrate-piwik-2-to-3 that will be great so it's easy for them to do.

@tsteur added some migration notes here: https://github.com/matomo-org/matomo/wiki/Matomo-4.0.0-release-notes#migrating-templates-to-twig-3

updated the branch and it imho should be good to merge now.
@tsteur @diosmosis @Findus23 any concerns?

Looks good to me 👍

Concerns re the required PHP version etc but all good to merge.

@tsteur the Twig documentation still clearly says it requires PHP 7.2.0 (see https://twig.symfony.com/doc/3.x/intro.html), not sure why they increased the requirement in composer.json to PHP 7.2.5 🤷 So maybe it would even run with PHP 7.2.0

It seems like it was set in https://github.com/twigphp/Twig/pull/3222 to match symphony requirements. So there might be a chance that it works anyway.

But I doubt that matters as I hope no one uses PHP >=7.2.0,<7.2.5.