Fix can use options argument at DatabaseAuthenticatable#update_with_password

[ihatov08]

No description provided.

[tegon]

Hello @ihatov08, thanks for the pull request.

Would you mind to explain what issue this is solving?

[ihatov08]

@tegon
This method can specify the options variable as the second argument, but it fixed that an error occurs even if it is specified.

before

[2] pry(main)> User.first.update_with_password({current_password: 'password'}, password: 'new_password')
ArgumentError: wrong number of arguments (given 2, expected 1)
from /usr/local/bundle/gems/activerecord-5.2.1/lib/active_record/persistence.rb:423:in `update'

after

[2] pry(main)> User.first.update_with_password({current_password: 'password'}, password: 'new_password') 
=> true

[feliperenan]

Could you add a test for this change? We could add it closest to this one https://github.com/plataformatec/devise/blob/master/test/models/database_authenticatable_test.rb#L155

[tegon]

@ihatov08 Sorry for taking so long to reply to you.
I've taken a look at your response, and it seems like we don't need the options argument at all. I say this because in your changes you added the following line:

params.merge(options)

Which for the code example you sent

User.first.update_with_password({current_password: 'password'}, password: 'new_password')

It will became a single hash:

params.merge(options)
=> {current_password: 'password', password: 'new_password'}

So, in the end, it's the same as if we pass one argument only:

User.first.update_with_password(current_password: 'password', password: 'new_password')

I thought this was weird so I took a look at the code history, and the options argument was added to support a feature present in Rails 3.1, where it was possible to pass a as option to specify a mass assignment scope.
That option was then deprecated in Rails 4 in favor of strong parameters.

Since we're supporting only Rails 4 and up, I think we can remove the options argument completely. But because #update_password is public API and people could be using in other ways, I think we should do the following:
1 - Change this PR to include a deprecation warning when the options atttribute is present, to warn people that this argument will be removed in v5;
2 - Create another PR that removes the attribute completely and merge into the v5 branch.

WDYT?

[ihatov08]

@tegon
Thank you for your reply.
The example I showed was not very good.

User.first.update_with_password({current_password: 'password'}, password: 'new_password') 

I think it is better to remove the options argument as well as you.

I think the second plan is good.

2 - Create another PR that removes the attribute completely and merge into the v5 branch.

[tegon]

@ihatov08 Great! Do you want to change this PR to add the deprecation warning?
If not, let me know so we can do it over here. I believe it would be something like the following:

def update_with_password(params, **options)
  if options.present?
        ActiveSupport::Deprecation.warn <<-DEPRECATION.strip_heredoc
          [Devise] The second argument of `DatabaseAuthenticatable#update_with_password`
          (`options`) is deprecated and it will be removed in the next major version. 
          It was added to support a feature deprecated in Rails 4, so you can safely remove it 
          from your code.
        DEPRECATION
  end
  ...
end

[ihatov08]

@tegon OK!
I will change this PR to add the deprication warning.

[ihatov08]

@tegon Done!

[tegon]

@ihatov08 Thanks!

I just noticed that there's another method doing a similar thing below: #update_without_password. Would you mind to add the deprecation warning there too?
Also, it's better to revert the old changes:

-def update_with_password(params, **options)
+def update_with_password(params, *options)

-update(params.merge(options)) 
+update(params, *options)

-assign_attributes(params.merge(options))
+assign_attributes(params, *options)

[ihatov08]

@tegon Fixed!

[tegon]

Thanks!