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

Fix bug in tokenizer with nil source value #1873

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Fix bug in tokenizer with nil source value #1873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/liquid/tokenizer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class Tokenizer
attr_reader :line_number, :for_liquid_tag

def initialize(source, line_numbers = false, line_number: nil, for_liquid_tag: false)
@source = source
@source = source.to_s
Copy link
Member Author

@bahar-p bahar-p Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not entirely sure why liquid_c does .to_s.to_str. 🤷🏼‍♀️

to_s should be sufficient, but let me know if I'm missing anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there was a legit reason originally to use both to_s.to_str, see #1421 (comment)

Looks like @ggmichaelgo then removed the logic here #1775

Copy link
Member Author

@bahar-p bahar-p Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah the first PR didn't introduce a bug though. In the second PR @ggmichaelgo is addressing a different bug, where then removed it from the Tokenizer and doing the conversion in Template class. But that's not always the entry point to the Tokenizer. Note that in Core the templates are subclasses of Liquid::Document not Liquid::Template.

My question is more around doing double conversion to_s.to_str which I'm not sure I understand yet. I've double checked the documents and they're essentially alias of each other

From the docs

to_s → str
Returns self.

If called on a subclass of String, converts the receiver to a [String](https://devdocs.io/ruby~3/string) object.

Also aliased as: to_str
Returns self.

If called on a subclass of [String](https://devdocs.io/ruby~3/string), converts the receiver to a String object.

Alias for: to_s

I'm asking Ruby folks to see what I may be missing. Will address in follow up as soon as I hear back and learn that we legitimately need the double conversion.

@line_number = line_number || (line_numbers ? 1 : nil)
@for_liquid_tag = for_liquid_tag
@offset = 0
Expand Down
4 changes: 4 additions & 0 deletions test/unit/tokenizer_unit_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ def test_calculate_line_numbers_per_token_with_profiling
assert_equal([1, 1, 3], tokenize_line_numbers(" {{\n funk \n}} "))
end

def test_tokenize_with_nil_source_returns_empty_array
assert_equal([], tokenize(nil))
end

private

def new_tokenizer(source, parse_context: Liquid::ParseContext.new, start_line_number: nil)
Expand Down
Loading