Jekyll 4.0 gets confused about page variables

[kvz]

Hi, congrats on launching Jekyll 4! Very much enjoying the speed boost.

While upgrading our site, I was syncing _site to a local empty git repo to diff changes. One thing I noticed, was that Jekyll seems to leak variables between pages.

Here's code I had in my default layout:

            {%if page.sidebar and page.sidebar != ""%}
              {%assign sidebarClass = page.sidebar%}
              {%capture sidebar%}
                {%include {{page.sidebar}}.html%}
              {%endcapture%}
            {%endif%}

            <!-- We're not including the sidebar directly, so the CRM can fill its contents too -->
            <aside class="sidebar {{sidebarClass}}">
              <!-- SIDEBAR_START -->
              {{sidebar}}
              <!-- SIDEBAR_STOP -->
            </aside>

Now some pages would set sidebarInclude: about to indicate they'd like to have the 'about sidebar' included by the default layout.

After upgrading to 4.0, pages that didn't want the about sidebar loaded, got one anyway, until I added these two lines at the top:

            {%assign sidebarClass = ""%}
            {%assign sidebar = ""%}

It would appear that maybe caching makes variable scope leak over to different pages(?). This is what my very limited understanding from Jekyll/Ruby would lead me to believe, but it could be something else entirely.

[dcabo]

Hi. We recently upgraded our site to Jekyll 4.0 and just noticed a new behaviour that seems to match what is described here. Very roughly, we have a "project" attribute for all pages except the home, that gets assigned into another variable. Somehow the homepage was being rendered as if the variable was set.

[ashmaroli]

Thank you reporting this guys. I think it'll be easier to debug if one of you provided a sample repository that I could just clone and run.. (and hopefully it being light-weight as well..)

[dcabo]

Our repo is quite huge at the moment, but if @kvz's one is also big I'll try to reproduce the problem in a small subset of it. Thanks for looking at this.

[kvz]

kvz at xps in ~/code/content on master
$ du -hs .
8.3G	.

i wonder who wins? :)

[dcabo]

@kvz it could be just a bunch of big JPEGs :P

[dcabo]

I'll try to create a small version of our repo over the coming days...

[dcabo]

@ashmaroli I created a small repo reproducing this issue.

The repo (dcabo/jekyll-4-issue-7811) is private because the original one was so, although I think I got rid of most of the private stuff. You should have got an invitation. If it being private is a nuisance let me know, I can probably take a closer look and make it public.

The steps to reproduce the issue are detailed in README.md. I got rid of most of the site structure to expose it better, but there are still a bunch of include files, which you can ignore.

Any question, please let me know.

[ashmaroli]

@dcabo Thank you very much for providing the test repo.
I was able to reproduce your issue with Jekyll 4.0.0 and current master.

The source of this issue is at:

jekyll/lib/jekyll/liquid_renderer/file.rb

Line 13 in 9f389e0

@renderer.cache[@filename] ||= Liquid::Template.parse(content, :line_numbers => true)

If you're building your site locally and are comfortable with the Gem environment in your system, then you may make the following change within your gem itself. Otherwise, the best alternative is to make the change in a temporary branch on your fork of the Jekyll repo and point your site's Gemfile to that branch.

- @renderer.cache[@filename] ||= Liquid::Template.parse(content, :line_numbers => true)
+ @renderer.cache[@filename] = Liquid::Template.parse(content, :line_numbers => true)

# Gemfile

gem "jekyll", github: "dcabo/jekyll", branch: "patch-7811"

Once I can come up with a solution to disable the Liquid cache via the config file and open a PR, you (and others affected) can elect to point to that PR's branch until v4.1 ships.

Wish this issue had surfaced while we were still in the beta phase..

The inconvenience caused is deeply regretted.

[dcabo]

@ashmaroli happy to help, and no worries, bugs happen. 🤷‍♂ I have a workaround working where I set the variable in all cases, just as you proposed in my repo, but thanks for the suggestion on how to fix the gem.

