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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

knu
Copy link
Contributor

@knu knu commented Jul 13, 2015

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.

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.
@fw42
Copy link
Contributor

fw42 commented Jul 13, 2015

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.

@fw42
Copy link
Contributor

fw42 commented Jul 13, 2015

Does this bug exist in the strict parser as well?

@nickpearson
Copy link
Contributor

It's not ideal, but you can work around this with a capture. Using your test case ({{funk | replace: "}", '}}' }}) as an example:

{% capture match %}}{% endcapture %}
{% capture replacement %}}}{% endcapture %}
{{ funk | replace: match, replacement }}
# the above Liquid is in the `text` variable
Liquid::Template.parse(text).render({ 'funk' => '{x}' })
#=> {x}}

@trishume
Copy link
Contributor

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.

@pushrax
Copy link
Contributor

pushrax commented Jul 13, 2015

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 :(

@dylanahsmith
Copy link
Contributor

We should try implementing this in liquid-c, since performance was the main concern with #170. #170 also seems to have implemented support for \ escaping, but I don't know if that is safe to add without breaking things.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is [^'"{}]+| needed?

Copy link
Contributor Author

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.

@dylanahsmith
Copy link
Contributor

This is missing integration tests, which are preferred since they get used with liquid-c.

@knu
Copy link
Contributor Author

knu commented Jul 14, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants