Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable drops should fallback to their own methods when a mutation isn't present #112

Merged
merged 3 commits into from Sep 7, 2017

Conversation

benbalter
Copy link
Contributor

This is a side effect of monkey patching jekyll/jekyll#6338 into #110.

In jekyll/jekyll#6338 the problem was that Drop#key? needed to call self.class.mutable? not self.class.mutable. Rather than wait on the upstream fix, I monkey patched a modified version of key? with that single character change.

As it turns out, there's more wrong with key? in Jekyll core. If you read through the logic of:

https://github.com/jekyll/jekyll/blob/8b47fb1f7a85d518a21f5fcfd3b20df18c60bf7a/lib/jekyll/drops/drop.rb#L101-L112

You'll see that if a Drop is mutable, and the user hasn't overridden the requested key, the Drop will respond that it doesn't know anything about the key (even if it has a non-mutated native method that's the intended value).

In this case, if a user, for example, calls site.github.url, it'd work fine if they overrode the value, but Liquid, which calls Drop#key? would assume the drop didn't respond to the key since the Drop only looked at mutations.

This PR adds tests for the temporarily monkey-patched methods, and corrects the logic of Drop#key? to look for a corresponding key in the mutable data, in its own methods, and then in the fallback data, responding true if a key exists in any (meaning the Drop could respond to the requested key).

/cc @parkr

@benbalter benbalter merged commit f18d6dd into master Sep 7, 2017
@benbalter benbalter deleted the fallback-to-mutable-methods branch September 7, 2017 22:47
@jekyll jekyll locked and limited conversation to collaborators Apr 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants