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

Automatic strong parameters detection #154

Conversation

Maxim-Filimonov
Copy link
Contributor

Should prevent users from creating an extra initializer when used in Rails 4 or with StrongParameters gem

@dblock
Copy link
Member

dblock commented May 2, 2014

Isn't this something that should be done via Railtie? I also wonder what happens when you have Rails first in Gemfile before hashie?

@Maxim-Filimonov
Copy link
Contributor Author

It's possible to do the same with Railtie but I have no clue how do you test that, do you ? Not sure how order of gems will affect it

end
context 'when strong parameters are used' do
before :each do
module ActionController
Copy link
Member

Choose a reason for hiding this comment

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

From a purist pov, I would try to implement this by defining, then un-defining the module. This way the test doesn't modify global state. Another, maybe better way would be to include actionpack in Gemfile with a require: false and load that. This way the test wouldn't rely on an implementation detail.

@Maxim-Filimonov
Copy link
Contributor Author

@dblock is this better? I'm starting to think the same approach could be used with rails/railtie.
Something like:

# gemspec
gem.add_development_dependency 'actionpack'
# mash.rb
require 'mash/railtie' if defined? ::Rails::Rightly
# mash_spec.rb
it 'does not respond to permitted?' do
     expect(subject).to be_respond_to(:permitted?)
     require 'rails/railtie'
     load 'hashie/mash.rb'      
     expect( subject ).not_to be_respond_to(:permitted?)
     expect { subject.method(:permitted?) }.to raise_error(NameError)
     expect { subject.permitted? }.to raise_error(ArgumentError)
end

Thoughts?
Strong parameters gem already has dependency on railties(https://github.com/rails/strong_parameters/blob/master/strong_parameters.gemspec) so even if someone is using it outside of Rails application it should still work( like Grape mounted inside Sinatra for example. )

@Maxim-Filimonov
Copy link
Contributor Author

Ah just realised the whole module doesn't actually work due to the way modules got included in ruby :\
See example:

irb> class A
  def call_me
    'A'
  end
end

irb> class B
  def call_me
    'B'
  end
end


irb> module Overrider
  def call_me
    "Override"
  end
end


irb> class A
  include Overrider
end
irb> A.included_modules
=> [#<Class:0x007fc8ed1cb228>::Overrider, PP::ObjectMixin, Kernel]
irb> A.new.call_me
=> "A"
irb> sneaky = Class.new(B) do
   include Overrider
 end
> sneaky.new.call_me
=> "Override""

So based on the example in your tests you have used the sneaky method and that's why the test passed. However, if you actually try to include the module into Mash in an initialiser it's not gonna work.
:\
As far as I understood, sneaky method works because ruby injects module defined methods above methods defined in a class but below methods defined in a base class. Class.new actually creates a subclass of class you pass to it in which case module defined methods takes precedence over inherited ones.
There is ruby 2.0 Module#prepend which will make it work. However, i'm not sure do you want to break compatibility with 1.9.3?

@dblock
Copy link
Member

dblock commented May 5, 2014

Lets ignore the test for a second.

Isn't Railtie the "official" way of doing this?

require 'rails'

class Hashie::Extensions::Mash::ActiveModel::Railtie < Rails::Railtie
  initializer "mash" do
    Mash.send :include, Hashie::Extensions::Mash::ActiveModel
  end
end

Or something like that? Or are you saying it's not going to work either way for the reason above (order of includes)? In which case we should just use alias_method.

It would be very helpful to get a quick Rails project together that has a spec that we can test against HEAD of Hashie.

@Maxim-Filimonov
Copy link
Contributor Author

@dblock I have tried alias method before and it works. It would be a good idea to have a dummy app. But it's hard to combine them together inside this gem. What are you thoughts on moving this logic from hashie to hashie-rails ?

@dblock
Copy link
Member

dblock commented May 6, 2014

I would be down with hashie-rails.

@Maxim-Filimonov
Copy link
Contributor Author

@dblock Apologise for delay. RL got in the way :
Just finished first rough version of the plugin -> https://github.com/Maxim-Filimonov/hashie_rails
Can't release it until there is a version of hashie which I can use on rubygems. Latest version on rubygems works with Rails without patch so I need the version which needs the patch.

@dblock
Copy link
Member

dblock commented Jun 1, 2014

Looks great. Can you please PR a README change to Hashie that talks about Hashie+Rails integration via HashieRails? There's also things to change in UPGRADING and possibly CHANGELOG. A README on the HashieRails side will also be useful. We can close this issue then and I will cut a 3.0 release. Thx.

@Maxim-Filimonov
Copy link
Contributor Author

See #162
Not sure what do you want me to update in changelog.

@dblock
Copy link
Member

dblock commented Jun 2, 2014

Committed 41e6c67 for README.

@Maxim-Filimonov Should we get rid of Hashie::Mash::ActiveModel?

@Maxim-Filimonov
Copy link
Contributor Author

Just did see -> #163
Closing this one

@Maxim-Filimonov Maxim-Filimonov deleted the automatic_strong_parameters_detection branch June 3, 2014 04:32
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.

None yet

2 participants