Fix backward compatibility issue introduced by #126#143
Conversation
lib/active_record/session_store.rb
Outdated
| ActiveSupport.on_load(:active_record, yield: true) do | ||
| next unless ActiveRecord::SessionStore.autoload?(:Session) | ||
|
|
||
| ActionDispatch::Session::ActiveRecordStore.session_class = ActiveRecord::SessionStore::Session |
There was a problem hiding this comment.
Isn't this duplicated in the session class?
There was a problem hiding this comment.
Not sure if we should se the session_class when loading session. Do we need to do that?
There was a problem hiding this comment.
We can't remove the session_class assignment when loading the session.rb file and just have the assignment in the lazy hook. This is because of the edge case I mentioned in the PR description.
TL;DR If the host application reference Session, "session.rb" gets loaded, the active_record hook is dispatched, at this point the Session constant is not yet defined since the AS hook takes precedence, calling Session one more time in the hook will just raise a undefined constant error. Chicken and egg situation really
There was a problem hiding this comment.
Ok, so we should just require 'active_record/session_store/session'in this hook them. If the file was already required it will just returnfalse`.
There was a problem hiding this comment.
I have a gift for finding overcomplicated solutions 🤦♂️ . Thanks!
|
I agree it seem over complicated but couldn't find a better alternative.
I'm not sure if I understand what you mean, the problem will persist as soon as |
|
From #142 (comment): # config/initializers/session_store.rb
Rails.application.config.session_store(:active_record_store)
ActiveSupport.on_load(:active_record) do
ActiveRecord::SessionStore::Session.serializer = :json
endIt's only referenced to set the serializer (unless I'm misunderstanding how the first line works). |
|
Yeah that code is actually the right way to configure the session. What we need to fix is the case where |
Edouard-chin
force-pushed
the
ec-fix-lazy-hook-backward-compatibility
branch
from
a32740e to
33231d8
Compare
- The goal of rails#126 was to lazily require the 'session.rb' file as otherwise, we'd load `Active::Record` base too prematurely during the boot process. This change had the negative effect of breakin apps that were setting configuration on the `Session` **before** the hook was getting triggered, thus ending up with a "uninitialized constant" error. This patches move the `session_class` assignment when loading then `session.rb` file as well as autoloading the `AR::SessionStore::Session` constant so that application referencing it too early will still work.
Edouard-chin
force-pushed
the
ec-fix-lazy-hook-backward-compatibility
branch
from
33231d8 to
588d83d
Compare
|
This should be good to go Rafael/Kasper |
|
Thank you! I'm going to push out |
…ard-compatibility Fix backward compatibility issue introduced by rails#126
…ard-compatibility Fix backward compatibility issue introduced by rails#126
4 participants
otherwise, we'd load
Active::Recordbase too prematurely duringthe boot process.
This change had the negative effect of breakin apps that were
setting configuration on the
Sessionbefore the hook wasgetting triggered, thus ending up with a "uninitialized constant"
error.
This patches move the
session_classassignment when loading thensession.rbfile as well as autoloading theAR::SessionStore::Sessionconstant so that application referencingit too early will still work.