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

Add always permitted parameters as a configurable option. #15933

Merged
merged 2 commits into from
Jun 27, 2014

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! 💃

# to change these is to specify `always_permitted_parameters` in your
# config, e.g.
# `config.always_permitted_parameters = %w( controller action format )`

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!

# because they are added by Rails and should be of no concern. One way
# to change these is to specify `always_permitted_parameters` in your
# config, e.g.
# `config.always_permitted_parameters = %w( controller action format )`
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.

None yet

5 participants