Pass the tag markup and tokenizer to Document#unknown_tag#1290
Conversation
dylanahsmith
force-pushed
the
refactor-for-c-block-body
branch
from
e146906 to
dfbbf87
Compare
dylanahsmith
force-pushed
the
document-unknown-tag-refactor
branch
from
ea97ca3 to
a372baa
Compare
| @body.parse(tokens, parse_context) do |end_tag_name, _end_tag_params| | ||
| unknown_tag(end_tag_name, parse_context) if end_tag_name | ||
| def parse(tokenizer, parse_context) | ||
| while parse_body(tokenizer) |
There was a problem hiding this comment.
Because we don't want the handled unknown tag (e.g. section tags) to end the parsing of the document, we still need to parse the rest of the document.
There was a problem hiding this comment.
Right, but why wasn't there a loop in the previous implementation? Sorry if I'm missing something obvious 😅 I reviewed expecting a refactor without change in behavior.
There was a problem hiding this comment.
Previously unknown_tag was expected to always raise when called on a Liquid::Document, unlike Liquid::Block which has its unknown_tag method overridden by child classes which don't always raise. This won't change any existing code, since Liquid::Document#unknown_tag itself still always raises. It just means that now we can override unknown_tag in a child class of Liquid::Document and not raise to have it continue parsing.
There was a problem hiding this comment.
It just means that now we can override
unknown_tagin a child class ofLiquid::Documentand not raise to have it continue parsing.
Ahhh that explains it. 👍
| @body.parse(tokens, parse_context) do |end_tag_name, _end_tag_params| | ||
| unknown_tag(end_tag_name, parse_context) if end_tag_name | ||
| def parse(tokenizer, parse_context) | ||
| while parse_body(tokenizer) |
There was a problem hiding this comment.
It just means that now we can override
unknown_tagin a child class ofLiquid::Documentand not raise to have it continue parsing.
Ahhh that explains it. 👍
The parse_context no longer needs to be passed in because it is available through through an attr_reader on the instance. However, the markup and tokenizer weren't made available. This refactor also makes the parameters given to Document#unknown_tag consistent with Block#unknown_tag.
dylanahsmith
force-pushed
the
document-unknown-tag-refactor
branch
from
a372baa to
0d02dea
Compare
3 participants
Depends on #1289
Problem
Shopify is currently overriding Liquid::Document#registered_tags to implement theme section tags, since they are only supposed to be available at the top-level of the section document. However,
registered_tagsis actually defined in Liquid::BlockBody and #1289 will make Liquid::Document no longer inherit from Liquid::BlockBody, so we need another way to enforce this constraint.Solution
Liquid already has builtin context sensitive tags, such as
elsifandelsewithin theiftag. These are implemented usingLiquid::Block#unknown_tag. Liquid::Document also has anunknown_tagmethod, but it isn't provided the markup or tokenizer needed to the information we need, so the first commit of this pull request changesLiquid::Document#unknown_tagto take the same parameters asLiquid::Block#unknown_tag.Liquid::Document#parsewas also changed to be more likeLiquid::Block#parseby continuing to parse ifunknown_taghandles the tag without raising.The second commit merely changes the
Liquid::Block#unknown_tagparameter names for clarify since we usemarkupelsewhere to refer to the text within the tag after the tag name and the third parameter is actually a tokenizer.