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

Remove ActionView::Helpers::Tags::Base#value Monkey Patch #1485

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Aesthetikx
Copy link

This library provides an override of ActionView::Helpers::Tags::Base#value (the method that is used to determine the current value of a form field built from a template object) that allows calling private methods. The implementation is otherwise the same as the one provide by ActionView.

I ran into some bizarre edge cases with this implementation when using OpenStruct, for example just having the ransack gem in memory and then using a f.text_field :test with an OpenStruct as model will display a confusing wrong number of arguments, given 0 expected 2..3 message because #test is a private method on Kernel. Regardless of how I got here, I don't think this monkey patch is necessary or desired anymore.

Without looking in detail, the reason I believe this patch was created in the first place is because here we define a select tag with :p, and like :test, :p is a private method on Kernel, and creating a select tag in this way will cause the form builder to see if there is a :p method to fetch an existing value.

In the second commit in this PR, we can simply default selected: nil in options such that the form builder does not try to figure out what the existing value is to see if one of the values is selected.

I don't use Ransack directly much (outside of ActiveAdmin) so I can't say if that will be a breaking change, but the tests do pass after that change. Open to feedback!

@Aesthetikx
Copy link
Author

Adding another comment for clarity, to explain another way:

Ransack is giving form elements short names like p for predicates, c for conditions, or g for groupings. Presumably these end up coming into a Rails controller as params that Ransack can understand. These aren't intended to provide default values to those fields, as e.g. it is unlikely that an object has a #c method. That said, ActionView form builder will still see if an object responds to that method (the method matching name of the field) to get a current value, or selected dropdown item. In general nothing happens here because there is no #c or #g, but there is in fact a #p method (the one that prints things, e.g. p 'Hello World!'. This method also happens to be a private method, so that you can use it in an IRB or Main, but you can't say Object.new.p 'hello'. However, you can Object.new.send(:p), which coincidentally does nothing and returns nil when no arguments are passed. In the documentation here you can see

The value returned from calling method on the instance object will be selected. If calling method returns nil, no selection is made without including :prompt or :include_blank in the options hash.

and hence because #p returns nil, nothing 'bad' happens -- although you do have to allow form builder to call private methods to make the problem 'go away' and presumably hence the reason for the monkey patch in the first place.

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

Successfully merging this pull request may close these issues.

2 participants