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

fix: Don't delegate public methods overridden by a private in the decorator #849

Merged
merged 4 commits into from
Mar 18, 2019
Merged

fix: Don't delegate public methods overridden by a private in the decorator #849

merged 4 commits into from
Mar 18, 2019

Conversation

brunohkbx
Copy link
Contributor

Public methods defined on the object was being delegated to the decorator even though it was overridden by a private method.

Testing

  1. Create a simple PORO with a public method
class Job
  def location
    Struct.new(:id) { }
  end
end
  1. Create a decorator and override the public method with a private
class JobDecorator < Draper::Decorator
  delegate_all

  private

  def location
    LocationDecorator.new(object.location)
  end
end
  1. It should raise an exception when calling .location from the decorator
[9] pry(main)> JobDecorator.decorate(Job.new).location
NoMethodError: private method `location' called for #<JobDecorator:0x0000559f113ba118>

References

Public methods defined on the object was being delegated to the decorator even though it was
overridden by a private method.
Fix #771
sobrinho
sobrinho previously approved these changes Mar 18, 2019
Copy link
Collaborator

@codebycliff codebycliff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left to nitpicky suggestions on style / formatting, but other than that, it's good to go. I wonder how breaking of a change this should be considered.

spec/draper/decorator_spec.rb Outdated Show resolved Hide resolved
spec/draper/decorator_spec.rb Outdated Show resolved Hide resolved
Co-Authored-By: brunohkbx <bruno.castro@codeminer42.com>
Co-Authored-By: brunohkbx <bruno.castro@codeminer42.com>
Copy link
Collaborator

@codebycliff codebycliff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your contributions lately, they have been greatly appreciated!

@codebycliff codebycliff merged commit d44c86f into drapergem:master Mar 18, 2019
@brunohkbx brunohkbx deleted the fix/automatic_delegation branch March 18, 2019 16:36
@brunohkbx
Copy link
Contributor Author

@codebycliff I was thinking the same. Could it be a breaking change? 🤔
What if we raise a deprecated warning instead of raising an error?

@codebycliff
Copy link
Collaborator

@brunohkbx I think that's definitely an option. I'm also considering just bumping the major version when I release this. It's been a while since a major version has been released, so I don't think it would be that big of a deal.

springerigor added a commit to springerigor/draper that referenced this pull request Feb 10, 2020
drapergem#849 introduced a valid fix to prevent calling object's public method when a decorator overwrites it through a private one. There is one caveat, though. `private_methods` methods has `all` parameter set to `true` by default (https://ruby-doc.org/core-2.7.0/Object.html#method-i-private_methods)`. As a result all private methods are returned, instead of only the ones defined on the object.

This commit fixes the above issue by setting the `all` parameter to `false`.
@springerigor
Copy link
Contributor

@brunohkbx & @codebycliff I need your attention #875. Thanks in advance 🙂

springerigor added a commit to springerigor/draper that referenced this pull request Feb 10, 2020
drapergem#849 introduced a valid fix to prevent calling object's public method when a decorator overwrites it through a private one. There is one caveat, though. `private_methods` methods has `all` parameter set to `true` by default (https://ruby-doc.org/core-2.7.0/Object.html#method-i-private_methods)`. As a result all private methods are returned, instead of only the ones defined on the object.

This commit fixes the above issue by setting the `all` parameter to `false`.
codebycliff pushed a commit that referenced this pull request Feb 11, 2020
#849 introduced a valid fix to prevent calling object's public method when a decorator overwrites it through a private one. There is one caveat, though. `private_methods` methods has `all` parameter set to `true` by default (https://ruby-doc.org/core-2.7.0/Object.html#method-i-private_methods)`. As a result all private methods are returned, instead of only the ones defined on the object.

This commit fixes the above issue by setting the `all` parameter to `false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants