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

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

Conversation

soartec-lab
Copy link
Contributor

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:

0c2cab7

@rafaelfranca rafaelfranca force-pushed the task/removed-older-deprecation-warning branch from df57366 to e524a3d Compare June 9, 2023 23:20
@rafaelfranca rafaelfranca merged commit 9be24c0 into heartcombo:main Jun 9, 2023
@ghiculescu
Copy link
Contributor

Could we get a new release for this?

@carlosantoniodasilva
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

ghiculescu commented Sep 12, 2023

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 #5583 that is causing it.

@soartec-lab
Copy link
Contributor Author

@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:

rails/rails#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.

#5600

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

@carlosantoniodasilva
Copy link
Member

@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
Copy link
Contributor Author

@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?

#5600

@carlosantoniodasilva
Copy link
Member

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

@soartec-lab
Copy link
Contributor Author

@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
Copy link
Contributor Author

@carlosantoniodasilva

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

#5628

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.

4 participants