raise invalid integer argument error from tablerow

[ggmichaelgo]

Problem

When a tablerow tag gets an invalid parameter for cols, limit, or offset, Liquid renders an internal error message:

{% tablerow a in (1...10) limit: true %}{{ a }}{% endtablerow %}

Liquid error: internal

This is a different behaviour than the for tag, and tablerow should have the same behaviour with the for tag:

{% for a in (1..2) limit: true %}{{ a }}{% endfor %}

Liquid error: invalid integer

[dylanahsmith]

Utils.to_integer isn't really the same conversion as to_i, so this would be changing the behaviour in a way that could break existing code. Instead of changing conversion functions, I think we just need a method that calls to_i and converts error. We don't really need this conversion function anywhere else, so that can just be defined as a private method in this class instead of adding complexity to a generic function.

    private

    def to_integer(value)
      value.to_i
    rescue NoMethodError
      raise Liquid::ArgumentError, "invalid integer"
    end

then we can just use to_integer(...) instead of Utils.to_integer(..., allow_nil: true)

Suggested change

	from = if @attributes.key?('offset')
	Utils.to_integer(context.evaluate(@attributes['offset']), allow_nil: true)
	else
	0
	end
	to = if @attributes.key?('limit')
	from + Utils.to_integer(context.evaluate(@attributes['limit']), allow_nil: true)
	end
	from = @attributes.key?('offset') ? to_integer(context.evaluate(@attributes['offset'])) : 0
	to = @attributes.key?('limit') ? from + to_integer(context.evaluate(@attributes['limit'])) : nil

[ggmichaelgo]

for tag is already using the Utils#to_integer helper function to parse the parameters, and I believe we should do the same for the tablerow tag:
https://github.com/Shopify/liquid/blob/master/lib/liquid/tags/for.rb#L117-L126 (I should have also refactored the for tag to use the new helper)

I think it is important for our tags to have consistent parameter parsing behaviour, but I do agree that using Utils#to_integer might break some themes...
With to_i, themes were able to use String float such as "100.25" for parameters, but with Utils#to_integer would render an error...

I am a bit conflicted on this now... @dylanahsmith what do you think?

[dylanahsmith]

My suggestion would fix the reported problem, which was the internal error.

Fixing inconsistencies is much harder to do safely in liquid, where it can be hard to know the impact upfront with instrumenting it. We have Liquid::Usage.instrument which can be used to determine if something is being used, which I used recently in #1667 and found out it is being used. We could similarly add instrumentation to see if the change to make it consistent would be cause a breaking change before actually making the change, but you might find the same unsatisfying answer.