Fix VariableNotExistException for variable knowingly set to null #295
Conversation
Currently, we would get a VariableNotExistException when in strict mode,
even if a page property, for example, is knowingly set to null.
This is specifically important since {% if val %} returns true even for
empty strings, making it almost impossible to avoid an exception and
still support properties like page.last_modified_at.
Introduce a "found" state when traversing structures, and only consider
a non-existant variable if found=false
This change breaks backwards compatibility with classes implementing
LookupNode.Indexable. This is considered low-risk at the moment.
|
@kohlschuetter having the variable at context level needs some more explanation. Description said about missing variable and nullable variable difference. I agree somehow about this bug. As for me solution may be in use Map implementation that permits null values, use return type something more then value or null (some wrapper with states: not_found, found_null, found_value). But this global for template context not clear how will solve the issue. |
The current "strict variables" checking strategy in Liquid Templates is
somewhat unusable as one cannot even check for the existence of an
object property without raising an error in strict mode.
This is a known limitation, and actively being discussed upstream
(liquid issue 1034).
Several Jekyll template make use of checks like {% if page.mermaid %},
etc., preventing enforcement of strict mode.
This patch adds a "sane" strict variable policy, which allows for such
checks to succeed.
Since this is new functionality specific to liqp, we keep the existing
strict mode configuration unchanged.
Shopify/liquid#1034
|
@msangel sorry, I just saw your message. I've added another change that depends on this one, which is tangentially related ("sane" strict variable checks) Regarding your question: would fail without this patch (also see "paleo"'s comment in Shopify/liquid#1034) I thought about adding a wrapper, but then we would have to rely on all participating classes, which is hard to enforce. Adding a "found" property looks like the simpler approach. Do you think there's a situation we currently don't cover? |
Previously, only "obj.missing" was allowed.
kohlschuetter
changed the title
|
I dug the issue and agree that we must provide a fix. What I still think is wrong in this change is: We have global flag for anyone for everyone for any variable. This goes against thread safety, immutability (which we are trying to achieve) and static behavior. Can this functionality be done without such flags? @kohlschuetter, don't worry about backward compatibility, a lot of recent commits are breaking compatibility, so next version probably will have own major version set. There a lot of |
Currently, we would get a VariableNotExistException when in strict mode, even if a page property, for example, is knowingly set to null.
This is specifically important since {% if val %} returns true even for empty strings, making it almost impossible to avoid an exception and still support properties like page.last_modified_at.
Introduce a "found" state when traversing structures, and only consider a non-existant variable if found=false
This change breaks backwards compatibility with classes implementing LookupNode.Indexable. This is considered low-risk at the moment.