New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#11381: Ignore config.eager_load=true for rake #11389

Merged
merged 1 commit into from Jul 10, 2013

Conversation

Projects
None yet
6 participants
@pftg
Contributor

pftg commented Jul 10, 2013

Closes #11381

@@ -9,7 +9,6 @@ class RakeTest < ActiveSupport::TestCase
def setup
build_app
boot_rails
FileUtils.rm_rf("#{app_path}/config/environments")

This comment has been minimized.

@pftg

pftg Jul 10, 2013

Contributor

I'm not sure, why this added. But for current version it's useless.

@pftg

pftg Jul 10, 2013

Contributor

I'm not sure, why this added. But for current version it's useless.

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Jul 10, 2013

Contributor

@josevalim may your review this PR.

Contributor

pftg commented Jul 10, 2013

@josevalim may your review this PR.

@miry

This comment has been minimized.

Show comment
Hide comment
@miry

miry commented Jul 10, 2013

👍

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward
Member

vipulnsward commented Jul 10, 2013

👍

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 10, 2013

Contributor

Thanks @pftg ! Have you investigated why setting the config option is not enough?

Contributor

josevalim commented Jul 10, 2013

Thanks @pftg ! Have you investigated why setting the config option is not enough?

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Jul 10, 2013

Contributor

@josevalim, default config/environments/production.rb has config.eager_load = true which invoked on loading environment https://github.com/rails/rails/blob/master/railties/lib/rails/application.rb#L295 and overide https://github.com/rails/rails/blob/master/railties/lib/rails/application.rb#L294. (Environment loads only on invoking rake task, after we setup config.eager_load = false). After in initializer :eager_load! do https://github.com/rails/rails/blob/master/railties/lib/rails/application/finisher.rb#L53 we have config.eager_load == true, but I think expected value should be false

Contributor

pftg commented Jul 10, 2013

@josevalim, default config/environments/production.rb has config.eager_load = true which invoked on loading environment https://github.com/rails/rails/blob/master/railties/lib/rails/application.rb#L295 and overide https://github.com/rails/rails/blob/master/railties/lib/rails/application.rb#L294. (Environment loads only on invoking rake task, after we setup config.eager_load = false). After in initializer :eager_load! do https://github.com/rails/rails/blob/master/railties/lib/rails/application/finisher.rb#L53 we have config.eager_load == true, but I think expected value should be false

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 10, 2013

Contributor

@pftg Thanks a lot! Couldn't we simply then move the config.eager_load=false to the line after require_environment! and avoid overriding the whole def eager_load method?

Contributor

josevalim commented Jul 10, 2013

@pftg Thanks a lot! Couldn't we simply then move the config.eager_load=false to the line after require_environment! and avoid overriding the whole def eager_load method?

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Jul 10, 2013

Contributor

I'm also looking for more clean solution, maybe adding new initializer with setting eager_load to false.

But for now Finisher.initializers_for(self) runs in require_environment!, and config.eager_load=false after require_environment will be usable only for next rake tasks only.

Contributor

pftg commented Jul 10, 2013

I'm also looking for more clean solution, maybe adding new initializer with setting eager_load to false.

But for now Finisher.initializers_for(self) runs in require_environment!, and config.eager_load=false after require_environment will be usable only for next rake tasks only.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 10, 2013

Contributor

@pftg there is a before_initialize callback that we could use. Try this:

ActiveSupport.on_load(:before_initialize) { config.eager_load = false }

Or:

ActiveSupport.on_load(:before_initialize) { |app| app.config.eager_load = false }

Could you please give it a try?

Contributor

josevalim commented Jul 10, 2013

@pftg there is a before_initialize callback that we could use. Try this:

ActiveSupport.on_load(:before_initialize) { config.eager_load = false }

Or:

ActiveSupport.on_load(:before_initialize) { |app| app.config.eager_load = false }

Could you please give it a try?

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Jul 10, 2013

Contributor

Cool, thanks. I'm on my way to try it!

Contributor

pftg commented Jul 10, 2013

Cool, thanks. I'm on my way to try it!

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 10, 2013

Contributor

@pftg Nice, let me know so we can merge this, thanks! :)

Contributor

josevalim commented Jul 10, 2013

@pftg Nice, let me know so we can merge this, thanks! :)

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Jul 10, 2013

Contributor

@josevalim, sorry for delay. I updated PR. before_initialize works like a charm. Thanks!

Contributor

pftg commented Jul 10, 2013

@josevalim, sorry for delay. I updated PR. before_initialize works like a charm. Thanks!

josevalim added a commit that referenced this pull request Jul 10, 2013

Merge pull request #11389 from jetthoughts/11381_fix_hit_database_on_…
…precompile

 #11381: Ignore config.eager_load=true for rake

@josevalim josevalim merged commit e7e81b4 into rails:master Jul 10, 2013

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 10, 2013

Contributor

❤️ 💚 💙 💛 💜

Contributor

josevalim commented Jul 10, 2013

❤️ 💚 💙 💛 💜

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Jul 10, 2013

Contributor

Thanks! 🆒

Contributor

pftg commented Jul 10, 2013

Thanks! 🆒

@roman-zaharenkov

This comment has been minimized.

Show comment
Hide comment
@roman-zaharenkov

roman-zaharenkov Nov 20, 2014

I have only one question, why should we ignore eager load for rake?

roman-zaharenkov commented on 9cac69c Nov 20, 2014

I have only one question, why should we ignore eager load for rake?

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 20, 2014

Member

See the linked issue.

Member

rafaelfranca replied Nov 20, 2014

See the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment