Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Only monkey patch ActiveRecord::Base when it's been loaded. #10

Merged
merged 3 commits into from Mar 25, 2012

Conversation

Projects
None yet
2 participants
Contributor

parndt commented Mar 25, 2012

This prevents the database connection being activated when it is not appropriate.
E.g. asset precompilation with sprockets.

All relevant tests passing for me.
@josevalim Also all of devise tests are passing with this change.

Also, all tests pass with Refinery CMS with this change which is the reason I'm making this change so that asset pre-compilation works out of the box with Heroku.

Thanks! :)

@parndt parndt Only monkey patch ActiveRecord::Base when it's been loaded.
This prevents the database connection being activated when it is not appropriate.
E.g. asset precompilation with sprockets.
c333fc5

@parndt parndt referenced this pull request in refinery/refinerycms Mar 25, 2012

Merged

Asset precompilation has been solved for 2.0.x #1511

@josevalim josevalim commented on an outdated diff Mar 25, 2012

lib/orm_adapter/adapters/active_record.rb
@@ -4,89 +4,91 @@
require 'active_record'
end
-class ActiveRecord::Base
- extend OrmAdapter::ToAdapter
-
- class OrmAdapter < ::OrmAdapter::Base
-
- # Do not consider these to be part of the class list
- def self.except_classes
- @@except_classes ||= [
- "CGI::Session::ActiveRecordStore::Session",
- "ActiveRecord::SessionStore::Session"
- ]
- end
+ActiveSupport.on_load(:active_record) do
+ class ActiveRecord::Base
@josevalim

josevalim Mar 25, 2012

Collaborator

ActiveSupport.on_load is instance eval'ed inside AR, this means we don't need to open ActiveRecord::Base here :)

@josevalim josevalim and 1 other commented on an outdated diff Mar 25, 2012

lib/orm_adapter/adapters/active_record.rb
- extend OrmAdapter::ToAdapter
-
- class OrmAdapter < ::OrmAdapter::Base
-
- # Do not consider these to be part of the class list
- def self.except_classes
- @@except_classes ||= [
- "CGI::Session::ActiveRecordStore::Session",
- "ActiveRecord::SessionStore::Session"
- ]
- end
+ActiveSupport.on_load(:active_record) do
+ class ActiveRecord::Base
+ extend OrmAdapter::ToAdapter
+
+ class OrmAdapter < ::OrmAdapter::Base
@josevalim

josevalim Mar 25, 2012

Collaborator

Can we move this class outside the on_load hook? We could define it outside of the AR like this:

module OrmAdapter
  class ActiveRecord < Base
    # ...
  end
end

And then inside the load hook we simply do:

ActiveSupport.on_load(:active_record) do
  extend ::OrmAdapter::ToAdapter
  self::OrmAdapter = ::OrmAdapter::ActiveRecord
end

Wdyt?

@parndt

parndt Mar 25, 2012

Contributor

I'll take a look right now. I was kinda "ugh" about the original solution too. :)

Contributor

parndt commented Mar 25, 2012

@josevalim I've pushed another commit. Please let me know if you have any problems with it :-)

Collaborator

josevalim commented Mar 25, 2012

This looks great. Could you please run Devise tests again? If they look good, I can release a new version right away. Thanks a lot.

Contributor

parndt commented Mar 25, 2012

@josevalim Devise's tests are passing for me using Ruby 1.9.3 on the master & v2.0.4 tag.

@josevalim josevalim added a commit that referenced this pull request Mar 25, 2012

@josevalim josevalim Merge pull request #10 from parndt/respect_active_support_on_load_hoo…
…k_for_active_record

Only monkey patch ActiveRecord::Base when it's been loaded.
3abfec0

@josevalim josevalim merged commit 3abfec0 into ianwhite:master Mar 25, 2012

Collaborator

josevalim commented Mar 25, 2012

Gem released. Should we bump the version requirement in Devise?

@parndt parndt added a commit to parndt/devise that referenced this pull request Mar 25, 2012

@parndt parndt Bumped orm_adapter requirement up to ~> 0.0.7 per ianwhite/orm_adapte… 473a68d
Contributor

parndt commented Mar 25, 2012

@josevalim haha, we think alike: plataformatec/devise#1738

If you want to do the same for earlier versions than master please cherry-pick that commit to the appropriate locations :-)

@josevalim josevalim added a commit to plataformatec/devise that referenced this pull request Mar 25, 2012

@josevalim josevalim Merge pull request #1738 from parndt/patch-1
Bumped orm_adapter requirement up to ~> 0.0.7 per ianwhite/orm_adapter#10
e9a8c6c

@bryanmtl bryanmtl pushed a commit to bryanmtl/g-refinerycms that referenced this pull request Jan 28, 2014

@parndt @ugisozols parndt + ugisozols Fixed asset precompilation out of the box but requires git source on …
…orm_adapter until my pull request is merged.

Pull Request: ianwhite/orm_adapter#10
456d2c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment