Drop#[]: only use public_send for keys in the content_methods array #4388

Merged
merged 2 commits into from Jan 22, 2016

Conversation

Projects
None yet
3 participants
@parkr
Member

parkr commented Jan 22, 2016

Fixes #4383.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 22, 2016

Contributor

If respond_to? responds to what you consider private methods then they aren't exactly private, they can still get to those methods anyways. In order for Liquid to ignore them and not respond to them in the liquid you need to actually flag it as private.

If respond_to? responds to what you consider private methods then they aren't exactly private, they can still get to those methods anyways. In order for Liquid to ignore them and not respond to them in the liquid you need to actually flag it as private.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 22, 2016

Contributor

What I mean is:

class MyDrop < Liquid::Drop
  private
  def my_method
    "hello"
  end
end

{{ drop.my_method }} will not work, the method is private, Liquid will ignore it.

Contributor

envygeeks replied Jan 22, 2016

What I mean is:

class MyDrop < Liquid::Drop
  private
  def my_method
    "hello"
  end
end

{{ drop.my_method }} will not work, the method is private, Liquid will ignore it.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 22, 2016

Contributor

To add, respond_to? only checks public methods by default, so if respond_to? method works, without respond_to? method, true then the method is public rather than private, the user can still get at it with {{ drop.method }}, you can tell Liquid to ignore things with the private flag.

Contributor

envygeeks replied Jan 22, 2016

To add, respond_to? only checks public methods by default, so if respond_to? method works, without respond_to? method, true then the method is public rather than private, the user can still get at it with {{ drop.method }}, you can tell Liquid to ignore things with the private flag.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 22, 2016

Member

@envygeeks It's not about public or private. Liquid seems to only use [] anyway, so if you're accessing page.class, you're accessing DocumentDrop#[] and therefore obviously asking for content, not for DocumentDrop#class.

Member

parkr commented Jan 22, 2016

@envygeeks It's not about public or private. Liquid seems to only use [] anyway, so if you're accessing page.class, you're accessing DocumentDrop#[] and therefore obviously asking for content, not for DocumentDrop#class.

@parkr parkr added the fix label Jan 22, 2016

@parkr parkr added this to the 3.1 milestone Jan 22, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 22, 2016

Contributor

@parkr Liquid will respond to and use any method that is public on a drop that is why we do this in Jekyll-Assets: https://github.com/jekyll/jekyll-assets/blob/master/lib/jekyll/assets/liquid/drop.rb#L45 but you can see from the specs: https://github.com/jekyll/jekyll-assets/blob/master/spec/lib/jekyll/assets/liquid/drop_spec.rb#L35 you can see that Liquid does in-fact respond to and use any method that is considered public on the Drop class.

Contributor

envygeeks commented Jan 22, 2016

@parkr Liquid will respond to and use any method that is public on a drop that is why we do this in Jekyll-Assets: https://github.com/jekyll/jekyll-assets/blob/master/lib/jekyll/assets/liquid/drop.rb#L45 but you can see from the specs: https://github.com/jekyll/jekyll-assets/blob/master/spec/lib/jekyll/assets/liquid/drop_spec.rb#L35 you can see that Liquid does in-fact respond to and use any method that is considered public on the Drop class.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 22, 2016

Member

@envygeeks I updated it to use Liquid::Drop.invokable?.

Member

parkr commented Jan 22, 2016

@envygeeks I updated it to use Liquid::Drop.invokable?.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 22, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Jan 22, 2016

@jekyllbot: merge +bug

jekyllbot added a commit that referenced this pull request Jan 22, 2016

@jekyllbot jekyllbot merged commit 512c7fd into master Jan 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the fix-page.class-access branch Jan 22, 2016

jekyllbot added a commit that referenced this pull request Jan 22, 2016

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.