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

Scope parameters according to Rails version #964

Closed
wants to merge 1 commit into from

Conversation

mygulamali
Copy link
Contributor

Rails 5 has deprecated the keywords arguments for #permit. This patch scopes the request params according to the version of Rails.

Fixes #867. Supercedes #917 with passing tests.

Thank you to @freesteph for helpin' out!

Rails 5 has deprecated the keywords arguments for #permit. This patch
scopes the request params according to the ActionPack version.

Fixes #867.  Supercedes #917 with passing tests.

Thank you to @freesteph for helpin' out!
@mygulamali mygulamali force-pushed the fix-keyword-params-for-rails-5 branch from 35fb146 to c64468e Compare September 23, 2016 12:01
@@ -250,7 +250,10 @@ def matches?(controller)
parameters_double_registry.register

Doublespeak.with_doubles_activated do
context.__send__(verb, action, request_params)
scoped = ::ActionPack::VERSION::MAJOR >= 5
Copy link
Collaborator

@mcmire mcmire Sep 23, 2016

Choose a reason for hiding this comment

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

Thanks for this. We tend to place logic that's dependent on Rails versions in RailsShim, that way everything is in one place. What if you extracted context.__send__ to a method, say, make_controller_request(context, verb, action, options = {})?

Additionally, please note that I won't be able to merge this in until I have Travis running the gem against Rails 5. But I'll at least add this to #960.

@mygulamali
Copy link
Contributor Author

Closing this PR as it has been superseded by #989, which takes the review comments into account, and is also based on the rails5 branch.

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.

2 participants