heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
23.85k stars 5.53k forks source link

Removed deprecations warning output for `Devise::Models::Authenticatable::BLACKLIST_FOR_SERIALIZATION` #5598

Closed soartec-lab closed 1 year ago

soartec-lab commented 1 year ago

This deprecation warning worked for us, but it has served its purpose. It was added over 2 years ago now, isn't that long enough to trigger a deprecation warning?

https://github.com/heartcombo/devise/blob/main/CHANGELOG.md#480---2021-04-29

Now that I've given it enough time, I've removed this as I think it's noise for many people who don't see Devise::Models::Authenticatable::BLACKLIST_FOR_SERIALIZATION.

However, Devise::DeprecatedConstantAccessor, which was added in the same commit, is not referenced anywhere else, but has not been removed as it may be used in the future. Please let me know so I can delete it if necessary.

These source codes were added in the PR below:

https://github.com/heartcombo/devise/commit/0c2cab7c946e0796c673a36aebba7c0352e5fec8

ghiculescu commented 10 months ago

Could we get a new release for this?

carlosantoniodasilva commented 10 months ago

I can look into it, but is it causing any issue I'm not aware of? It is technically a breaking change which means a patch bump might not be enough for this, so I've been holding off until deciding what to do next.

ghiculescu commented 10 months ago

It means you get a deprecation warning when you boot an app running Rails 7.1 (alpha). So not a problem, just a little nuisance.

carlosantoniodasilva commented 10 months ago

@ghiculescu that's interesting, my understanding is that the deprecation shouldn't trigger unless the constant was accessed directly? I don't see it on a v7.0 app, I'll have to test with main / 7.1 alpha.

ghiculescu commented 10 months ago

This is the warning:

DEPRECATION WARNING: DeprecatedConstantAccessor.deprecate_constant without a deprecator is deprecated (called from <module:Authenticatable> at /home/ruby/.rvm/gems/ruby-2.7.7/gems/devise-4.9.2/lib/devise/models/authenticatable.rb:65)

So my bad, it's the absence of https://github.com/heartcombo/devise/pull/5583 that is causing it.

soartec-lab commented 10 months ago

@ghiculescu @carlosantoniodasilva

Hi.

This happens by calling ActiveSupport::Deprecation.instance with the code below:

https://github.com/soartec-lab/devise/blob/master/lib/devise/rails/deprecated_constant_accessor.rb#L29

This is because from rails 7.1, ActiveSupport::Deprecation.instance has been deprecated and changed to issue a warning. Please check below:

https://github.com/rails/rails/pull/47354

The solution is to change lib/devise/rails/deprecated_constant_accessor.rb to the latest source code. However, this file is a process to maintain backward compatibility, and backward compatibility support for older rails versions will be removed in the following PR.

https://github.com/heartcombo/devise/pull/5600

I think it will probably be removed by the next major update.

carlosantoniodasilva commented 10 months ago

@ghiculescu gotcha, makes sense. That may or may not be easier to handle (in terms of a new release that's backwards compatible)

@soartec-lab thanks.

soartec-lab commented 10 months ago

@carlosantoniodasilva

OK, If there is no prospect of responding to the PR below, I will create a PR to update this file to the latest version. What do you think?

https://github.com/heartcombo/devise/pull/5600

carlosantoniodasilva commented 10 months ago

@soartec-lab if all we need is to update ActiveSupport::Deprecation.instance to Devise.deprecator there, based on https://github.com/heartcombo/devise/pull/5583, then yeah that sounds good to me, sounds like it was just missed there.

soartec-lab commented 10 months ago

@carlosantoniodasilva

I have the same opinion. It looks like there was an update missing, so I'll create a new PR and fix it.

soartec-lab commented 10 months ago

@carlosantoniodasilva

I corrected this issue with the PR below. Could you review?

https://github.com/heartcombo/devise/pull/5628