[letrastudio]

Seeing as how this bug might be silently breaking a lot of sites, and has apparently such a simple fix, wouldn't it be good to ship the fix in a 4.0.1 patch release? 4.1 seems ambitious, so I'm guessing it might still take a while.

I already followed @ashmaroli's advice and implemented the fix in a fork of Jekyll:

the best alternative is to make the change in a temporary branch on your fork of the Jekyll repo and point your site's Gemfile to that branch.

The problem with that solution is that Netlify won't cache the Jekyll version, so it has to build from source for every deploy — negating any of Jekyll 4's performance improvements and eating up precious build minutes.

[ashmaroli]

@letrastudio We can't ship the workaround immediately because if Liquid templates are no longer cached, the build times will increase significantly.

[letrastudio]

Ah, gotcha. So this isn't a real fix, just a temporary workaround with a performance tradeoff?

- @renderer.cache[@filename] ||= Liquid::Template.parse(content, :line_numbers => true)
+ @renderer.cache[@filename] = Liquid::Template.parse(content, :line_numbers => true)

[ashmaroli]

Yep. It is a workaround for those who can't adjust their layouts / includes within to get around the issue...

[kierenpitts]

I have slightly different example of this issue causing a problem in case it helps anyone else. I am writing a custom plugin and the plugin loops through some data creating instances of one of two Ruby classes that inherit from Page and concatenating them into site.pages. The output is two sorts of related pages (one is a page listing posts appearing in a month, the other is a page listing months that have posts). In the initialize function for each Class there is a line like this to set the layout (the filename is slightly different for the two page types and therefore in each Class):-

self.read_yaml(File.join(@site.source, '_layouts'), 'some_specific_layout.html')

(The above is based on the second example given in the documentation here: https://jekyllrb.com/docs/plugins/generators/)

However, when the plugin runs, the second set of pages will use the _layout file specified in the Class that was instantiated first in the code even though a different _layout file is specified in the initialize function of the second class. If I comment out the code that creates the first set of pages then the layout for the second set is used correctly.

This issue is also fixed by making the temporary change @ashmaroli suggested in Sept.

[ashmaroli]

@kierenpitts Is your plugin open-source? If yes, I'd like to take a look at it.
I'm having trouble understanding how caching is affecting your scenario.

[kierenpitts]

@ashmaroli It's very much a work in progress (and somewhat niche for a site I'm working on) but I can try and create a small example of the issue for you. Basically the _layout specified in one Class seems to be leaking into the second even though it is declared a second time. It could be I'm doing something daft but making the change you suggested in Sept does fix the issue too.

[ashmaroli]

create a small example of the issue

That would be great. I just need to see how caching Liquid is affecting data from front matter..

[kierenpitts]

Hi @ashmaroli

I've created a small Jekyll site with a custom plugin that demonstrates the problem:
https://github.com/kierenpitts/jekyll-issue-7811-example

I've put some info in the README but there's more in the /index.html that the site generates.

I'm fairly new to Jekyll (and Ruby; I generally write Python) so this could easily be something that I'm doing wrong. However, I can't immediately see why the two different page types end up using the same layout file when different ones are specified in the custom plugin - the code works fine if I make the same change as you suggested above.

Hope this helps.

[ashmaroli]

@kierenpitts Thank you for providing the repository and plugin code.
The reason Jekyll 4.0, Liquid template cache is affecting your plugin is because the Page subclass instances have the same relative_path attribute (all equaling "").
If you assign each instance a unique relative_path, your issue will resolve automatically.

[kierenpitts]

@ashmaroli Thanks for the reply. Is the cache keyed on the relative_path then? I assumed that, as there's no source file for the listings pages I generate that I didn't need to specify a relative_path - one isn't specified in the generator example in Jekyll docs so I'm perhaps confused.

[ashmaroli]

