Upgrade from Liquid 4 to Liquid 5

[parkr]

This is a 🙋 feature or enhancement.

Summary

Upgrade from Liquid v4.x to Liquid v5.x. Keeping up-to-date on Liquid allows our users to benefit from Liquid's continual improvements from Shopify.

Context

Prior art and issues/discussions: #8662 #8760 #8535 #8615

[parkr]

expected: "<p>Dog, Goat, Horse, Iguana</p>"
got: "<p>Goat</p>"

Failing Scenarios:
cucumber features/embed_filters.feature:111 # Scenario: Filter posts by given property and value

This is what I feared. Using empty or blank in the where template (e.g. {{ ... | where: "property", empty }}) gives our filter just an empty string ("") as the value to filter by – in Liquid v4, it returned the MethodLiteral related to empty so we could more easily discern this special keyword.

[parkr]

I filed Shopify/liquid#1566 with the Liquid team to see if this was intended behavior. If we can work through a solution for this one case (empty and blank) then we're good to make the upgrade according to our test suite!

[parkr]

The following diff seems to have fixed the cucumber failure for me:

    def compare_property_vs_target(property, target)
    case target
    when NilClass
      return true if property.nil?
-      when Liquid::Condition::MethodLiteral # `empty` or `blank`
-        target = target.to_s
+      when ""
      return true if property == target || Array(property).join == target
    else
      target = target.to_s

[ashmaroli]

@parkr I'm not comfortable with restricting to just Liquid 5 in a minor release. There are bound to be cases that may not be covered by our test-suite (referring to third-party plugins).

It would be better if we could still support Liquid 4 and let the end-user decide if they need Liquid 5.
However, in order to support both Liquid 4 and 5, we may have to implement an adapter of sorts..

[parkr]

@ashmaroli Do you know how many plugins are using Liquid's API rather than ours? Can we anticipate what will fail other than perhaps the gemspec mismatch? This change doesn't really modify anything on Liquid's API except for our use of their internals around empty/blank so it feels safe and plug-in owners can easily bump (and test) an upgrade to Liquid 5. At least requiring it makes the change more obvious other than some potentially vague user report who doesn't know they upgraded to Liquid 5.

The downside to allowing both is that we end up punting our decision to the end user. This further increases the amount of work they have to do to make their site work.

Would you be open to a deprecation period of a few MINOR releases? We allow both for now with a big call out in the release notes that at v4.5 or v5.0 of Jekyll, we'll remove Liquid v4 support and give plug-in authors and site owners time to upgrade.

We could search through GitHub and rubygems to try to figure out plugins that depend on liquid explicitly. My hunch is that this upgrade won't be noticed by any plugin authors since they're not using low-level APIs.

[ashmaroli]

Would you be open to a deprecation period of a few MINOR releases? We allow both for now with a big call out in the release notes that at v4.5 or v5.0 of Jekyll, we'll remove Liquid v4 support and give plug-in authors and site owners time to upgrade.

Yes. My plan is similar..
Allow using both Liquid v4 and v5 (relaxed version constraint in gemspec). Which translates to new sites and existing sites invoking bundle update automatically loads Liquid v5. If something breaks, the user reports the issue to us and we recommend them to lock their Gemfile to Liquid v4 for resolution.
Eventually, as we edge towards Jekyll v5, we deprecate Liquid v4 softly (via a post_install_message and in release post(s)) and drop its support entirely by locking the jekyll-5.0.0 gemspec to Liquid v5 along with making necessary changes based on issues regarding Liquid v5 reported till then.

I'm only disputing the move that robs users to fall back to Liquid v4 (if necessary).. not the move to allow hopping onto the Liquid v5 wagon.

[ashmaroli]

@parkr How about a change in perspective..: What if the existing tests were a poor abstraction as well..? Jekyll shouldn't be concerned about internal intricacies of Liquid. Semantically, Jekyll should just be concerned about whether the sample Liquid construct {{ my_object | where: property, empty }} results in the same answer for both Liquid v4 and v5..

[parkr]

The semantics have slightly changed since this change. Before, we knew if it was either "empty" or "blank" so we matched Array and Hash and Nil objects against this operator. In Liquid 5, it now passes just "" so we can't keep the same functionality we had before. We would have to do one of the following:

1. Match "" against Array, Hash, and Nil objects. This breaks checking for "this object is strictly an empty string", but preserves most of the current behavior.
2. Remove checking for Array, Hash, and Nil objects, preserving strict empty string equality but breaking sites which want to check for an empty object.
3. Change the where signature to include another argument which allows the user to specify strict or loose equals, with a default to loose to preserve as much of Liquid 4's behavior as possible.

[ashmaroli]

@parkr what if we monkey-patch Liquid to restore Liquid v4 behavior and remove the patch in Jekyll 5?

I understand that monkey-patching is frowned upon and Shopify Team considers the old behavior a leaky abstraction, but from my understanding of the current Liquid codebase, that route allows us to bridge the two versions with minimal issues.

[ashmaroli]

@parkr what if we monkey-patch Liquid to restore Liquid v4 behavior and remove the patch in Jekyll 5?

Proof-of-concept at #9036