Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

raise invalid integer argument error from tablerow #1676

Merged
merged 2 commits into from
Jan 18, 2023
Merged

raise invalid integer argument error from tablerow #1676

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f11c97
raise invalid integer argument error from tablerow
ggmichaelgo Jan 16, 2023
e889a9d
use to_i to parse parameters of tablerow tag
ggmichaelgo Jan 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 14 additions & 3 deletions lib/liquid/tags/table_row.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,24 @@ def initialize(tag_name, markup, options)
def render_to_output_buffer(context, output)
(collection = context.evaluate(@collection_name)) || (return '')

from = @attributes.key?('offset') ? context.evaluate(@attributes['offset']).to_i : 0
to = @attributes.key?('limit') ? from + context.evaluate(@attributes['limit']).to_i : nil
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


collection = Utils.slice_collection(collection, from, to)
length = collection.length

cols = @attributes.key?('cols') ? context.evaluate(@attributes['cols']).to_i : length
cols = if @attributes.key?('cols')
Utils.to_integer(context.evaluate(@attributes['cols']), allow_nil: true)
else
length
end

output << "<tr class=\"row1\">\n"
context.stack do
Expand Down
5 changes: 4 additions & 1 deletion lib/liquid/utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ def self.slice_collection_using_each(collection, from, to)
segments
end

def self.to_integer(num)
def self.to_integer(num, allow_nil: false)
return num if num.is_a?(Integer)
# with allow_nil param, return 0 which is equal to nil.to_i
return 0 if num.nil? && allow_nil

num = num.to_s
begin
Integer(num)
Expand Down
46 changes: 46 additions & 0 deletions test/integration/tags/table_row_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,32 @@ def test_cols_nil_constant_same_as_evaluated_nil_expression
{ "var" => nil })
end

def test_nil_limit_is_treated_as_zero
expect = "<tr class=\"row1\">\n" \
"</tr>\n"

assert_template_result(expect,
"{% tablerow i in (1..2) limit:nil %}{{ i }}{% endtablerow %}")

assert_template_result(expect,
"{% tablerow i in (1..2) limit:var %}{{ i }}{% endtablerow %}",
{ "var" => nil })
end

def test_nil_offset_is_treated_as_zero
expect = "<tr class=\"row1\">\n" \
"<td class=\"col1\">1:false</td>" \
"<td class=\"col2\">2:true</td>" \
"</tr>\n"

assert_template_result(expect,
"{% tablerow i in (1..2) offset:nil %}{{ i }}:{{ tablerowloop.col_last }}{% endtablerow %}")

assert_template_result(expect,
"{% tablerow i in (1..2) offset:var %}{{ i }}:{{ tablerowloop.col_last }}{% endtablerow %}",
{ "var" => nil })
end

def test_tablerow_loop_drop_attributes
template = <<~LIQUID.chomp
{% tablerow i in (1...2) %}
Expand Down Expand Up @@ -131,4 +157,24 @@ def test_tablerow_loop_drop_attributes

assert_template_result(expected_output, template)
end

def test_table_row_renders_correct_error_message_for_invalid_parameters
assert_template_result(
"Liquid error (line 1): invalid integer",
'{% tablerow n in (1...10) limit:true %} {{n}} {% endtablerow %}',
render_errors: true,
)

assert_template_result(
"Liquid error (line 1): invalid integer",
'{% tablerow n in (1...10) offset:true %} {{n}} {% endtablerow %}',
render_errors: true,
)

assert_template_result(
"Liquid error (line 1): invalid integer",
'{% tablerow n in (1...10) cols:true %} {{n}} {% endtablerow %}',
render_errors: true,
)
end
end