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

Fix deprecation warning in ParameterSanitizer#permit #5551

Closed
wants to merge 1 commit into from

Conversation

j-sieg
Copy link

@j-sieg j-sieg commented Feb 5, 2023

Hello first time here

I'm using Devise 4.8.1 with Rails and I'm upgrading my Rails version to 7.0.4
When I ran the tests on my app, I get these deprecation warnings:

app/models/user/parameter_sanitizer.rb:24: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/devise-4.8.1/lib/devise/parameter_sanitizer.rb:110: warning: The called method `permit' is defined here

There were plenty of them, so I decided to bundle open devise.
I edited the file, ran the tests on my app, and the deprecation warnings about the ParameterSanitizer were gone.
I put what I wrote on this PR.

@carlosantoniodasilva
Copy link
Member

@j-sieg I don't see those warnings when running Devise tests on Ruby 2.7, can you show the code in your custom parameter sanitizer: app/models/user/parameter_sanitizer.rb?

Maybe that's implemented in a way that's delegating as non-kwargs to Devise or something. Thanks!

@j-sieg
Copy link
Author

j-sieg commented Mar 2, 2023

@carlosantoniodasilva here's the code in app/models/user/parameter_sanitizer.rb

class User::ParameterSanitizer < Devise::ParameterSanitizer
  def initialize(params, resource = nil)
    # resource's class, resource's name, params
    super(User, :user, params)

    sign_up_defaults = [:name, :mobile_number]
    account_update_defaults = [
      :name,
      :date_of_birth,
      :mobile_number,
      :avatar_url
    ]

    # Omitted code
    # I use `resource` here to push some fields depending on biz logic

    permit(:sign_up, keys: sign_up_defaults)
    permit(:account_update, keys: account_update_defaults)
  end
end

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Mar 2, 2023

@j-sieg thanks. I don't see anything wrong at a glance, I thought maybe the calling code could be passing a hash like this:

permit_options = { keys: [:foo, :bar] }

...

# permit_options is a hash in this sample
permit(:sign_up, permit_options)

If that was the case, then the calling code would have to be changed to actual kwargs:

permit(:sign_up, **permit_options)

But it doesn't seem to be the case... what's interesting is that this code from Devise hasn't changed since 2015 so I'm not sure I want to switch it back to a hash of options when kwargs are much clearer there. 🤔

@nashby
Copy link
Collaborator

nashby commented Nov 7, 2024

@j-sieg thanks for the PR but I can't see any warning as well unless you're using it the way @carlosantoniodasilva mentioned so I'm closing it for now. Feel free to ping us if it's not the case and you these warning anyway.

@nashby nashby closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants