Fix Liquid filter typo in breadcrumb component (strip instead of trim)

[Zarthus]

We are running into an issue upgrading from 0.7 to 0.8, suggesting that trim is not a method.

1.963   Liquid Exception: Liquid error (/usr/gem/gems/just-the-docs-0.8.0/_includes/components/breadcrumbs.html line 59): undefined filter trim included in /_layouts/default.html
1.968 /usr/gem/gems/liquid-4.0.4/lib/liquid/strainer.rb:58:in `invoke': Liquid error (/usr/gem/gems/just-the-docs-0.8.0/_includes/components/breadcrumbs.html line 59): undefined filter trim included  (Liquid::UndefinedFilter)

Looking at the liquid docs, we're probably looking for strip and not trim

https://shopify.github.io/liquid/filters/strip/

https://github.com/Shopify/liquid/blob/main/History.md#300--2014-11-12, I don't see a trim so it may have been an extension? This does work normally on just-the-docs itself running from scratch, so it might also be a bug in our application (or combination of different versions in plugins).

[mattxwang]

Thank you for submitting a PR @Zarthus!

This does work normally on just-the-docs itself running from scratch

Oh interesting, this difference really surprises me. We ran 0.8.0 on a few different client sites just to see if there's anything that our internal testing has missed, and it's completely possible that something still slipped through the cracks (e.g. maybe the unless branch is never entered in any of our test cases).

If I could ask: do you have a public repo that's able to demonstrate this problem? Would help us double-check and verify what the problem is! If not, a toy repository that can reproduce the issue would also be really helpful.

Separately, I agree that trim doesn't seem to be a native Liquid filter or part of the list provided by Jekyll. Let me ponder this a bit more and make sure that I'm not missing any context!

(will also tag in @pdmosses)

[Zarthus]

Hey Matt! Unfortunately our repository is private (and I confirm it works for this repository specifically, I just cannot figure out where the trim method comes from here, and why we don't have it. My gut says some sort of version incompatibility.)

FWIW; it's being built on an enterprise account on GitHub (the SaaS version; so github.com) privately hosted on our internal company network (it's a docs page).

Something perhaps interesting is that we have the following setting enabled, and disabling strict_filters makes our build green:

   
    liquid:
      error_mode: strict
      strict_filters: true   # <== setting this to false makes the build pass
   

(https://jekyllrb.com/docs/configuration/liquid/)

[rmg]

strict_filters was a great find. It turns missing filters into no-ops: https://github.com/Shopify/liquid/blob/4a4fe3c72a91cc8a26869ed2dad8e3fcac33565a/lib/liquid/strainer_template.rb#L50-L60

    def invoke(method, *args)
      if self.class.invokable?(method)
        send(method, *args)
      elsif @context.strict_filters
        raise Liquid::UndefinedFilter, "undefined filter #{method}"
      else
        args.first
      end
    rescue ::ArgumentError => e
      raise Liquid::ArgumentError, e.message, e.backtrace
    end

[pdmosses]

Something perhaps interesting is that we have the following setting enabled, and disabling strict_filters makes our build green:

   
    liquid:
      error_mode: strict
      strict_filters: true   # <== setting this to false makes the build pass
   

(https://jekyllrb.com/docs/configuration/liquid/)

@Zarthus Many thanks for reporting and diagnosing this bug! The filter trim should indeed be replaced by strip.¹

Apparently JtD has never set liquid options, and the Jekyll default is strict_filters: nil. This PR could include adding the options that you are using to _config.yml, to confirm that there are no other (used) non-existing filters.

However, JtD can't set strict_variables: true: it stops the build when executing many valid tests, such as the following (where collections is an optional site setting):

{%- if site.just_the_docs.collections %}

Footnotes

1. Liquid isn't the only language I use, and trim in another language has e same effect as strip in Liquid. ↩

[Zarthus]

Thank you @pdmosses! I'm happy to include a commit into a singular PR (this one), if that is maintainers preference. For now, I've opened a separate one so we can talk and review both issues in isolation (#1435)

As this is a legitimate bugfix I assume it might be more straightforward to not mix concerns (and have as an easy to release .1 patch version), and review both topics separately. But again - happy to go whichever way is preferred.

[mattxwang]

Thanks everyone for your quick responses! I feel silly and also did not know about strict_filters, this is a really useful config feature that we should definitely enable! Thanks @Zarthus and @rmg for chiming in.

As this is a legitimate bugfix I assume it might be more straightforward to not mix concerns (and have as an easy to release .1 patch version), and review both topics separately.

I agree with this! (I will also note that changes in our _config.yml do not propagate to our users, so #1435 would also not be a breaking change)

[mattxwang]

Quickly tested this on our own site + just-the-docs tests, and this LGTM! I'll also let @pdmosses chime in with an official review before merging. Thanks again @Zarthus for your contribution!

---

Notably, I have not found a site that outputs different content, whitespace or not, with this change. I assume one such site exists - happy to investigate this later if we're interested.

[pdmosses]

LGTM.

@mattxwang presumably you'll add a CHANGELOG entry before merging.

[pdmosses]

Notably, I have not found a site that outputs different content, whitespace or not, with this change. I assume one such site exists - happy to investigate this later if we're interested.

I think the filter was added to make the result independent of whether the site nav HTML rendered by JtD has any whitespace between its elements. Currently, most Liquid tags and filters in the nav code remove adjacent whitespace; so does HTML compression.

[mattxwang]

Huh. CI failure is an issue with htmlproofer, not this PR. Going to rerun that check.

[Zarthus]

Appreciate it, thank you all, folks!

[mattxwang]

Okay, I've just cut a release to RubyGems (and GPR, though I suspect few people use it). @Zarthus you should be able to pull and build 0.8.1 shortly (if it's not available already). Could you give this a spin and verify that it also resolves your issue?

Thank you to @Zarthus, @rmg, and @pdmosses for your quick communication on this PR - couldn't have done it without y'all!

[Zarthus]

Happy to do so! Will report back tomorrow as it's midnight here at this point :)

(though I'll be very confident it'll work, as when debugging the issue and test the PR I used our internal docs site and just replaced 'trim' with 'strip' in the gem's installed source to be confident that was problem and also the only issue 😉)