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

prepare_locals: Use Object#send instead of calling directly. #136

Open
wants to merge 1 commit into
base: 1-4-stable
Choose a base branch
from

Conversation

agarian-alex
Copy link

Object#respond_to? will return true for protected methods. However
calling a protected method directly results in an exception. Using
Object#send allows a protected method (like current_user) to be
called.

Object#respond_to? will return true for protected methods. However
calling a protected method directly results in an exception. Using
Object#send allows a protected method (like current_user) to be
called.
@farnoy
Copy link
Contributor

farnoy commented Feb 16, 2014

Do you need this fix for your case? #current_user is a public method and a helper most of the time.

I don't remember anyone complaining about this, is this an issue for you?

@agarian-alex
Copy link
Author

Yep. We define #current_user as a protected method because it's not an action (which would be public), and it's shared amongst different controller classes (so it's not private).

@farnoy
Copy link
Contributor

farnoy commented Feb 16, 2014

Perhaps you could use this to address the problem:
http://api.rubyonrails.org/classes/ActionController/HideActions/ClassMethods.html#method-i-hide_action
.

I don't think anyone makes #current_user protected/private, does it work as
a helper in views that way?

On 16 February 2014 20:24, Alex Zepeda notifications@github.com wrote:

Yep. We define #current_user as a protected method because it's not an
action (which would be public), and it's shared amongst different
controller classes (so it's not private).

Reply to this email directly or view it on GitHubhttps://github.com//pull/136#issuecomment-35207686
.

@agarian-alex
Copy link
Author

In our case, #current_user is a protected method on the controller superclass (ApplicationController). There's a separate method in one of the helpers to accomplish the same thing in the view context. What I've submitted only affects how a function on the controller object is executed. The way it was written:

controller.respond_to?(:current_user) ? controller.current_user : nil

Object#respond_to? will return true for a protected method, but controller.current_user will throw an exception For a private method, Object#respond_to? would return false and the ternary operator would grab nil.

@farnoy
Copy link
Contributor

farnoy commented Feb 16, 2014

I understand why it's not working for you, but this helper has
traditionally been a public helper in the controller, this convention is as
popular as rails itself. Could you explain what motivated you to put it in
the helper?

On 16 February 2014 20:46, Alex Zepeda notifications@github.com wrote:

In our case, #current_user is a protected method on the controller
superclass (ApplicationController). There's a separate method in one of the
helpers to accomplish the same thing in the view context. What I've
submitted only affects how a function on the controller object is
executed. The way it was written:

controller.respond_to?(:current_user) ? controller.current_user : nil

Object#respond_to? will return true for a protected method, but
controller.current_user will throw an exception For a private method,
Object#respond_to? would return false and the ternary operator would grab
nil.

Reply to this email directly or view it on GitHubhttps://github.com//pull/136#issuecomment-35208402
.

@pokonski
Copy link
Member

If you need a method in controllers (and you clearly do) don't make it as protected.

@agarian-alex
Copy link
Author

Regardless of my motivation (the authentication system was not of my design), respond_to? :foo ? object.foo : nil is unsafe. That's what the pull request is designed to address.

@pokonski
Copy link
Member

using send looks like a hack, which I don't want. You can pass true as the second argument to respond_to? to ignore protected methods.

Please also add a spec for this case.

@agarian-alex
Copy link
Author

The second argument to respond_to? is for private, not protected methods.

http://ruby-doc.org/core-1.9.3/Object.html#method-i-respond_to-3F

respond_to?(symbol, include_private=false) → true or false

The default for include_private means that private, but not protected methods will be excluded. This will / has changed in ruby 2.x.

http://tenderlovemaking.com/2012/09/07/protected-methods-and-ruby-2-0.html

As for making the method public, everything I've come across indicates that non-action methods on a controller should be marked protected or private.

ex:

http://stackoverflow.com/questions/4495078/protected-and-private-methods-in-rails

http://tatey.com/2012/11/23/testing-private-and-protected-methods-in-rails-controllers-without-being-awkward/

http://stackoverflow.com/questions/896556/do-you-ever-use-protected-visibility-in-rails

@STRd6
Copy link

STRd6 commented Jun 19, 2014

I was just bitten by this as well.

@STRd6
Copy link

STRd6 commented Jun 19, 2014

Actually my problem may be similar but different.

I have a current_user helper method on my application controller that works everywhere in my application.

When I call current_user from within a public_activity view it returns nil instead.

What I expect to happen is that views within app/views/public_activity/... will use the same current_user helper method as elsewhere in my application.

@pokonski pokonski added the bug label Aug 19, 2014
@sungwoncho
Copy link

Why are the views within app/views/public_activity/ using different current_user helper? Should they not use the one defined in your ApplicationController because they are within the same app?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants