Integrate strong_parameters in Rails 4#7251
Conversation
There was a problem hiding this comment.
Is not better call this exception ForbiddenAttrbibutesError or something like this?
There was a problem hiding this comment.
Agree, sounds better, we should also change it in the gem to keep both synced
There was a problem hiding this comment.
Done
Note to myself: This change is pending on gem
|
I think we will need a hidden option to disable strong_parameters that should be set by the attr_accessible plugin. Also guides and docs should be updated/written. |
|
Thanks @guilleiguaran for the PR and @rafaelfranca for the review. I want to take a look at this as well, but I will be able to do it just later this week! |
|
Agree with @rafaelfranca about disabling strong_parameters, should be a hidden option to do that to allow attr_accessible plugin to work without strong_parameters. |
|
What if people attr_accessible or attr_protected in 4.0 without the plugin?. Couldn't we leave those methods and raise an error saying "Add the plugin to the Gemfile or move to strong_parameters. @josevalim what do you think? |
|
Good point. 👍 for this Rafael Mendonça França
|
|
Besides from all that is looking great ❤️ ❤️ ❤️ |
|
@spastorino @josevalim @rafaelfranca thanks for you feedback guys, I'm starting to change the code with your suggestions |
|
❤️ |
|
Could you explain how does the |
|
@kirs: there is documentation in the strong parameters gem. |
There was a problem hiding this comment.
Would it be better if ActionController::Parameters wrapped a Hash rather than inherited from ActiveSupport::HashWithIndifferentAccess? For example CookieJar used to inherit from Hash but was changed in 0ca69ca. @tenderlove, what do you think?
There was a problem hiding this comment.
@rafaelfranca can you work on this, I'm no having much luck with this and @spastorino won't have time to check it on the next weeks
There was a problem hiding this comment.
Even though my opinion is to do so, I think the final decision was to not change.
There was a problem hiding this comment.
I remember some discussion about this one but I didn't read yet. I'll try to read now.
There was a problem hiding this comment.
@rafaelfranca it'll be great you post a link :)
|
I've extracted ActiveModel::MassAssignmentSecurity and ActiveRecord/AC::ParamsWrapper integration to the protected_attributes plugin: https://github.com/rails/protected_attributes Right now everything is working fine except integration with AR deprecated finders (related tests), can someone help me fixing it? |
|
The plugin is ready and all tests passing, this is ready now /cc @dhh |
There was a problem hiding this comment.
Since we're dealing with parameters that emulate a hash, it seems to be more appropriate to use KeyError, not? (I know it's a subclass, just thought it might be a better error to inherit from)
>> [].fetch 1
IndexError: index 1 outside of array bounds: 0...0
>> {}.fetch :a
KeyError: key not found: :aThere was a problem hiding this comment.
Agree 100% on this, I will change it
This will be moved out to protected_attributes gem
…Hash instead of monkey patch permitted? method in regular hashes
…psulate permissible params
…eference from abstract_unit
…if attributes list isn't given
…rameters protection
… plugin or new protection model
…ed than IndexError
Integrate strong_parameters in Rails 4
|
Strong Prameters does not solve the problem of checking parameters in Rails. The problem is model-based validation. Require or not require, permit or not permit, validate and how to validate - these are questions of the Form, not of a model and not of a controller. Model-based validation must be like SQL constraints - simple, basic and unconditional. It's about solidity of data, it's about consistency, not roles or permissions. And One form can require Another problem with StrongParameters - they ara introduce duplication of responsibilities: and What is difference between these approaches? ResolutionChecking parameters (validation, requirements etc) must be extracted to the dedicated class. |
|
Basically nobody on the rails team but me is obsessive enough to read everything in comments on commits. The official place to talk about stuff like this is the rails-core list. |
|
I have many forms pointing to the same controllers. Desktop and mobile web views, APIs, etc. This verification also cannot be trusted to the view as you don't want to tamper with it. I wouldn't bother with the validates :user, :presence check if you feel like you're duplicating yourself. On Sep 24, 2012, at 10:25 AM, Danil Pismenny wrote:
David Heinemeier Hansson |
|
I'm talking not about view's. In real life Forms contains two parts. The list of required and permitted parameters is linked to forms, not to Example.
In every form specific list of fields, specific list of required and In controller we must have something like this: RegistrationFormVlidator.new.validate params Every FormValidator requires and permits and also validates parameters in Yes we can have default UserFormValidator or specific role of current_user The key question is validation, not just requires and permits. And On Mon, Sep 24, 2012 at 8:14 PM, David Heinemeier Hansson <
äÁÎÉÌ ðÉÓØÍÅÎÎÙÊ danil@investcafe.ru birg@investcafe.ru | www.investcafe.ru |
|
That abstraction does not offer any compelling benefit to me. But feel free to wrap it up like that in your application. You can wrap these form validators around the require/permit setup. On Sep 24, 2012, at 11:56 AM, Danil Pismenny wrote:
David Heinemeier Hansson |
This integrate strong_parameters plugin in Rails core and remove attr_accessible/attr_protected.