Parse section as a standalone tag - #1309
stephanie-shopify wants to merge 4 commits into
Conversation
| * +section+ is a standalone tag, so +{% endsection %}+ parses as an unknown | ||
| * tag. Ruby Liquid reports it the same way. | ||
| */ | ||
| export function checkEndsectionTag(node: LiquidTag, context: Context): void { |
There was a problem hiding this comment.
not sure if this is correct behavior to add
There was a problem hiding this comment.
This moves existing behavior rather than adding new behavior. On main, LiquidSyntaxError already reports Unknown tag 'endsection', from inside checkSectionTag via the section node's blockEndPosition. That only exists because of the hybrid block form. With section standalone, endsection is its own LiquidTag node, so the report moves to a checker keyed on endsection. The message and range are the same, and it matches Ruby Liquid (Shopify core asserts Unknown tag 'endsection' for the legacy block form, and the parity corpus expects it).
That said, it's only here because the parser change moves endsection. If we keep the hybrid tag and only fix the scan (see the thread on ast.test.ts), this goes away and checkSectionTag stays as it is on main.
| // Re-export all tag definition types so existing imports from './environment' keep working. | ||
| export { | ||
| TagKind, | ||
| isStructuralEndTag, |
There was a problem hiding this comment.
Plumbing, not API. liquid-tags.ts already imports TagKind from ../environment, so I put the helper there too. But the re-export comment says it's there "so existing imports keep working", and the tag files import from ../tag-definitions directly, so a new symbol arguably doesn't belong here. environment isn't exported from the package index.ts, so nothing external sees it. If we keep this approach I'll import from ../tag-definitions and drop the re-export.
| } | ||
| }); | ||
|
|
||
| it('should throw on orphaned endsection', () => { |
There was a problem hiding this comment.
why was this here? why does removing hybrid tag change it?
There was a problem hiding this comment.
On main, any end tag for a registered tag with no opener is a parse error ({% endrender %}, {% endif %}, …). section is registered, so a lone {% endsection %} threw too. With the hybrid form, a paired endsection was legitimate, and this test pinned that difference: paired parses, orphan throws.
Once section isn't a block, every endsection is effectively an orphan. Left alone, the legacy {% section %}…{% endsection %} form would go from "Unknown tag, other checks still run" to "whole file fails to parse" (theme-check skips every other check on files that fail to parse). So I exempted endsection from that throw, which is what flips this test. Other tags' end-tag errors are unchanged, and there's a regression test for that.
This, the checker move and the new export all follow from removing the block form. They also cause a few remaining differences on already-invalid Liquid, which the tophat caught: different messages for a lone endsection, one in {% liquid %} and one inside HTML, and Prettier output changes for those. <p>{% section 'x' %}y{% endsection %}</p> gets mangled.
Alternative with zero behavior change: keep section hybrid and replace the per-tag forward scan with one pass that pairs each section with its endsection (like matching parentheses), per document and per {% liquid %} block. That's still linear in the worst case, and theme-check, Prettier and all existing tests stay as on main. Removing the block form could then be a separate follow-up where the behavior change is the explicit point. I'm leaning that way. I'll check with CP, since removing hybrid was their suggestion.
Ruby Liquid has no block form for section, but the parser treated it as a
hybrid tag and scanned forward for a matching endsection on every section
tag (both in documents and in {% liquid %} bodies). Files with many section
tags parsed in quadratic time.
Make section a plain TagKind.Tag and remove the hybrid tag kind. Theme
Check still reports a stray endsection as "Unknown tag 'endsection'" by
mapping the parser error.
The previous commit made a stray endsection throw "Attempting to close LiquidTag 'section'". In a full Theme Check run that turns the file's AST into an error, so the only offense was LiquidHTMLSyntaxError and every other check skipped the file. Only tags with a body (Block/Raw) have end tags, so only those now throw the structural close errors. endsection parses as an unknown tag like any other end* name, and LiquidSyntaxError reports it as "Unknown tag 'endsection'".
laxRecoverTagMarkup only recovers Tag/Block kinds, so section (previously
Hybrid) was never recovered. Now that section is a Tag it would be, and
{% section 'x' junk %} would recover and render, while Ruby's section tag
raises in every error mode. Exclude it explicitly to keep the previous
behavior.
The previous change stopped throwing structural close errors for every
registered tag without a body, so stray {% endrender %}, {% endecho %},
{% endsections %}, etc. silently parsed as unknown tags instead of failing
as they do on main. Only endsection needs the exception. Restore the
existing behavior for everything else and add a regression test.
75220dc to
334cac7
Compare
This branch has not been deployed
1 participant
WHY are these changes introduced?
h2 env pullwraps values that need quoting in double quotes and backslash-escapes\,"and tabs (added in #3050). dotenv (used byh2 dev,h2 env pushandh2 deployvia cli-kit'sreadAndParseDotEnv) has no general escape syntax. In double-quoted values it only expands\nand\r. So escaped characters end up in the parsed value:env pulltodaya\b"a\\b"a\\bsay "hi""say \"hi\""say \"hi\"a<TAB>b"a\tb"a\tb(literal backslash-t)Separately,
$and backticks aren't escaped inside the double quotes. Shell-sourcing isn't a supported use of the generated file, but if someone runsset -a; . .env,$(...)and backtick values execute.WHAT is this pull request doing?
Changes
quoteEnvValueinenv pull:KEY=abc123).'or line breaks. dotenv reads single-quoted values literally, and so do POSIX shells, so$, backticks,\and"all survive and nothing expands if the file is sourced.'or line breaks fall back to double quotes with only\n/\rescaped, because that's all dotenv unescapes.Tests assert round-trips through
readAndParseDotEnvinstead of the exact file format. That covers a fresh file and patching an existing.envthat has comments, neighbouring keys and multiline values (plus a re-pull being a no-op). There's also a POSIX-only test that shell-sources the output and checks nothing executes.Known limitations (none of these are new):
'or a line break and both"and#still can't be written in a way dotenv parses back. As far as I can tell dotenv has no way to write it.$or backticks are still not safe to shell-source. That's unsupported anyway.loadEnv(only used by mini-oxygen as a fallback when no env bindings are passed) runs dotenv-expand, so$VARin values is still expanded on that path regardless of quoting. That's out of scope here.HOW to test your changes?
On the old implementation, the round-trip, patch-existing-file and shell-sourcing tests fail.
I also fuzzed this locally with about 20k random values made of quotes,
$, backticks, backslashes,#, whitespace, control chars and line breaks, using cli-kit 3.80.4 / dotenv 16.4.7:'or newline plus"and#" case above.Manual (not done yet):
pa$s"\wordand another toit's.h2 env pulland confirm they're written as'pa$s"\word'and"it's".h2 devand confirm the worker sees the exact values.Post-merge steps
@shopify/clipins@shopify/cli-hydrogen, so after the next cli-hydrogen release the pin needs bumping in Shopify/cli for most users to get this.Checklist