Skip to content
This repository has been archived by the owner on Jul 27, 2024. It is now read-only.

Fix gnarly bug in Node and SpaceInsideBraces #423

Merged
merged 5 commits into from
Sep 9, 2021

Conversation

charlespwd
Copy link
Contributor

@charlespwd charlespwd commented Sep 2, 2021

Fixes #420

This fixes a LOT of bugs actually! This bug was like finding a little crack in a wall and then finding you have termites.

  1. This is Liquid's raw output for the following tag:

    {%
      render
      'file'
    %}
    # expected
    => "render\n  'file'\n"
    # actual
    => "render 'file'\n"
    

    As you can see, not only do we not have the whitespace all the way till "render", we also don't have the whitespace between render and 'file'. This breaks all Position calculations since the needle does not exist in the source.

  2. The implementation of whitespace_trimmed? didn't account for tags starting in the middle of a line. Or for tags starting on a different line

    <ul class="{%- if true -%}oh no{% endif %}">
    => not whitespace trimmed!
    {%-
      render 'oh no'
    -%}
    => not whitespace trimmed!
    
  3. The implementation of whitespace_trimmed? assumed that it was trimmed in pairs (start and end) when it's possible to do it independently.

    {% assign true -%}
    => not whitespace trimmed!
    
  4. Similarly a lot of our logic for space before/after tokens was wrong inside SpaceInsideBraces

  5. Our implementation of inside_liquid_tag? was flipped. The tests were wrong and the implementation was too!

Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

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

Wow amazing! Thanks for putting in the work to make this part solid 🙏

Just a couple questions and suggestions.

Comment on lines +168 to +176
output += "{{" if variable?
output += "{%" if tag?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't be a variable & tag at the same time:

Suggested change
output += "{{" if variable?
output += "{%" if tag?
if variable?
output += "{{"
elsif tag?
output += "{%"
end

Same bellow.

Comment on lines +191 to +202
# Here we're hacking around a glorious bug in Liquid that makes it so the
# line_number and markup of a tag is wrong if there's whitespace
# between the tag_name and the markup of the tag.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a sense if this is fixable in Liquid, or is just the way it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be fixable but the way tags are created you don't really know. We'd need to somehow pass it in the parse_context since we lose the information on tag creation (the tag_name and markup come in as two separate variables)

https://github.com/Shopify/liquid/blob/eab13a07d9861a38d993d2749ae25f06ff76426b/lib/liquid/tag.rb#L25-L37

@macournoyer
Copy link
Contributor

Whoops! Actually found a tiny bug, checking Dawn:

▶ dev check ../dawn
Checking ../dawn ...
sections/image-banner.liquid:13: style: SpaceInsideBraces: Space missing before '%}'.
	<div id="Banner-{{ section.id }}" class="banner{% if section.settings.stack_images_on_mobile and section.settings.ima...
	                                                                          ^```

The `^` is not at the right place.

@charlespwd
Copy link
Contributor Author

So the location is wrong, but the error is correct!

{% if section.settings.stack_images_on_mobile and section.settings.image != blank and section.settings.image_2 != blank%}

There's a missing space here!

@charlespwd charlespwd force-pushed the fix/420-space-inside-brace-bug branch 4 times, most recently from b94e0a9 to 02d2633 Compare September 7, 2021 19:45
contents.lines is a very expensive operation if you do it 100,000 times!

This reimplements the function without being so careless about memory.
@charlespwd charlespwd force-pushed the fix/420-space-inside-brace-bug branch from 02d2633 to c5475c5 Compare September 7, 2021 19:46
@charlespwd charlespwd merged commit 228b463 into main Sep 9, 2021
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems September 9, 2021 17:52 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception when checking for spaces inside braces
2 participants