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

Deprecate ActionController::Parameters #558

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Conversation

jaredbeck
Copy link
Collaborator

Fixes #512

What do you think? Should we have special code to deal with ActionController::Parameters?

We could fix #512 by adding even more special code, or we could simplify the whole thing by requiring a plain Hash. But, that's a breaking change.

Fixes #512

What do you think? Should we have special code to deal with
ActionController::Parameters?

We could fix #512 by adding even more special code, or we could
simplify the whole thing by requiring a plain Hash. But, that's
a breaking change.
@binarylogic
Copy link
Owner

Looks good to me 👍

@brendon
Copy link

brendon commented Nov 21, 2017

It seems that a simple .to_h on authlogic's side would have solved this without any dependency issues. Am I correct? I suppose we could argue that that is still special treatment for AC::Params but all we're really saying is that we want the object passed to us to be convertible to a hash.

@jaredbeck
Copy link
Collaborator Author

It seems that a simple .to_h on authlogic's side would have solved this without any dependency issues. Am I correct? I suppose we could argue that that is still special treatment for AC::Params but all we're really saying is that we want the object passed to us to be convertible to a hash.

Good point Brendon. In the spirit of ruby, I'd like to keep the existing .to_h. However, I'd also like people to get an error if they forget to permit. So, that's where the idea of "only plain hashes" came from. Raising a TypeError with a friendly message would make such an omission very clear. Type-checking may not be the spirit of ruby, but I think in this case it will help people.

I'm open to any other suggestions that make it very clear to people when they forget to permit.

@brendon
Copy link

brendon commented Nov 21, 2017

Perhaps instead of type checking, you could raise an error if the username and password keys aren't present in the hash, with a suggestion that they check that they permitted those values? Or maybe even just raise if the hash is empty since that's the most likely scenario.

@jaredbeck
Copy link
Collaborator Author

Perhaps instead of type checking, you could raise an error if the username and password keys aren't present in the hash, with a suggestion that they check that they permitted those values? Or maybe even just raise if the hash is empty since that's the most likely scenario.

So, authlogic would continue to call .to_h, and then raise an error unless the hash has login_field and password_field as keys. (We should not check for the :remember_me key, as many people omit that from their login form) We would need to be able to differentiate a missing hash key from a blank one (for, e.g. the "Login cannot be blank" error). Of course, this is possible in ruby using Hash#key?. I like it. Can you prepare a PR please?

This was referenced Nov 24, 2017
@jaredbeck jaredbeck added this to the 3.7.0 milestone Dec 6, 2017
@jaredbeck
Copy link
Collaborator Author

jaredbeck commented Feb 8, 2018

This deprecation warning was released as 3.7.0.

#580 makes the actual breaking change, was merged into master and will be released in 4.0.0.

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

Successfully merging this pull request may close these issues.

None yet

4 participants