@kierenpitts Yes, the cache is keyed on the relative_path since no two files can exist on the same relative path on disk. The cache was introduced so that Jekyll wouldn't waste time parsing the same layout contents for every page / document rendered.

In your test-plugin's case, the content attribute of your Page subclasses are dynamically injected for the same relative_path. Instead of working against the cache, you may be able to work with the cache by using different layout keys in the object's data hash. i.e.

#<Jekyll::NewsItemPage @relative_path="" @data={"layout"=>"news_by_month", "title"=> ...}>
#<Jekyll::NewsListingPage @relative_path="" @data={"layout"=>"news_browse", "title"=> ...}>

[kierenpitts]

@ashmaroli Thank you for the clarification and the suggestion. As you said, adding in a unique @relative_path like this to each of the initialize functions fixes the clash:

@relative_path = 'somethinguniquetoeachsubclass'

In terms of the cache, and thinking that this sort of key clash might be really hard for people to track down (especially if using several generator plugins that work fine in isolation but cause a clash when running together), would it be worth having the cache generate a fake but functionally unique relative_path (i.e. a long, random, alphanumeric) if the relative_path for a page is empty? This would prevent this sort of clash in the cache and hopefully wouldn't significantly impact the cache.

Thank you for the help with resolving the issue in my plugin, it's much appreciated.

[ashmaroli]

Not really sure about this, but I think this could be a bug with Liquid (or just its assign tag) instead..
On running the following Ruby script (without involving jekyll)...:

require 'liquid'

CONTENTS = <<~TEXT

       Neo: "I am just {{ title | default: 'a regular guy' }}.."
  {%- if agent == '   Smith' %}{% assign title = 'A DISEASE' %}{% endif %}
  {{ agent }}: "Neo, you're {{ title | default: 'The One' }}!!"
TEXT

TEMPLATE = Liquid::Template.parse(CONTENTS)

puts TEMPLATE.render("agent" => "Morpheus")
puts TEMPLATE.render("agent" => " Trinity")
puts TEMPLATE.render("agent" => "  Oracle")
puts TEMPLATE.render("agent" => " Trinity")
puts TEMPLATE.render("agent" => "   Smith")
puts TEMPLATE.render("agent" => "  Oracle")
puts TEMPLATE.render("agent" => " Trinity")
puts TEMPLATE.render("agent" => "Morpheus")

...outputs the following:

     Neo: "I am just a regular guy.."
Morpheus: "Neo, you're The One!!"

     Neo: "I am just a regular guy.."
 Trinity: "Neo, you're The One!!"

     Neo: "I am just a regular guy.."
  Oracle: "Neo, you're The One!!"

     Neo: "I am just a regular guy.."
 Trinity: "Neo, you're The One!!"

     Neo: "I am just a regular guy.."
   Smith: "Neo, you're A DISEASE!!"

     Neo: "I am just A DISEASE.."
  Oracle: "Neo, you're A DISEASE!!"

     Neo: "I am just A DISEASE.."
 Trinity: "Neo, you're A DISEASE!!"

     Neo: "I am just A DISEASE.."
Morpheus: "Neo, you're A DISEASE!!"

[kierenpitts]

Isn't assign a bit odd in its behaviour as it is written to the root scope and then deletes from child scopes? I'm not sure why the upgrade to Jekyll 4 and the caching would trigger that issue for the OP now though.

[ashmaroli]

I'm not sure why the upgrade to Jekyll 4 and the caching would trigger that issue for the OP now..

The upgrade to Jekyll 4 introduced the caching. The caching in turn simulated the behavior of the Ruby script above (i.e. 1 Liquid::Template instance responding to multiple render calls). In older versions of Jekyll, each call to render would create a new Liquid::Template instance thereby having a fresh template parse-context...

[ashmaroli]

@kvz, @dcabo I've opened a pull request that should effectively resolve this issue.
I would appreciate it if you guys could leave a feedback (here) from testing that branch with your sites (minus the workarounds, if any) and also let me know if there's any unexpected or new side-effects:

