Honor public/private in ActionView::Helpers::Tags::Base#value #14180

Closed
wants to merge 2 commits into
from

2 participants

@PragTob
  • use public_send instead of send to avoid calling private methods in form helpers

I do not know if this feature is wanted, but it was simple enough to implement so I just went ahead and did it.

I discovered this together with a bug in MRI/simple_delegator.
E.g. When you have an object using SimpleDelegator that has an open method which you try to call with send it ends up calling an open method somewhere in Ruby that wants 1..3 arguments in Ruby 2.0. Too bad if you have an attribute named open.
( see this gist for a sample)

That does not happen with public_send. However that is a bug somewhere else. So writing a test for it seemed too hard so I wrote a simple honor public/private test. Any suggestions for a better testing of this?

I do think that in general it'd be good to honor public/private in this case. What do you think?

Cheers,
Tobi

@PragTob PragTob Honor public/private in ActionView::Helpers::Tags::Base#value
* use public_send instead of send to avoid calling private
  methods in form helpers
0daabea
@dmathieu dmathieu and 1 other commented on an outdated diff Feb 24, 2014
actionview/test/template/form_helper_test.rb
@@ -124,6 +124,12 @@ def test_tags_base_child_without_render_method
assert_raise(NotImplementedError) { FooTag.new.render }
end
+ def test_tags_base_value_honors_public_private
+ test_object = Class.new { private def my_method ; end }.new
+ tag = ActionView::Helpers::Tags::Base.new 'test_object', :my_method, nil
+ assert_raise(NoMethodError) { tag.send :value, test_object }
@dmathieu
dmathieu added a line comment Feb 24, 2014

Why the send here? Couldn't you just do tag.value(test_object) ?

@PragTob
PragTob added a line comment Feb 24, 2014

value is a private method unfortunately... :-/ So using send to work around it...

edit: thanks for your quick feedback! =D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@PragTob

Weirdly the build fails if GEM=ap,am,amo,as,av for 1.9.3 and 2.0 - does anyone know why that might be?

FormHelperTest#test_tags_base_value_honors_public_private:
TypeError: nil is not a symbol
test/template/form_helper_test.rb:128:in `private'
test/template/form_helper_test.rb:128:in `block in test_tags_base_value_honors_public_private'
test/template/form_helper_test.rb:128:in `initialize'
test/template/form_helper_test.rb:128:in `new'
test/template/form_helper_test.rb:128:in `test_tags_base_value_honors_public_private'

I think it might be the inline private/method definition of the test class, but don't know what that has to do with the gems as it works in the other gems with all the other ruby versions..

@carlosantoniodasilva carlosantoniodasilva commented on the diff Feb 25, 2014
actionview/test/template/form_helper_test.rb
@@ -124,6 +124,15 @@ def test_tags_base_child_without_render_method
assert_raise(NotImplementedError) { FooTag.new.render }
end
+ def test_tags_base_value_honors_public_private
+ test_object = Class.new do
+ def my_method ; end
+ private :my_method
+ end.new
+ tag = ActionView::Helpers::Tags::Base.new 'test_object', :my_method, nil
+ assert_raise(NoMethodError) { tag.send :value, test_object }
+ end
@carlosantoniodasilva
Ruby on Rails member

I'm not sure that using send here would be a good way to test this, can you think of another way to test it other than bypassing encapsulation?

@PragTob
PragTob added a line comment Feb 25, 2014

yeah I am unsure about that as well. The right way to test this would be exercising a behavior that ends up calling value(object) and then raises the error/exercises a different behavior.

However the downside is, that it will most likely be a much bigger/more complicated test set up. Dunno if it's worth the cost so didn't wanna start with it (as I was unsure if this change is even welcome).

Happy to try out a more behavior driven approach for testing.

@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Oct 7, 2014

How about trying to render a text input?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe

Did this ever get followed up @carlosantoniodasilva / @PragTob ?

@PragTob

Nope, sorry it didn't. I ran into this during my Master's thesis and then was lacking the time to dive deeper into this for writing the more realistic test case.

Tomorrow I'll be on my way to RubyConf Portugal so no time in the next week. Maybe I can get to it after that.

As I'm not very familiar with the ActionView would appreciate some guidance.

Thanks for pinging!

Tobi

@rafaelfranca rafaelfranca added a commit that referenced this pull request Oct 31, 2014
@rafaelfranca rafaelfranca Merge branch 'tags-public-send'
Fixes #14180
Fixes #17461
67698f3
@rafaelfranca rafaelfranca added a commit that referenced this pull request Oct 31, 2014
@rafaelfranca rafaelfranca Merge branch 'tags-public-send'
Fixes #14180
Fixes #17461
820e294
@rafaelfranca rafaelfranca added a commit that closed this pull request Oct 31, 2014
@rafaelfranca rafaelfranca Merge branch 'tags-public-send'
Fixes #14180
Fixes #17461
c1a118a
@ttosch ttosch pushed a commit that referenced this pull request Jan 19, 2015
@rafaelfranca rafaelfranca Merge branch 'tags-public-send'
Fixes #14180
Fixes #17461
cabde42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment