Skip to content
This repository has been archived by the owner on Aug 17, 2017. It is now read-only.

'require' should be named something like 'scope' or 'narrow' to not violate the principle of least surprise #182

Open
deevis opened this issue Dec 6, 2013 · 12 comments

Comments

@deevis
Copy link

deevis commented Dec 6, 2013

I believe the method 'require' does more than it should. It is counter-intuitive that it also FILTER the results of the statement. I expected it to do nothing more than raise an exception if the argument was not present in the target Hash.

From the API doc:
Ensures that a parameter is present. If it's present, returns the parameter at the given key, otherwise raises an ActionController::ParameterMissing error.

So, part 1 ( "Ensures that a parameter is present") makes perfect sense.

Doing anything beyond that is SURPRISING.

The fact that it ALSO does part 2, and filters my resulting map, was definitely SURPRISING. I know - RTFM. But this is rails and we don't want to violate the principle of least surprise.

I suggest that if 'require' should also FILTER the target Hash, that it be named something that gives a hint to the likelihood this may occur, like 'scope' or 'narrow'.

@scrapcupcake
Copy link

Yes please. This section of the rails guides in particular totally bombs when trying to use it, because if you try it in a real app, in the person_params method, the variable params is nil, and nil.require ends up actually being Kernel.require... which is a totally different beast and throws the VERY confusing error of TypeError (can't convert Symbol into String)

http://edgeguides.rubyonrails.org/action_controller_overview.html#strong-parameters

For the love of cake, please change the name of this method. I'm going to go make a pull request on the guides to fix that one section.

@scrapcupcake
Copy link

My issue was occurring because I'd managed to create a local params variable that got set to nil.

It's still way too easy, and confusing, to end up with an object other than the expected params hash, and accidentally calling Kernel.require, leading to very surprising results.

@fotinakis
Copy link

Wow, totally agree here. I just spent way too long debugging this because I didn't expect that .require(:foo) would return the value, since so many docs directly contradict this by showing chaining like params.require(:post).permit(:title) (on the front page of https://github.com/rails/strong_parameters ).

@chancancode
Copy link
Member

Probably a tangent, but the example above is actually correct. params.require(:post) returns params[:post] (when present) – which is a strong parameters hash. Then you call permit on it to whitelist params[:post][:title].

I don't know about this for sure, but I think the rationale to return the value when present is precisely to make this possible. Otherwise permitting nested hashes (nested attributes) would be a lot clumsier.

@fotinakis
Copy link

That makes more sense, the reason it was confusing to me was because I tried to do something like params.require(:id).permit(:other_param) thinking that those methods behaved the same way and all applied to the base params hash.

I agree with this issue in general, I think clearer naming could help here.

@nickhoffman
Copy link

It's definitely confusing. Another thing that's confusing is how to require nested parameters. Say you want to ensure that params[:foo][:bar] exists. params.require :foo gets you half-way, but how do you do the rest? The only solution that I've found is fugly, surprising, and not idiomatic:

params.require :foo
params[:foo].require :bar

I expected to be able to chain these calls, like params.require(:foo).require(:bar), or provide a hash along the lines of params.require :foo => [:bar].

@atrauzzi
Copy link

atrauzzi commented May 1, 2014

Totally agree, require should only throw an exception if the parameter is missing, but otherwise do nothing. As an addition, it should also be able to accept an array of parameters to check for:

params.require :username, :password, :password_confirmation

Documentation is otherwise not clear on the usage and this might be as a result of being "astonishing".

@johnnyshields
Copy link

👍 It is quite confusing how .require returns the key (like [] method) while .permit behaves like .slice. It would be nice to have .slice-like and []-like variants of both require and permit (a total of 4 methods.). For the sake of argument maybe something like:

params.must_have(:foo)   # .require without scoping
params.may_have(:foo)    # current .permit
params.scope!(:foo)      # current .require
params.scope(:foo)       # .permit with scoping

@thomasfedb
Copy link
Contributor

The changes suggested here would constitute a massive breaking change to the vast majority of Rails applications which are using strong parameters correctly without issue.

If you feel that this is a particularly important change to have in your applications it would not be particularly difficult to implement as a gem which replaced the existing behaviour of #require.

@fxn this can likely be closed.

@johnnyshields
Copy link

Could we propose other method names here which would make sense, and deprecate the old ones? I think the key request here is to distinguish the roles of "requiring/permitting" versus "shifting scope"

I think #require is a bad name for a method anyway since it's a reserved word in Ruby, granted it's valid to use but I think it adds to the confusion here.

@thomasfedb
Copy link
Contributor

@johnnyshields You'd have to convince @dhh or other Rails core members. In any case, you had better continue the discussion over at rails/rails as this is just a compatability gem.

@johnnyshields
Copy link

OK moved to rails/rails#19963

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants