Add always permitted parameters as a configurable option. #15933

Merged
merged 2 commits into from Jun 27, 2014

Conversation

Projects
None yet
5 participants
@rafael
Contributor

rafael commented Jun 27, 2014

  • This commit adds back the always_permitted_parameters configuration option to strong parameters.
  • The initial pull requests where this feature was added are the following:
Add always_permitted_parameters as an option.
* 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:
  - #12682
  - rails/strong_parameters#174
@garysweaver

This comment has been minimized.

Show comment
Hide comment
@garysweaver

garysweaver Jun 27, 2014

Contributor

Nice! 💃

Contributor

garysweaver commented Jun 27, 2014

Nice! 💃

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 27, 2014

Member

✂️

+ # `config.always_permitted_parameters = %w( controller action format )`
+
+ cattr_accessor :always_permitted_parameters
+

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 27, 2014

Member

✂️

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2014

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.

Member

rafaelfranca commented Jun 27, 2014

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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2014

Member

BTW, thank you for the patch.

Member

rafaelfranca commented Jun 27, 2014

BTW, thank you for the patch.

@rafael

This comment has been minimized.

Show comment
Hide comment
@rafael

rafael Jun 27, 2014

Contributor

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

Contributor

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 )`

This comment has been minimized.

@robin850

robin850 Jun 27, 2014

Member

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 ? :-)

@robin850

robin850 Jun 27, 2014

Member

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 ? :-)

This comment has been minimized.

@rafael

rafael Jun 27, 2014

Contributor

Sure, will do that!

@rafael

rafael Jun 27, 2014

Contributor

Sure, will do that!

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2014

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.

Member

rafaelfranca commented Jun 27, 2014

  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

This comment has been minimized.

Show comment
Hide comment
@rafael

rafael Jun 27, 2014

Contributor

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

Contributor

rafael commented Jun 27, 2014

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

Improvements per code review.
* General style fixes.
* Add changes to configuration guide.
* Add missing tests.
@rafael

This comment has been minimized.

Show comment
Hide comment
@rafael

rafael Jun 27, 2014

Contributor

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

Contributor

rafael commented Jun 27, 2014

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

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2014

Member

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

Member

rafaelfranca commented Jun 27, 2014

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

@rafaelfranca rafaelfranca merged commit 58399e1 into rails:master Jun 27, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

rafaelfranca added a commit that referenced this pull request Jun 27, 2014

Merge pull request #15933 from rafael/master
Add always permitted parameters as a configurable option.

[Rafael Mendonça França + Gary S. Weaver]
@asafbrukarz

This comment has been minimized.

Show comment
Hide comment
@asafbrukarz

asafbrukarz Jan 27, 2016

Should this be config.action_controller.always_permitted_parameters = %w( controller action format ) ?

Should this be config.action_controller.always_permitted_parameters = %w( controller action format ) ?

kamipo added a commit to kamipo/rails that referenced this pull request Feb 14, 2016

Remove `const_missing` which fallback to deprecated `EVER_UNPERMITTED…
…_PARAMS`

`NEVER_UNPERMITTED_PARAMS` is deprecated in Rails 4.2. See #15933.

kamipo added a commit to kamipo/rails that referenced this pull request Feb 14, 2016

Remove `const_missing` which fallback to deprecated `NEVER_UNPERMITTE…
…D_PARAMS`

`NEVER_UNPERMITTED_PARAMS` is deprecated in Rails 4.2. See #15933.

kamipo added a commit to kamipo/rails that referenced this pull request Feb 14, 2016

Remove `const_missing` which fallback to deprecated `NEVER_UNPERMITTE…
…D_PARAMS`

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