-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Allow use of curlies in a properly quoted string #623
base: main
Are you sure you want to change the base?
Conversation
Currently Liquid does not even allow a closing curly brace to be put inside of a variable, but you sometimes need to deal with the character.
Hm I think we tried to fix this before and we didn't merge it because it negatively impacted performance by quite a bit. @trishume might remember. |
Does this bug exist in the strict parser as well? |
It's not ideal, but you can work around this with a
# the above Liquid is in the `text` variable
Liquid::Template.parse(text).render({ 'funk' => '{x}' })
#=> {x}} |
See #170 I don't think the strict parser would help since this is an issue with the tokenizer, which comes before the strict parser. It is possible that liquid-c could fix this without a performance hit but that would make it act differently than base liquid. |
The problem is that we separate the tokenizer and parser, which is necessary for the regex approach but not for the strict approach. I don't think we're at the state where we could merge the two though :( |
@@ -35,7 +35,7 @@ module Liquid | |||
QuotedFragment = /#{QuotedString}|(?:[^\s,\|'"]|#{QuotedString})+/o | |||
TagAttributes = /(\w+)\s*\:\s*(#{QuotedFragment})/o | |||
AnyStartingTag = /\{\{|\{\%/ | |||
PartialTemplateParser = /#{TagStart}.*?#{TagEnd}|#{VariableStart}.*?#{VariableIncompleteEnd}/om | |||
PartialTemplateParser = /#{TagStart}.*?#{TagEnd}|#{VariableStart}(?:(?:[^'"{}]+|#{QuotedString})*?|.*?)#{VariableIncompleteEnd}/om |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is [^'"{}]+|
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean it could be (?:#{QuotedString})*?|.*?)
? No, that would only save a quoted string at the beginning of a variable block (which normally begins with a variable name) and the following .*?
would stop at the next }
that might be in the next quoted string.
I included {}
in the character class because it looked like the tokenizer has to be generous with curly brace mismatch for the "lax" parser to work.
This is missing integration tests, which are preferred since they get used with liquid-c. |
Thanks for taking a look, guys. I actually didn't know a fix for this had once been rejected due to performance issues, but if there's any kind of benchmark index/threshold Liquid must surpass, I'm willing to improve the implementation. |
Currently Liquid does not even allow a closing curly brace to be put inside of a variable, but you sometimes need to deal with the character.