liquid: strict_variables throws exception on checking if variable exists

I’m using a jekyll layout where I want to include content of pages, depending on whether it exists. Apparently, it is supposed to be done like described in https://github.com/Shopify/liquid/issues/89 In my case it would look as follows:

{% assign title = page.title %}
{% if page.name %}
    {% assign title = page.name %}
{% endif %}
<title> {{ title }} | {{ site.title }} </title>

But since I have strict_variables set to true, which is really useful for development, Liquid throws an exception on building (undefined variable name included).

In my opinion, strict_variables should not throw exceptions for cases where the undefined variable is checked for existance.

About this issue

  • Original URL
  • State: open
  • Created 6 years ago
  • Reactions: 17
  • Comments: 23 (7 by maintainers)

Commits related to this issue

Most upvoted comments

I’ve found that the Liquid contains operator works well for existence testing variables. Shouldn’t this be the canonical way to test for fields?

Example:

{% if page contains "name" %}
   Name: {{ page.name }}
{% endif %}

A brief reminder that this issue is Insanely Stupid ™ and makes strict_variables unexpectedly useless. It’s like building a house with no door. Or a swimming pool in the desert. Don’t mean to offend anyone, I’m sure you guys are busy and all. It’s just clearly, aesthetically, stupid.

I would also like to be able to check if a top-level variable is defined, not just keys on hashes or methods on drops.

{% if defined page %} feels like the best option to make clear that it’s a special case where strict_variable checking is not being used and it makes it clear that it’s separate from being defined but nil.

Just adding my 👍 as we’d love to enable strict_variables on our project (in our case, a Jekyll site) but we rely heavily on checking for variable existence in our templates, so we can’t use this as currently implemented.

(thanks also for a great templating library!)

Here is another 👍 as I was hoping to use strict_variables too to avoid dumb mistakes - but it doesn’t work as existence check is used in not only my templates but also many themes.

I would say to be backwards compatible it should be that page.missing_key is allowed, but page.missing_key.value should give error and then have a more explicit operator to get better checks in new code.

imo:

  • check for existence via if should be possible regardless of whether the variable is defined.
  • an exception should be raised when attempting to render the contents of said variable

Thus the following code would be exception-free. last_name is not defined, but it is only used within a guarded if block:

content = "Hello {% if last_name %}{{last_name}}{% else %}placeholder{% endif %}"

template = Liquid::Template.parse(content, error_mode: :strict)
template.render!({}, {strict_variables: true})

Hello placeholder

I’m surprised this behavior is not supported.

Re: docs, Using contains with hashes is mentioned on the Wiki, but not on the gh_pages branch page.

https://github.com/Shopify/liquid/wiki/Liquid-for-Designers#if--else

contains will work for container types like hashes, but I don’t think it will work for drops, which don’t respond to include?. https://github.com/Shopify/liquid/blob/e83b1e415990894c9517f94a8c2020ff825da027/lib/liquid/condition.rb#L20-L27

Though modifying the implementation of contains to call key? if it exists could work. Or we could alias Drop#include? to Drop#key?. All are slightly breaking changes but likely safe.

This article on Jekyll plugins taught me that plugins can just be dumped in a _plugins folder, so your filter seems to be working in my Jekyll instance now, @pascalbetz. Thanks!

@pascalbetz, that custom filter looks exactly like what I’m after – until this issue is resolved, at least. I’m just wondering how you “install” such a custom filter into a Jekyll site. Do I need to create a whole repository, gem, bundle and all to host those 6 lines of code to say plugins: "jekyll-key" in my Jekyll site’s _config.yml or can a Jekyll plugin reside “locally” within the Jekyll site, somehow?

Has a syntax been chosen?

It seems like there are a lot of good ideas and one needs to be chosen and implemented.

Defined and present (truthy/falsey) are two different things, IMHO.

I would prefer to be able to check if:

  • a key is defined
  • a value from an existing key is present

while having strict_variables: true

To achieve this I had to jump through some hoops:

I added a custom filter

module Filters
  def key(object, element)
    object.key?(element)
  end
end
Liquid::Template.register_filter(Filters)

And then used it in my template like so

{%- assign hasKey = data | key: 'bandwidth' %}
{%- if hasKey and data.bandwidth %}
  Bandwith: {{ data.bandwidth }}
{%- endif %}
template.render!("data" => {}) #=> ""
template.render!("data" => {"bandwidth" => nil}) #=> ""
template.render("data" => {"bandwidth" => 1234 }) #=> "Bandwidth: 1234"

while direct access to undefined keys still raises.

I think we need to decide whether we want to expose the concept of undefined as being distinct from nil. Currently, strict_variables has that distinction and it can help with typos or variable renames that miss some usage. However, it also requires either making sure the variable is always initialized (e.g. explicitly set to nil) or that we add something new to explicit check for undefined or handle the undefined value (e.g. using a filter like the default filter).

Alternatively, we could decide we don’t really want that concept of undefined and treat undefined as nil. That is basically how strict_variables works. This would also be more consistent with the already recommended way of checking for the existence of a variable. We could still make things more strict about where nil could be used in even in this case, like doing a variable lookup on nil (e.g. page.name when page is nil). We could also try to do static analysis in the future to determine where a variable lookup always returns nil as a way of making it more strict.

I think we should try to avoid mixing concepts for consistency, because it would lead to unexpected behaviour. For instance, if we treat the {% if some_var %} as a special case, then it would lead to the expectation that {{ some_var | default: other_var }} would work. If we special case the default filter as well, then it would seem like other filters might be able to work with an undefined value. So I think we would end up with either special cases that we would have to document or something more like javascript where undefined is an actual value which application code would have to be updated to handle in a backwards compatible way.

  • make this work more like a Ruby hash: allow access to missing keys but raise if accessing a key on a missing value

I think this probably makes the most sense, since it will be conceptually consistent with how liquid works without strict_variables where there isn’t a distinction between undefined and nil.

Do we have an update on this?

This is a really good point, thanks for opening the issue. Some options:

  • your suggestion of using if as a key existence operator in strict_variables mode
  • new filter ( | has: 'name') or operator (if defined page.name or if page has name) to check existence of a key
  • make this work more like a Ruby hash: allow access to missing keys but raise if accessing a key on a missing value