heartcombo / devise

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

release 4.9.1 broke devise-security gem #5580

Closed francescob closed 1 year ago

francescob commented 1 year ago

in latest release of devise the method activerecord51? in Devise module was removed/renamed, thus breeaking other gems that used that method, like devise-security

carlosantoniodasilva commented 1 year ago

This was part of a refactor I implemented, but the method was explicitly set to :nodoc: and shouldn't be relied upon by others. If possible, I'd recommend them updating the code to use their own version checks.

If that doesn't seem feasible in the short-term, I can consider re-adding the method with a temporary deprecation, but again, external code shouldn't rely on that code that's internal to Devise.

francescob commented 1 year ago

thanks for the super fast reply, i've just posted this here in case anyone encountered the same error. Opening an issue on devise-security as well

carlosantoniodasilva commented 1 year ago

For reference: https://github.com/devise-security/devise-security/issues/413

I'll close it for now, but if we hear more people having trouble, or any other intergration not working, we can reconsider. Thanks!

alec-c4 commented 1 year ago

@carlosantoniodasilva another gem is affected too https://github.com/michaelbanfield/devise-pwned_password/issues/40

carlosantoniodasilva commented 1 year ago

Seriously =/, I'm going to release a new patch version bringing that method back to avoid that much breakage, but will add a deprecation warning.

Thanks for reporting.

rocket-turtle commented 1 year ago

@carlosantoniodasilva Maybe you could reconsider adding this methode back and drop Support for Rails <= 5.0 in a Major release? It's out of Maintanance for a long time now.

https://guides.rubyonrails.org/maintenance_policy.html

If someone still uses Rails 5.0 or older they can pin the version.

Devise::Orm.active_record_51?(model) as method seems odd because it's true for Rails > 5.1 as well.

carlosantoniodasilva commented 1 year ago

Maybe you could reconsider adding this methode back and drop Support for Rails <= 5.0 in a Major release? It's out of Maintanance for a long time now.

I have just released v4.9.2 with that method again, but it will show a deprecation warning now. To be clear, people shouldn't be relying on an explicitly non-documented method, but from experience they tend to ignore that.

I plan on dropping more versions when I bump a major release to be more inline with the Rails maintenance policy, but there's more work to do that doesn't require breaking changes or a major release before I get to that, and I'm trying to be a bit mindful of that to not make it harder on people to upgrade. But that should happen soon-ish anyway. Thanks.

Devise::Orm.active_record_51?(model) as method seems odd because it's true for Rails > 5.1 as well.

Indeed, that's to represent AR 5.1 and up, but the method name isn't that clear. At least it is only used in one place now after the changes I refactored there. But again, that method (and whole module) is not intended for external usage.

rocket-turtle commented 1 year ago

Thank you for the clarification and your time working on that gem.

Looking forward to the new Major release :)

olbrich commented 1 year ago

There is a new release of devise-security that addresses this issue. https://github.com/devise-security/devise-security/releases/tag/v0.18.0