Allow to load the application only after calling Combustion.initialize!

[parndt]

I'm not sure about this.. I've been hearing about how autoload is an anti-pattern that busts the method cache. WDYT @pat

[pabloh]

@pat about the method cache, I don't really think this could make any impact at all because of the following reasons: first, even when invalidating the method cache this will only run once before the WHOLE test suite, and secondly, and more important, Rails code already does all sorts of stuff that invalidate the method cache (besides using autoload all over the codebase), so even if this change would flush the method cache, It will be just one time among thousands.

[pat]

I generally avoid autoload for a couple of reasons - every now and then I come across discussions on whether it's threadsafe or not, and it's magic that I feel is unneccessary. But, I'm open to discussion: what's the value in using autoload? Why is this commit useful?

[pabloh]

The primary reason I wanted to add this is to make be sure every rails module is already loaded before the combustion's rails application is, as it would happen on an regular dummy application for engine testing or an actual rails application.

[pat]

I came across this earlier today: autoload will be phased out in Ruby – http://www.ruby-forum.com/topic/3036681

@pabloh does your suggestion of passing a block to Combustion.initialize help deal with this problem as an alternative to using autoload?

[pabloh]

@pat, yes it would but please do not close this PR just yet, there are a few things I would like to bring up first.

[pabloh]

Consider the following scenario: if you aren't making an explicit require of 'active_record/railtie' at spec_herlper.rb and then latter you call 'Combustion.initialize!(:active_record)'.

When the application is loaded (before the Rails gems), this line:

config.active_record.whitelist_attributes = true if config.respond_to? :active_record    

(at https://github.com/pat/combustion/blob/master/lib/combustion/application.rb#L19)

will be executed before this other line:

modules.each { |mod| require "#{mod}/railtie" }

(at https://github.com/pat/combustion/blob/master/lib/combustion.rb#L19)

so for example the condition will be evaluated as false when it but shouldn't had to if ':active_record' was given as a parameter to the Combustion#initialize! call later.

I think this behavior should be regarded as a bug, and even though this is only a single bug, similar issues may arise in the future if someone for some reason needs to modify combustion/application.rb.

When I sent this PR I was trying to make combustion's application behave more like a real-world rails app, I just didn't anticipate that autoload would be so resisted, I mean is merely an implementation detail.

[pat]

Just to clarify - is this still an outstanding issue? Your other pull requests have not resolved it?

[pabloh]

@pat, nope, is not an outstanding issue for me, and is not affecting me right now (given the other PR has been accepted), I just wanted to bring it up to eventually address it.

[pat]

But it's still a bug, then? It would be great to have it addressed before we release 0.5.0.

[pabloh]

I was thinking about releasing 0.5.0 first and then addressing this at 0.5.1. since we don't seem to agree about how to do it. I won't really mind about using autoload though it hasn't been decided if it's going to be deprecated or fixed, and we don't have and probably won't ever have muli-threaded code at this gem.

[pat]

If we shifted the whitelist_attributes line into the configure_for_combustion method, then it should be reliable, right?

[pabloh]

I guess so, unless the user changes the value for 'whitelist_attributes' at spec_helper.rb and then executes Combustion.initialize!, he may be overwriting the value he set inadvertently.

May be a new strategy for setting environment values is needed.

[pabloh]

@pat thinking more about this I think you could just move that line to #configure_for_combustion, since there's same thing for every other config value set at that method.

[pat]

Done, so closing this PR.