collectiveidea / audited

Audited (formerly acts_as_audited) is an ORM extension that logs all changes to your Rails models.
MIT License
3.38k stars 662 forks source link

Audited gem is partially incompatible with Rails 7.2.x and the Rails team insist you fix it #725

Open pond opened 3 weeks ago

pond commented 3 weeks ago

Please see https://github.com/rails/rails/issues/52715 - this looks more to me like a Rails bug than an Audited gem bug, which is why I've filed the report over there. There's a small chance, though, that it's some issue with the inner workings of Audited.

Issue posted here really as an FYI, but of course, feel free to close if you're confident that it's a Rails problem.

pond commented 3 weeks ago

BUMP To my absolute horror the Rails maintainers have closed the issue saying it must be fixed in the gem, even though I can't see how you could possibly do that.

This is an urgent issue. Audited is incompatible with Rails 7.2 at this time, for certain use cases that worked fine for many years prior.

pond commented 3 weeks ago

(Edited) Rails maintainers have clarified their position and point to documentation that indicates Audited may have been relying on undefined behaviour previously.

https://api.rubyonrails.org/classes/ActiveRecord/Inheritance/ClassMethods.html

If you are using inheritance with Active Record and don’t want a class to be considered as part of the STI hierarchy, you must set this to true.

...they recommend defining inherited in the Audit::Audited class and setting self to abstract therein.

If you're happy with that approach, I am happy to do that work and send a PR for the change.

byroot commented 3 weeks ago

and the Rails team insist you fix it

FYI, we don't insist anything and this sort of phrasing isn't a good way to get project maintainers help.

pond commented 3 weeks ago

@byroot I don't care how my words land, I just want this fixed, and I'm happy to do the work to do so, whichever project that lands in. My numerous PRs to fix open source projects prior speak for themselves. I contribute to this community routinely.

I do not have time for politics.

byroot commented 3 weeks ago

I don't care how my words land,

Well you should, because I was very close to just ignore you on the other issue because of your tone.

And I could fix your issue in 2 minutes or even give you a one line snippet you could include in an initializer that would fix your issue. But since you don't care about how your words land, I guess I don't care if your problem is fixed.

pond commented 3 weeks ago

Well you should, because I was very close to just ignore you on the other issue because of your tone.

The other issue being https://github.com/rails/rails/issues/52715, which you closed as "won't fix", saying that the gem must do it, but that's not "insisting"? What word would you use for that, then?

because of your tone.

Exactly which part of which comment, please? A link would help. Would be this be the part where I produced a tight replication test case, explained everything in detail in a very well formatted issue precisely to the Rails guidelines required template? Or was it in https://github.com/rails/rails/issues/52715#issuecomment-2311352357, where I hoped for a resolution and conversed genially? Or would it be after you closed the issue saying you were not going to do anything on the Rails side?

And I could fix your issue in 2 minutes

If you mean a work-around, it's clearly trivial to add:

  if Rails.gem_version < Gem::Version.new('7.2.2')
    ::Audited::Audit.class_eval do
      self.table_name = 'audit_trails'
    end
  end

...in config/initializers/audited.rb, which is what I did as soon as the issue came to light. But this does not help other community members, which is what we're here for right now.

I do not have time for politics.

pond commented 3 weeks ago

Oh and incidentally - and apologies, I should have said this much sooner - could we perhaps take this off the Audited gem's issue?! I'm sure the maintainers don't want to plough through this ridiculous bickering.

You can contact me on ahodgkin@rowing.org.uk per public profile, or I can fork this gem and you can continue in an issue on the fork should you want the discourse to be public.

pond commented 3 weeks ago

After trying many approaches, I decided there was no good way to automate this and it essentially cannot be fixed. See https://github.com/collectiveidea/audited/pull/726. This describes three coherent solution options (out of many intermediate hacks not covered!) all of which have problems - a maintainer might in fact decide that one of those is acceptable, but otherwise, a warning in README.md is all we have left.