Skip to content

Conversation

@rafael
Copy link
Contributor

@rafael rafael commented Jun 27, 2014

* This commit adds back the always_permitted_parameters
  configuration option to strong paramaters.
* The initial pull requests where this feature was added
  are the following:
  - rails#12682
  - rails/strong_parameters#174
@garysweaver
Copy link
Contributor

Nice! 💃

Copy link
Member

Choose a reason for hiding this comment

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

✂️

@rafaelfranca
Copy link
Member

We need to update the configuring guide and also make sure if you change this configuration adding a parameter it will not raise if the parameter is present.

@rafaelfranca
Copy link
Member

BTW, thank you for the patch.

@rafael
Copy link
Contributor Author

rafael commented Jun 27, 2014

@rafaelfranca no problem. Quick questions:

  1. ✂️ means remove that line?
  2. Could you point me to the direction in the configuration guide where this should be added?
  3. Do you mean adding a test for the case when adding the configuration the exception is not raise?

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can do something like:

# config. For instance:
#
#    config.always_permitted_parameters = %w( controller action format )

Thus we will have syntax highlighting and this reads a bit better. What do you think ? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do that!

@rafaelfranca
Copy link
Member

  1. means remove that line?

yes.

  1. Could you point me to the direction in the configuration guide where this should be added?

In the Action Pack section of https://github.com/rails/rails/blob/master/guides/source/configuring.md

  1. Do you mean adding a test for the case when adding the configuration the exception is not raise?

Right now we only have test to make sure always_permitted_parameters can be set but no test to make sure it is used. If you revert the changes at https://github.com/rails/rails/pull/15933/files#diff-72f51b082f5c2a99c328f5f5bbbe8ec6R380 your tests will still pass. We should add a test where we change the values of always_permitted_parameters and when we get parameters with the values there nothing is raised.

@rafael
Copy link
Contributor Author

rafael commented Jun 27, 2014

@rafaelfranca: Awesome! Thanks for the feedback. I will add those changes to the PR.

* General style fixes.
* Add changes to configuration guide.
* Add missing tests.
@rafael
Copy link
Contributor Author

rafael commented Jun 27, 2014

@rafaelfranca What do you think? I added the changes and also one more test for the deprecation warning.

@rafaelfranca
Copy link
Member

Very good ❤️. I'll add a CHANGELOG entry and merge it. Thanks.

@rafaelfranca rafaelfranca merged commit 58399e1 into rails:master Jun 27, 2014
rafaelfranca added a commit that referenced this pull request Jun 27, 2014
Add always permitted parameters as a configurable option.

[Rafael Mendonça França + Gary S. Weaver]
kamipo added a commit to kamipo/rails that referenced this pull request Feb 14, 2016
…D_PARAMS`

`NEVER_UNPERMITTED_PARAMS` is deprecated in Rails 4.2. See rails#15933.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants