Alias Drop#invoke_drop to Drop#[]

[benbalter]

@DirtyF discovered an interesting behavior via jekyll/github-metadata#108 (comment), which I had attempted to resolve (unsuccessfully) via jekyll/github-metadata#110.

In the particular instance, Jekyll::Utils.deep_merge_hashes(drop, hash) was returning a drop as expected, but without the hash values overriding the drop values. After some digging, I discovered the merge logic was correct (and calling [key]=value, which gets stored in @mutations), but that when a drop method was called, it was returning the drop's value, not the mutation.

If a drop is mutable, #[] prefers the user-set value over the default (a Jekyll construct).

#invoke_drop(key) which Liquid uses, however, doesn't understand the concept of mutations as it's using Liquid::Drop's native implementation. This leads to scenarios where drop.key, drop.invoke_drop('key') and drop["key"] return different values.

Normally, Drop#[] is aliased to Drop#invoke_drop ensuring the values remain in sync. Since they both implement the same core logic public_send key if self.class.invokable? key, by aliasing #invoke_drop to #[], both will respect mutability and return the same value regardless of drop.invoke_drop('key') or drop['key] is called (and allowing Jekyll::Utils.deep_merge_hashes to perform the expected operation).

Given the following test case:

require 'jekyll'

class TestDrop < Jekyll::Drops::Drop
  mutable true

  def fallback_data
    @fallback_data ||= {}
  end

  def value
    "value from drop method"
  end

  alias_method :invoke_drop, :[]
end

obj = { "value" => "value from drop object" }
drop = TestDrop.new(obj)

puts "\ndrop.to_h after construction:"
puts drop.to_h

hash = { "value" => "value from hash" }
drop = drop.merge(hash)

puts "\ndrop.to_h after merge:"
puts drop.to_h

puts "\ndrop.invoke_drop('value') after merge:"
puts drop.invoke_drop('value')

puts "\ndrop['value'] after merge:"
puts drop['value']

Output before change:

drop.to_h after construction:
{"value"=>"value from drop method"}

drop.to_h after merge:
{"value"=>"value from hash"}

drop.invoke_drop('value') after merge:
value from drop method

drop['value'] after merge:
value from hash

Output after change:

drop.to_h after construction:
{"value"=>"value from drop method"}

drop.to_h after merge:
{"value"=>"value from hash"}

drop.invoke_drop('value') after merge:
value from hash

drop['value'] after merge:
value from hash

The one gotcha, is with this implementation is we lose the liquid_method_missing logic, which we could encorporate into #[] if we want, but isn't strictly necessary in our implementation since we never raise for missing methods.

[DirtyF]

@benbalter Great explanation, I'm a big fan of how you document issues to allow others to learn. 👏

[parkr]

Love it, and totally down to include this change in 3.6 and possibly backport. In order to ensure we don't lose this behaviour in the future, could you add a test to test/test_utils.rb?

[parkr]

Whoops – test/test_drop.rb would be a more appropriate place to add a test for this.

[benbalter]

could you add a test

Implemented via a948998.

[benbalter]

The base drop class (in #[], #keys and #key?) assumes fallback data to exist and return a hash. Stubbed out the default value so that child classes don't each have to implement it if they don't have fallback data (including our test fixture).

[parkr]

I intentionally left this method out so all drops had to implement this – it was part of the design of Jekyll Drops, that some fallback data had to be provided by the developer of a drop. I'm not sure I like that going away. :/

[benbalter]

it was part of the design of Jekyll Drops, that some fallback data had to be provided by the developer of a drop

Admittedly out of scope, so glad to remove from this PR. Curious if that rationale still holds true when Jekyll internally often implements it as an empty hash?

[benbalter]

Removed via 847ed7f

[benbalter]

Fixing this one problem revealed another blocker to mutability taking precedence over methods.

Take a look at 3776c44 (failing test) and c998c59 (fix).

In short, key? called mutable not mutable? meaning calls to fetch or Liquid's own use of key? at render time would call class.mutable without any arguments. Doing so would (A) always return false, but more importantly (B) set @is_mutable to false, meaning fetching a mutable key would zero out all mutability (even if accessed using another method like #[].

This now PR simply changes self.class.mutable to self.class.mutable? which is the getter, not the setter.

[parkr]

Sounds like we should backport this to 3.5.x once it ships.

👍 to merge once CI is passing.

[benbalter]

@jekyllbot: merge +minor.