@Findus23 opened this Pull Request on February 14th 2020 Member

This is just a quick test to see how much work upgrading to Twig 3 would be.

It turns out that apart from replacing tons of classnames, there is little else to change. On a first look all Matomo seemed to work with just two minor template changes.

Things left to do:

  • I had to raise the minimum PHP version from 7.2 to 7.2.5 as this is what twig requires.
  • I just wrote a quick ugly hack in core/View.php to see the full template path to twig errors
  • I had to upgrade matomo/referrer-spam-blacklist as composer refused to do anything before (maybe the composer.lock isn't up to date)
  • I couldn't find the correct return Object for the RenderTokenParser See below
  • tons of testing as this could subtly break things, so doing this early in the Matomo 4 development would allow to find all of them
@Findus23 commented on February 16th 2020 Member

@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("<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/_siteSelectHeader.twig", "<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/getDefaultIndexView.twig", 5)->display($context);
        // line 6
        echo "    ";
        $this->loadTemplate("<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/_periodSelect.twig", "<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/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()), "<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/getDefaultIndexView.twig", 8)->display(twig_array_merge($context,                                twig_call_macro($macros["dashboardSettingsControl"], "getTemplateVars", [], 8, $context, $this->getSourceContext())));
        // line 9
        echo "    ";
        $this->loadTemplate("<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/_headerMessage.twig", "<a class='mention' href='https://github.com/CoreHome'>@CoreHome</a>/getDefaultIndexView.twig", 9)->display($context);
    }

and $macro being empty.

@Findus23 commented on February 17th 2020 Member

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
@diosmosis commented on February 17th 2020 Member

@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?

Powered by GitHub Issue Mirror