GetAttrCompiler produces redundant macros arguments during macros call compilation

[fduch]

Hello @schmittjoh!

Probably there is an issue with compiling twig macros calls with parameters.

That's my case below.
I am using two twig templates in symfony 2 application.
The first one contains show macro definition and looks like this:

{% twig_js name="twig.compiled.flags" %}
{% macro show(country) %}
    <div class="flag_{{ country }}"></div>
{% endmacro %}

The second one includes show macro and invokes it with 2 parameters:

{% twig_js name="twig.compiled.flags_temp" %}
{% import "::blocks/flags/flags.html.twig" as flags %}

{{ flags.show(country) }}

After compilation i am trying to call the second template from javascript like this:

Twig.render(twig.compiled.flags_temp, {country: "fr"})

The expected result is:

<div class="flag flag_fr"></div>

But actual result looks like this:

<div class="flag flag_0"></div>

Attempting to understand the problem i've analyzed compiled representation of my both original twig templates. The second one has redundant argument 0 in show parameter list, therefore original macro did not work as expected:

/**
 * @inheritDoc
 */
twig.compiled.flags_temp.prototype.render_ = function(sb, context, blocks) {
    // line 2
    context["flags"] = this.env_.createTemplate(twig.compiled.flags);
    // line 3
    sb.append("\n");
    // line 4
    sb.append(twig.attr("flags" in context ? context["flags"] : null, "show", [0, "country" in context ? context["country"] : null], "method"));
};

The cause of this is clear to me: twig parser fills macro parameters into Twig_Node_Expression_Array object using its addElement method that prepends each element with new constant node that contains index of element (excluding associative arrays case with it predefined keys).
And then twig-js's GetAttrCompiler compiles all the parameters (including generated index-elements) provided by parser into js function arguments here: https://github.com/schmittjoh/twig.js/blob/master/src/TwigJs/Compiler/Expression/GetAttrCompiler.php#L60-L74 and it causes extra parameters appearance.

What if we'll use ArrayCompiler to compile getattr arguments? In this case we would manage flexibly argument sets as key-values and get only needed values in getattr arguments lists.

[jean-pasqualini]

Thank you to solve this problem because I also

[phoenix741]

Hi,

I have the same problem. I have a macro that call another macro, but it doesn't work because the generate file contains wrong array.
How can i resolve the problem ?

Thanks

{% macro form_widget(type, label, path, attr) %}
    {{ type }}
    {% if type == "textarea" %}
        {{ _self.textarea_widget(type, path, attr) }}
    {% elseif type == "choice" %}
        {{ _self.choice_widget_collapsed(type, path, attr) }}
    {% elseif type == "money" %}
        {{ _self.money_widget(type, path, attr) }}
    {% elseif type == "integer" %}
        {{ _self.integer_widget(type, path, attr) }}
    {% elseif type == "number" %}
        {{ _self.number_widget(type, path, attr) }}
    {% elseif type == "button" or type == "submit" %}
        {{ _self.button_widget(type, label, path, attr) }}
    {% else %} 
        {{ _self.form_widget_simple(type, path, attr) }}
    {% endif %}
{% endmacro %}

{% macro form_row(type, label, path, label_attr, attr) %}
    {{ type }}
    {% if type != "button" and type != "submit" %}
    <div class="form-group">
        {{ _self.form_label(label, path, label_attr, attr) }}
        {{ _self.form_widget(type, label, path, attr) }}
        {% if attr.help is defined %}
        <p class="help-block">{{ attr.help }}</p>
        {% endif %}
    </div>
    {% else %}
        {{ _self.form_widget(type, label, path, attr) }}
    {% endif %}
{% endmacro %}

give the output result :

macro_theme.html.prototype.getform_widget = function(opt_type, opt_label, opt_path, opt_attr) {
    var context = twig.extend({}, this.env_.getGlobals());

    var sb = new twig.StringBuffer;
    // line 108
    sb.append("\t");
    sb.append(twig.filter.escape(this.env_, opt_type, "html", null, true));
    sb.append("\n\t");
    // line 109
    if (((opt_type) == ("textarea"))) {
        // line 110
        sb.append("\t\t");
        sb.append(twig.attr(this, "textarea_widget", [0, opt_type, 1, opt_path, 2, opt_attr], "method"));
        sb.append("\n\t");
    } else if (((opt_type) == ("choice"))) {
...
        // line 121
        sb.append(" \n\t\t");
        // line 122
        sb.append(twig.attr(this, "form_widget_simple", [0, opt_type, 1, opt_path, 2, opt_attr], "method"));
        sb.append("\n\t");
    }

    return new twig.Markup(sb.toString());
};

