sgruhier / foundation_rails_helper

Rails Helper for Zurb Fondation framework
MIT License
152 stars 84 forks source link

alias_method_chain is deprecated #131

Closed dsandstrom closed 8 years ago

dsandstrom commented 8 years ago

DEPRECATION WARNING: alias_method_chain is deprecated. Please, use Module#prepend instead. From module, you can access the original method using super. (called from module:FormHelper at /Users/dsandstrom/Projects/einstein_video5/vendor/bundle/ruby/2.3.0/bundler/gems/foundation_rails_helper-646bd8477bf6/lib/foundation_rails_helper/action_view_extension.rb:19) DEPRECATION WARNING: alias_method_chain is deprecated. Please, use Module#prepend instead. From module, you can access the original method using super. (called from module:FormHelper at /Users/dsandstrom/Projects/einstein_video5/vendor/bundle/ruby/2.3.0/bundler/gems/foundation_rails_helper-646bd8477bf6/lib/foundation_rails_helper/action_view_extension.rb:20)

dgmstuart commented 8 years ago

I've done some investigation into this:

What is alias_method_chain used for?

The forms functionality of Foundation Rails Helper works by overriding the definitions of the two main form helpers in ActionView:

It does this by monkeypatching those function definitions (in ActionView::Helpers::FormHelper). Since it only wants to modify these functions, not replace them, it needs to call the original method definition (with super). alias_method_chain is a (now deprecated) technique for doing this which essentially works like this:

Say you have a method foo which you want to modify in a way that you're calling bar:

  1. Define a method called foo_with_bar, which calls super to get the original definition of foo that you're overriding.
  2. Declare alias_method_chain :foo, :bar

What this does is something like:

  1. Rename foo to foo_without_bar
  2. Rename foo_with_bar to foo

This effectively overrides the previous method definition with our new extended one.

What does our monkeypatching do?

This all occurs in lib/foundation_rails_helper/action_view_extension.rb

In form_for

  1. Use our form builder instead of the default Rails form builder
  2. Initialise a hash of html options
  3. Defaults the auto_labels option to true

2 is legacy: it was introduced in 3713cd4e923e68234f3d63e3313f3ed1c7c596b0 when a class ('nice') was added onto all forms. It could have been removed in #60 when that class was removed. It's no longer required.

3 This option is specific to this gem

In fields_for

  1. Use our form builder instead of the default Rails form builder
  2. If an attached_labels option was passed, add it as an html option
  3. Default the auto_labels option to true

2 Was added in #47 and it's not clear why: attached_labels has never been referenced elsewhere in the gem code, doesn't seem to be anything to do with Rails, and doesn't seem to be part of the html spec either.

My guess is that this was a hold over from a previous attempt to solve this by the submitter of #47 and was committed by accident.

What can we do about it?

Now that alias_method_chain is deprecated we have two options: find a better way to do the same monkeypatching, or redesign how foundation rails helper is used in an application.

The benefits of the current approach is that it's pretty seamless for developers using the gem: you just add it to the gemfile and it works.

However, it may no longer be possible to continue in this way without alias_method_chain and rails 5/foundation 6 would be a good opportunity to make a breaking change by moving to a design where it's more explicit how foundation rails helper is included in an application.

Monkeypatching

This stackoverflow answer provides a pretty good breakdown of the different approaches: http://stackoverflow.com/a/4471202

The error message suggests using module_prepend. It looks like we should be able to do this:

module FoundationRailsHelper
  module FormHelper
    def form_for(record, options = {}, &block)
      options[:builder] ||= FoundationRailsHelper::FormBuilder
      options[:auto_labels] = true unless options.has_key? :auto_labels
      super(record, options, &block)
    end

    def fields_for(record_name, record_object = nil, options = {}, &block)
      options[:builder] ||= FoundationRailsHelper::FormBuilder
      options[:auto_labels] = true unless options.has_key? :auto_labels
      super(record_name, record_object, options, &block)
    end
  end
end

module ActionView
  module Helpers
    module FormHelper
      prepend FoundationRailsHelper::FormHelper
    end
  end
end

...but for some reason this doesn't work - or at least, most of the tests fail because the default ActionView form builder is getting used.

Possible reasons:

  1. The above code has a mistake in it
  2. There's something in the way that we're mocking out Rails which means our test suite doesn't handle this properly
  3. There's something in the way that ActionView gets loaded (i.e. rails being quirky) which means that this approach doesn't work.
  4. module prepend only works with classes, not other modules. This isn't how I understand it works, but all the examples I've seen are of modules overriding instance methods in classes rather than in other modules (like in this case where we're overriding methods on the ActionView::Helpers::FormHelper class).

Redesign

There are two things we need to achieve:

Override the default form builder

This can be done in a couple of ways:

Firstly by adding the following line in application.rb:

config.action_view.default_form_builder = FoundationRailsHelper::FormBuilder

Alternatively we could follow the approach of SimpleForm and define our own form helpers e.g. foundation_form_for, foundation_fields_for.

Default auto_labels to true

This key is used in one place (in lib/foundation_rails_helper/form_builder.rb), and it looks like we could set the default value there instead.

dgmstuart commented 8 years ago

I propose that we go with the redesign and require users to explicitly configure their applications to use the form builder.

This has minimal impact and wouldn't require any new work. Also has the benefit that the different features of the gem could be used independently.

dgmstuart commented 8 years ago

I'm working on a PR to do the preliminary cleanup of lib/foundation_rails_helper/action_view_extension.rb

dgmstuart commented 8 years ago

I found another way to include the form builder! 😄

ActionView::Base.default_form_builder = FoundationRailsHelper::FormBuilder
dgmstuart commented 8 years ago

The above PR actually fixes this issue.

dsandstrom commented 8 years ago

Excellent write up, thanks for digging into this. I will review the options/PRs this week, however it seems you have found an elegant solution for this particular issue.