Support liquid strict_variables

[nathanweeks]

When developing a Jekyll site, it can be useful to set the Liquid strict_variables option to prevent accidentally using an undefined variable (e.g., typo, or when a property that is expected to exist in a data file doesn't actually exist). However, there are a number of places in this theme where a variable / property can optionally exist---checked for with an if tag---and the Liquid implementation used by Jekyll has a bug that causes an exception to be thrown when the existence of an undefined variable is checked for in this manner.

Enabling strict_variables in _config.yml:

liquid:
  strict_variables: true

Results in the following error:

$ bundle exec jekyll serve
...
  Liquid Exception: Liquid error (line 1): undefined variable tools_menu in /workspaces/jekyll-starter-legumeinfo/_themes/jekyll-theme-legumeinfo/_layouts/default.html
                    ------------------------------------------------
      Jekyll 4.2.2   Please append `--trace` to the `serve` command 
                     for any additional information or backtrace. 
                    ------------------------------------------------
/usr/local/rvm/gems/ruby-3.2.2/gems/liquid-4.0.4/lib/liquid/variable_lookup.rb:62:in `block in evaluate': Liquid error (line 1): undefined variable tools_menu (Liquid::UndefinedVariable)
...

A workaround (in most cases) is to use the contains operator.

[alancleary]

Thanks for pointing out this issue and providing a solution @nathanweeks. Feel free to open a PR, otherwise this issue will be included in the upcoming "bring things back to the theme" triage.

[alancleary]

Just noticed this is in the starter repo. I going to move it to the theme repo since that appears to be the origin of some of the errors.

[alancleary]

@nathanweeks I went ahead and made all the code changes necessary for the theme to pass the strict variables test and, honestly, I don't think it's worth it. The code is no longer idiomatic and in some cases it's outright mangled. I can push the changes if you would like to have a look for yourself but it's definitely going to take some persuasion for me to merge these changes into main.

[alancleary]

@nathanweeks I put the changes in the strict-variables branch. After reviewing the changes I think I could actually live with it. Sorry for waffling! Let me know if this suites your needs and I can open a PR.

[sammyjava]

:waffle:

[alancleary]

@nathanweeks Apparently I really dropped the ball on this one; I just remembered your draft PR that makes the theme pass the strict_varaibles test. So I guess my strict-variables branch basically replicates what you already did! However, one gotcha that slipped through in the PR is truthiness checking.

The nice thing about the old syntax is that {% if some.variable %} also checks truthiness. If the variable doesn't exist, then it's falsy, and if the variable exists but it's falsy then the statement is still falsy. In the new syntax, though, you have to check the thruthiness expliclity, i.e. {% if some contains "variable" and some.variable %}. This was part of my reluctance to adopt the new syntax, although I've gotten over it.

Anyway, let me know if this is still a workable solution for you.

[sammyjava]

Of course I looked to see if GitHub has a dropping-ball icon, but apparently not. Alan has opened a Pandora's Box by making me aware of the extensive GitHub icon library. :smiley: :balloon: (A balloon is sort of a dropping ball, it just goes in the opposite direction.)

[alancleary]

Of course I looked to see if GitHub has a dropping-ball icon, but apparently not. Alan has opened a Pandora's Box by making me aware of the extensive GitHub icon library. 😃 🎈 (A balloon is sort of a dropping ball, it just goes in the opposite direction.)

:open_hands: :baseball:

[nathanweeks]

@nathanweeks Apparently I really dropped the ball on this one; I just remembered your draft PR that makes the theme pass the strict_varaibles test. So I guess my strict-variables branch basically replicates what you already did! However, one gotcha that slipped through in the PR is truthiness checking.

The nice thing about the old syntax is that {% if some.variable %} also checks truthiness. If the variable doesn't exist, then it's falsy, and if the variable exists but it's falsy then the statement is still falsy. In the new syntax, though, you have to check the thruthiness expliclity, i.e. {% if some contains "variable" and some.variable %}. This was part of my reluctance to adopt the new syntax, although I've gotten over it.

Good point! I had missed that.

Anyway, let me know if this is still a workable solution for you.

Testing the jekyll-theme-legumeinfo strict-variables branch with jekyll-starter-legumeinfo & strict_variables: true, the following error appears:

Liquid Exception: Liquid error (/app/_themes/jekyll-theme-legumeinfo/_includes/navbar.html line 1): undefined variable alerts included in /app/_themes/jekyll-theme-legumeinfo/_layouts/base.html

A one-line update to the theme resolves the error:

--- a/_includes/alerts.html
+++ b/_includes/alerts.html
@@ -1,4 +1,4 @@
-{% if site.data.alerts %}
+{% if site.data contains "alerts" %}
 {% for alert in site.data.alerts %}
 <div class="uk-alert-{{ alert.type }}" uk-alert>
   <p>{{ alert.message }}</p>

Thanks!

[alancleary]

Good catch @nathanweeks. This has revealed a bug that I haven't been able to find a way around: Apparently the site variable is actually a drop so the contains syntax we've adopted doesn't work, i.e. {% if site contains "variable" %} will always be falsy whether or not variable is defined in _config.yml. The documentation on drops is very poor in general and non-existent in the Jekyll documentation so I'm at a loss for what to do here.

[nathanweeks]

Good catch @nathanweeks. This has revealed a bug that I haven't been able to find a way around: Apparently the site variable is actually a drop so the contains syntax we've adopted doesn't work, i.e. {% if site contains "variable" %} will always be falsy whether or not variable is defined in _config.yml. The documentation on drops is very poor in general and non-existent in the Jekyll documentation so I'm at a loss for what to do here.

I wasn't familiar with the concept of a "drop"; thanks for the reference. In this case, it's site.data contains "alerts", which seems to work as hoped?

[alancleary]

Yes, site.data contains "alerts" works as expected, but all checks for variables that may or may not be present in _config.yml are all evaluating to false! (For whatever reason I didn't notice until testing your alerts change.)

After poking and prodding the site drop I've discovered that it has a keys variable containing an array of all the keys present in the drop. Since the contains operator works on arrays we can use this to check if variables are defined in _config.yml, i.e. {% if site.keys contains "blog_card_item_limit" %}.

I'll update this PR to use that syntax.

[alancleary]

@nathanweeks I pushed a commit to the strict-variables branch that fixes the site contains syntax. Mind checking if the _config.yml variables soybase is using work correctly with the branch?

[nathanweeks]

Thanks, Alan! Using the updated strict-variables branch, I get an error here (looks like this use of site was missed):

https://github.com/legumeinfo/jekyll-theme-legumeinfo/blob/cbb44ac0c0d3608d35efc40910f8567c965397f8/_includes/blog-card.html#L5

[alancleary]

Good catch @nathanweeks! Apparently the regular expression I used to identify problematic site usage had some blind spots. I fixed the bug you spotted and a couple others I missed the first time around. Mind taking it for another spin?

[nathanweeks]

Thanks @alancleary, it works now! FWIW I did have to uninstall the jekyll-sitemap plugin (which doesn't work with strict_variables: true : https://github.com/jekyll/jekyll-sitemap/issues/272) --- just commenting it out of the _config.yml didn't work.