Don't directly access ::Rails::Application in VersionStrategy#registered_engines

[urbanautomaton]

Hi,

On updating to Cells 3.8.3, our Rails 3.0 app started spitting out the following deprecation message:

DEPRECATION WARNING: Calling a method in Rails::Application is deprecated, please call it directly in your application constant WhateverApp::Application

I traced this down to the Rail 3.0 lookup for registered engines, which talks to Rails::Application directly. The attached commit modifies the calls so that the app instance is passed through, and the old engines lookup can be used.

It's not ready to merge, however, because I've hit a possible bug in this area. The documentation says that adding the following to an initializer will disable the view_path_engines additions:

Cell::Base.config.view_path_engines = false

However the append_engines_view_paths_for method doesn't actually look at this config setting, as far as I can see - instead, it's talking to the application's action_controller config hash:

# lib/cells/railtie.rb
require "rails/railtie"

module Cells
  class Railtie < ::Rails::Railtie
    #...
    initializer "cells.setup_engines_view_paths" do |app|
      Cell::Engines.append_engines_view_paths_for(app.config.action_controller)
    end
  end
end

# lib/cells/engines.rb
#...
module Engines
  extend VersionStrategy

  def self.append_engines_view_paths_for(config)
    return if config.view_path_engines == false

    engines = config.view_path_engines || registered_engines(app)  #::Rails::Application::Railties.engines
    engines.each {|engine| append_engine_view_path!(engine) }
  end
end

There's no such setting in the action_controller config hash, so it seems that all registered Engines are always appended to the view path.

Should this check be talking to Cell::Base.config as per the documentation, or should it be looking elsewhere, and the documentation needs updating? If you let me know what the intention is, I'll update this pull request as necessary.

Cheers,
Simon

[urbanautomaton]

Hi Nick,

I've added a commit so that Engines.append_engines_view_paths_for now accesses Cell::Base.config.view_path_engines as per the docs, and have rebased the branch on top of the current master - tests all pass. Is there anything else I can do to make this ready to merge?

[ajlai]

What's left to be done to fix this? Would love to get rid of the warning on our rails app as well...

[apotonick]

It is true, every engine is added to cells view paths. It should look at Cells' setting first, and then AC. Do we still need this fix? I reckon yes.