Integer and Float Comparisons After Using Times Filter

[shalaniw]

I'm observing some odd behavior when using the times filter with a float value. In both of the cases below, the comparingValue variable should be 98.0. In both tests I check if 99 is greater than that value. For some reason, the case with the times filter fails with false and the case with the divide_by filter passes with true.

    // Test Fails
    @Test
    public void testUseVariableAndTimesFilter() throws RecognitionException {
        assertPatternResultEquals(TemplateParser.DEFAULT, "true", 
            "{% assign comparingValue = 98 | times: 1.0 %}{{ 99 > comparingValue }}");
    }

    // Test Passes   
    @Test
    public void testUseVariableAndDividedByFilter() throws RecognitionException {
        assertPatternResultEquals(TemplateParser.DEFAULT, "true",
            "{% assign comparingValue = 98 | divided_by: 1.0 %}{{ 99 > comparingValue }}");
    }

I believe this is a bug and that both cases should pass with true.

[msangel]

Hello! Thanks for the report. Bug confirmed. I already have a fix for this, but the problem seems to be wider, so more investigation needed.

tech details: problem was in this part of code:

        return (a instanceof Number) && (b instanceof Number) &&
                super.asNumber(a).doubleValue() > super.asNumber(b).doubleValue();

in a case when variables are string (and in this case they are since variable evaluation perform to string)

[msangel]

Another interesting question is about compatibility of this samples with original liquid. In Jekyll I got "98" and in Liquid I got

/usr/lib/ruby/gems/3.1.0/gems/liquid-5.4.0/lib/liquid/parser.rb:18:in `consume': Liquid syntax error: Expected end_of_string but found comparison in "{{ '98' > comparingValue }}" (Liquid::SyntaxError)

So in solving this issue the preference for ruby's implementation will be given.

[kohlschuetter]

@msangel Shouldn't we follow Jekyll's implementation here? We might control this using Flavor.

[msangel]

Yep, we should.

[msangel]

Have to debug ruby.... Looks like Liquid version of variable is not designed for printing anything but atom. it consume the atom and expects the end, even if there more tokens (that's where the error came from)

test code in shopify/liquid/test/integration/expression_test.rb file:

  def test_comparing_expression
    assert_template_result("true", "{% assign comparingValue = 98.0 %}{{ '98' > comparingValue }}")
  end

[msangel]

Will debug Jekyll ones later....

[msangel]

More of Liquid (just for self-documenting first) This is not something developers will expect from "evaluate expression" functionality so I am pretty sure this behavior for Liquid is simply broken BUT we must follow it

[msangel]

Jekyll have similar behavior but it simply return first atom(without any further expectations)

[msangel]

In the end they are the same, Jekyll uses :warn mode strictness, while liquid :strict Both are configurable in both places.

[msangel]

Jekyll default configuration is :warn: Liquid default is lax:

Traces are: lax name eval: https://github.com/Shopify/liquid/blob/master/lib/liquid/variable.rb#L50 strict name eval: https://github.com/Shopify/liquid/blob/master/lib/liquid/variable.rb#L68 ---> where only first token is taken (in our case number): https://github.com/Shopify/liquid/blob/master/lib/liquid/parser.rb#L51-L61

[msangel]

Currently, this library has only strict and non-strict modes. And this strictness is applied only to context behavior, as partial parsing is not supported. Template source is tokenized no matter what or general fail.

[msangel]

So we have two options:

• Have ruby compatibility with their weirdness - not execute expressions in out tag but simply print the first atom
• Have proper and predictable behavior that meets any expectations but goes against the original templator - execute expressions in out tag and print evaluated result

What would you pick? @bkiers

[bkiers]

During my active development, I've always thrived for compatibility with Ruby's Liquid library. But given this is rather odd, I'd go for the predictable route. But given I am not actively working on this, if you'd rather be compatible with Ruby, I'd support that too @msangel.

[msangel]

Ok. My decision is: by default this will behave as expected(as of now). BUT there will be a flag that will turn on either the first token either error on something more than one token. If someone will need full compatibility, the one will get it. Technical notes: here on parser level, lexer is not affected:

output
 : outStart expr filter* OutEnd
 ;

[msangel]

Closed fully in https://github.com/bkiers/Liqp/commit/af65618808cf686770ec2ddd7bfbed55dabf537e accoarding to exact ruby behavior