Closed toncid closed 3 years ago
For some reason, the default class from the gem is never overridden by our code in v12:
v11.1.0
Module.const_source_location("PaperTrail::Version")
"/home/tonci/dev/myapp/app/models/paper_trail/version.rb"
v12.0.0
Module.const_source_location("PaperTrail::Version")
"/home/tonci/.rvm/gems/ruby-2.6.6/gems/paper_trail-12.0.0/lib/paper_trail/frameworks/active_record/models/paper_trail/version.rb"
Hey @toncid, thanks for sharing your experience. I'm just chiming in that I'm fighting the same issue... without much success. I tried a few things but couldn't make it work.
I reverted back to to v11.1
, until there's some clarity what's the issue here ¯\(ツ)/¯
Edit:
p.s. overriding in v12 doesn't work in production
in my case, too.
@StanBright Thanks for the follow-up and testing in the production mode. We have tried a few different approaches to circumvent the problem, none of them successful, so reverting to v11.1 was also the only option left.
I can confirm the issue, also i have no resolution. On our projects we go with a lower version of papertrail for now.
Thank you for opening this and for everyone providing all of the extra details. Can confirm this issue. We recently changed loading in https://github.com/paper-trail-gem/paper_trail/pull/1281
I'm trying to understand the issue. It's interesting that this appears to work with Rails 6 and zeitwerk
enabled.
It seems like it could potentially work if we move the overridden Version
class into an initializer. I'm curious what you think @jaredbeck?
Hi y'all. Thanks for the report. I tried to reproduce this in a new rails app (6.1) but no luck.
bin/rails console
Loading development environment (Rails 6.1.3.1)
PaperTrail.gem_version
=> #<Gem::Version "12.0.0">
Module.const_source_location("PaperTrail::Version")
=> ["redacted/pt_issue_1305/app/models/paper_trail/version.rb", 2]
PaperTrail::Version.new.is_a?(PaperTrail::VersionConcern)
=> true
What am I missing?
We can reproduce in Rails 6.1 by enabling classic mode config.autoloader = :classic
We can reproduce in Rails 6.1 by enabling classic mode config.autoloader = :classic
Thanks Todd. With that setting, I can reproduce the issue:
Module.const_source_location("PaperTrail::Version")
=> ["redacted/paper_trail/frameworks/active_record/models/paper_trail/version.rb", 13]
Classic mode is deprecated, but I'd like PT to support it as long as is practical. Contributions are welcome. If there are practical workarounds, let's document them.
In the changelog, I will reclassify #1281 as a breaking change.
Hi @toncid, @StanBright, @criess, does it work if you move the overridden Version
class into an initializer?
You might also want to play around with adding a require_dependency
in your app/models/paper_trail/version.rb
.
This is happening because the new railtie is requiring the active_record framework: https://github.com/paper-trail-gem/paper_trail/pull/1281/files#diff-f226b8d714b180354bcadd32e8b0f55afb2e4d9e2c639c28bcf248aba53b38e0R26 , and the previous engine didn't. The AR framework in turn requires the model class. Apparently using AR-only PT never supported overriding the Version class. (possibly because it doesn't make much sense. Who is doing the autoloading if not rails?)
I have worked around this by adding this in my PT initializer:
module PaperTrail
remove_const :Version
end
I believe the proper fix is to not require the Version model, but to keep adding the autoload path. Here is a StackOverflow answer suggesting how to do it: https://stackoverflow.com/a/6394946 . So maybe something like moving the require 'paper_trail/frameworks/active_record/models/paper_trail/version.rb'
out of the active_record framework, and add an initializer that preserves the autoload mangling from 11.1 .
Thanks everyone for a productive discussion.
Classic mode is deprecated, but I'd like PT to support it as long as is practical. Contributions are welcome. If there are practical workarounds, let's document them.
I'll close this for now with the understanding that our preferred solution is zeitwerk.
- config.autoloader = :classic # deprecated
+ config.autoloader = :zeitwerk
PRs welcome.
Sorry for not mentioning this. I'm seeing the error with the zeitwerk loader.
Hmmmm, we're using zeitwerk and ran into this issue. However, we've solved it by creating a new class with our additions (PaperTrail::ApplicationVersion
) that inherits from PaperTrail::Version
and configuring our models to use that class for versioning.
Hmmmm, we're using zeitwerk and ran into this issue. However, we've solved it by creating a new class with our additions (
PaperTrail::ApplicationVersion
) that inherits fromPaperTrail::Version
and configuring our models to use that class for versioning.
I like the sound of that, Joel. You might also try include VersionConcern
if you prefer that over inheritance. You might want to inherit from ApplicationRecord
, for example.
class ApplicationVersion < ApplicationRecord
include PaperTrail::VersionConcern
end
I haven't tested the above, and I don't use Custom Version Classes personally, so I'd warmly welcome documentation PRs from people who do.
Well, I spoke too soon. That change worked just fine in rails console but completely blew up our spec suite (specs won't event run because of an error during setup). I'll report back after I know more.
@jaredbeck your include PaperTrail::VersionConcern
solution seems to work but I need to do more testing to confirm. Are we losing anything from PaperTrail::Version
when we do it that way?
Hey guys, why is this issue closed? I am still experimenting the same and I couldn't make any of the workarounds work. Maybe I am doing something wrong, that is our custom module which is getting ignored when upgrading from v11.1.0
to v12.3.0
(tried with :zeitwerk
and :classic
as well, no difference):
module PaperTrail
class Version < ActiveRecord::Base
include PaperTrail::VersionConcern
belongs_to :user, foreign_key: :whodunnit, optional: true
end
end
@istvan-ujjmeszaros I was able to work around this bug by defining Version
model in models
folder and inherit from PaperTrail::Version
to have access to such methods as changeset
Works like a charm!
# frozen_string_literal: true
class Version < PaperTrail::Version
belongs_to :item, polymorphic: true
belongs_to :user
end
Switched to 6.1.7 with zeitwerk. Facing the same issue with paper_trail 13.0.0 and with 14.0.0 Is it possible that there are still cases that haven't been addressed with the fix and this should not be closed.
We define
module PaperTrail
class Version < ActiveRecord::Base
include PaperTrail::VersionConcern
in app/models/paper_trail/version.rb
and this never gets loaded because of
Zeitwerk@rails.main: file /Users/kireto/..../app/models/paper_trail/version.rb is ignored because PaperTrail::Version is already defined
What I had to do for the spec to pass is to do
# config/initializers/paper_trail.rb
require "paper_trail/version"
And I don't believe this should be used, especially with zeitwerk. Right?
March 2023: I can report this issue to be current still running Rails 6.1 with Zeitwerk and PT 14.
Loading development environment (Rails 6.1.3.2)
irb(main):001:0> Rails.application.config.autoloader
=> :zeitwerk
irb(main):002:0> Module.const_source_location("PaperTrail::Version")
=> ["/home/adrien/.gem/ruby/2.7.4/gems/paper_trail-14.0.0/lib/paper_trail/frameworks/active_record/models/paper_trail/version.rb", 13]
irb(main):003:0>
@weezing Inheritance only works if you work from the item and up, but is useless if your starting point is PaperTrail
as in:
Contract.new.versions # works
# versus
PaperTrail::Version.includes(:item).where(item_type: 'Contract').order(created_at: :desc) # does not work
I have tried @thebravoman solution and it works.
Create an initializer that requires our custom model.
Hello,
It seems that the v12 somehow interferes with Rails lazy autoloading in the development environment. The overriding class is no longer concerned. See below the script for more info.
Check the following boxes:
paper_trail
gemDue to limited volunteers, we cannot answer usage questions. Please ask such questions on StackOverflow.
Bug reports must use the following template:
The script runs well, but this is due to avoided autoloading (
PaperTrail::Version
is defined in the same script).The problem is with Rails when
PaperTrail::Version
is redefined in another file (see: https://github.com/paper-trail-gem/paper_trail/blob/v12.0.0/README.md#5b2-item-association). This no longer works in development where autoloading is lazy.Any advice?