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

Fix handling of unknown tags in comments #256

Merged
merged 1 commit into from Sep 11, 2013
Merged

Fix handling of unknown tags in comments #256

merged 1 commit into from Sep 11, 2013

Conversation

fw42
Copy link
Contributor

@fw42 fw42 commented Sep 11, 2013

Fixes this:

>> Liquid::Template.parse("{% comment %} {% whatever %} {% endcomment %}")
Liquid::SyntaxError: Unknown tag 'whatever'

Please review @sirupsen @dylanahsmith

sirupsen
sirupsen reviewed Sep 11, 2013
View reviewed changes
lib/liquid/tags/comment.rb
@@ -4,6 +4,9 @@ def render(context)
''
end

def unknown_tag(tag, markup, tokens)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this 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.

Because Comment is a subclass of Block. This overwrites Block's unknown_tag method (which raises).

Copy link
Contributor

Choose a reason for hiding this comment

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

aah, I see! :shipit:

@fw42
Copy link
Contributor Author

fw42 commented Sep 11, 2013

Came up with a different solution: Make Comment a subclass of Raw (so it inherits it's parse method). This will also allow syntax errors inside the comment, i.e., fix this:

>> Liquid::Template.parse("{% comment %} {{ {% endcomment %}")
Liquid::SyntaxError: Variable '{{ {% endcomment %}' was not properly terminated with regexp: /\}\}/

Please take another look.

@sirupsen
Copy link
Contributor

Nice! 👍

@dylanahsmith
Copy link
Contributor

Nice clean solution 👍

fw42 added a commit that referenced this pull request Sep 11, 2013
@fw42
Merge pull request #256 from Shopify/unknown_tags_in_comments
1ae8c0e 
Fix handling of unknown tags in comments
@fw42 fw42 merged commit 1ae8c0e into master Sep 11, 2013
@fw42 fw42 deleted the unknown_tags_in_comments branch September 11, 2013 14:59
fw42 pushed a commit that referenced this pull request Sep 11, 2013
Revert "Merge pull request #256 from Shopify/unknown_tags_in_comments"
f938756 
This reverts commit 1ae8c0e, reversing
changes made to 01d352b.
@fw42 fw42 mentioned this pull request Sep 11, 2013
Merged
@parkr parkr mentioned this pull request Mar 10, 2014
Closed
@dylanahsmith dylanahsmith mentioned this pull request Nov 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fw42 @sirupsen @dylanahsmith