# Gemfile
gem "jekyll", git: "https://github.com/jekyll/jekyll.git", ref: "refs/pull/7967/head"

Thank you.

[kvz]

I'm using Jekyll through a custom docker image and I've tried many times, but failed to add custom gems to it through a Gemfile so I'm afraid this is going to be too tricky to test for me 🙀

[ashmaroli]

@kvz No pressure.
The branch already builds @dcabo's sample repository as expected. I just wanted confirmation that there isn't any new side-effect from the proposed change.

[letrastudio]

@ashmaroli I've tested your branch and the problem appears fixed on my end — no side effects that I could see in a diff of my _site folder. And in terms of performance, it's even faster than 4.0.0 — I'm getting build times of around 17-19 seconds instead of 25-29. I don't know whether that optimization comes from this change or some other post-4.0.0 code, but big thumbs up from me! 👍

[ashmaroli]

it's even faster than 4.0.0

@letrastudio Thank you for the feedback. Happy to know there are no discernible side-effects from the proposed change.
Regarding the performance gain, it is because the current master has cached results in the URLFilters as well — a nice contribution from one of our users.

[dcabo]

@ashmaroli I rebuilt my site using your branch, and the output looks fine: no differences in _site compared with the 4.0 gem. And it's significantly faster for me also, a ~30% improvement! I then removed the workaround I had in place, and using your branch the site seems to work fine. (I double-checked, to be sure I had removed the workaround properly, and I could see the original error again with the 4.0 gem.)

So it all looks fine, thanks. Is this going into a new release soon? Because I'm tempted to use your branch in production straight away, given the performance improvements.

[ashmaroli]

Thank you for the feedback @dcabo. Hopefully, the pull request would get merged into master soon. The release however will take slightly longer to cover more issues.
Either ways, thank you very much for providing the test repository. It played a significant role in all of this.

[letrastudio]

I think I've found an edge case where there's still a problem.

@sverrirs just released a 3.0.0 version of jekyll-paginate-v2 that's simply a rollback to 1.9.4 with a relaxed Jekyll version requirement, and I've been testing it today. The plugin itself seems to work perfectly, but I've found some sort of weird collision where, if pagination is enabled, this Jekyll 4 bug predictably manifests itself. Or it might be a different bug with similar symptoms.

@ashmaroli I know this might be a bug in jekyll-paginate-v2, but since you're associated with both codebases I'm hoping you might be able to look into it. For this case, I'm getting exactly the same results in 4.0.0 and with your PR, and the issue doesn't come up in Jekyll 3:

jekyll version	pagination	result
3.8.6	disabled	✅
3.8.6	enabled	✅
4.0.0	disabled	✅
4.0.0	enabled	🐛
#7967	disabled	✅
#7967	enabled	🐛

The problem I'm seeing is the {{ content }} variable getting mixed up when it contains liquid code. Using {{ page.content }} always returns the right content, but the liquid is of course not processed. If it's just markdown it's fine. If pagination is disabled, either at the page level or globally, the bug disappears.

I've published a public repo with a trimmed-down version of my site that reproduces this bug. I hope it helps to diagnose this issue. The repo's README includes more details and a gh-pages branch with a sample build where the bug is present. If there's anything more I can do to help fix this, please let me know.

[ashmaroli]

@letrastudio Thank you reporting the new find and for the test-repo. I shall look into this discovery in the coming days.

[ashmaroli]

@letrastudio I've looked into the issue and have come to the conclusion that the bug is indeed within jekyll-paginate-v2. It can be easily confirmed by looking at your test site's --debug output. All pages generated by the plugin has the same relative_path attribute (equaling .html).

Therefore, the fix for this is to simply allot a unique relative_path string for the each instance of Jekyll::PaginateV2::Generator::PaginationPage. I suggest you file an issue at the plugin's repository.