// line 126
/**
 * Macro "form_row"
 *
 * @param {*} opt_type
 * @param {*} opt_label
 * @param {*} opt_path
 * @param {*} opt_label_attr
 * @param {*} opt_attr
 * @return {string}
 */
macro_theme.html.prototype.getform_row = function(opt_type, opt_label, opt_path, opt_label_attr, opt_attr) {
    var context = twig.extend({}, this.env_.getGlobals());

    var sb = new twig.StringBuffer;
    // line 127
    sb.append("\t");
    sb.append(twig.filter.escape(this.env_, opt_type, "html", null, true));
    sb.append("\n\t");
    // line 128
    if (((((opt_type) != ("button"))) && (((opt_type) != ("submit"))))) {
        // line 129
        sb.append("\t<div class=\"form-group\">\n\t\t");
        // line 130
       sb.append(twig.attr(this, "form_label", [0, opt_label, 1, opt_path, 2, opt_label_attr, 3, opt_attr], "method"));
        sb.append("\n\t\t");
        // line 131
        sb.append(twig.attr(this, "form_widget", [0, opt_type, 1, opt_label, 2, opt_path, 3, opt_attr], "method"));
        sb.append("\n\t\t");
        // line 132
        if (twig.attr(twig.filter.def(opt_attr), "help", undefined, undefined, true)) {
            // line 133
            sb.append("\t\t<p class=\"help-block\">");
            sb.append(twig.filter.escape(this.env_, twig.attr(opt_attr, "help"), "html", null, true));
            sb.append("<\/p>\n\t\t");
        }
        // line 135
        sb.append("\t<\/div>\n\t");
    } else {
        // line 137
        sb.append("\t\t");
        sb.append(twig.attr(this, "form_widget", [0, opt_type, 1, opt_label, 2, opt_path, 3, opt_attr], "method"));
        sb.append("\n\t");
    }

    return new twig.Markup(sb.toString());
};

[fduch]

@phoenix741 Does your macros work with version of twig.js containing my changes proposed to fix this issue?

[henrycatalinismith]

I'm finding the {% import %} tags and real-world HTML code distracting. Here's an example that isolates the issue without any of that stuff.

Template

{% macro show(value) %}
The value is {{ value }}
{% endmacro %}
{{ _self.show("Hello World!") }}

Expected Output

The value is Hello World!

Actual Output

The value is 0

[henrycatalinismith]

Rerunning the test case with the code change from the proposed fix, it works as expected.

The value is Hello World!

I'll make some time to look at this in more depth and do some regression testing. It'd be nice if we could fit this into the automated test suite somehow so that it can never break again without us noticing as well.

Thanks for the patch btw :)

[fduch]

@h2s, yeah, your are right, some regression testing is very appreciated there. Unfortunately we have not enough test fixtures to cover the most cases.. Anyway we can add your example as a fixture to the current pull-request.

[henrycatalinismith]

I've ported Twig's integration testing system to Twig.js to help with this issue. It's awesome, now we can write full integration test cases to reproduce bugs like these in a way that we can incorporate into the test suite. Here's the test case for this one.

--TEST--
Macro bug from schmittjoh/twig.js#67
--TEMPLATE--
{% macro show(value) %}
The value is {{ value }}
{% endmacro %}
{{ _self.show("Hello World!") }}
--DATA--
return array()
--EXPECT--
The value is Hello World!

I'm confident enough now to merge your fix too. Thanks for being so patient with this issue. It took me a while to set aside some time to work on Twig.js again :)

[henrycatalinismith]

Hmmm, GitHub isn't clever enough to detect that I've rebased your pull request before merging it. Rest assured, your fix is now merged into schmittjoh/twig.js as commit 6775ffa. Looks like I'll have to close the PR manually though.

Thanks again!

[fduch]

Ok, thank you)