[PATCH] Experimental Plugin::Locator/Plugin::Loader implementation to make future changes to plugin loading system easier

Refactor plugin locator and loader into more testable classes

This feels excellent. One question however ( as I said in IRC ) :

Why does a plugin's init.rb need to access configuration object ? Can you please point to some good use case of that ?

+1

As far as I can tell, there's no use-case - in fact, the config variable was present in Jamis' original plugin checkin. That said, I think it's probably useful for a plugin to have access to the configuration. Once example might be detecting which frameworks are being loaded - if action_mailer isn't in the config.frameworks list, the plugin might opt not to enable it's mailing functionality.

+1 Works well for me with one small change: I think the call to add_plugin_load_paths in initializer.rb needs to be lower down.

As it is I can't autoload things from plugins as set_autoload_paths clobbers Dependencies.load_paths, killing whatever is set by add_plugin_load_paths. I'm not sure where the best place is, but anywhere after set_autoload_paths seems to work for me.

Other than that does exactly what it says on the tin.

+1

This seems like a good refactoring to me. Clearly #9793 also fixes the problem, however, this is a much cleaner attempt at making the affected area easier to understand.

Three +1s - adding the verified keyword.

I don't have a wifi connection to commit, but I ended up making the change that fcheung mentioned. I'll commit later today.

(In [8115]) Refactor Plugin Loader. Add plugin lib paths early, and add lots of tests. Closes #9795 [lazyatom]

I've found that this changeset has introduced bugs in my testing environment.

Specifically, I have a plugin that introduces some subclasses of Exception. The file containing these exceptions (exceptions.rb) looks something like this:

module MyModule
  class MyError < StandardError
  end
end

This file is require'd within the plugin's init.rb.

Then, within a functional test I do something like:

def test_error_found
  assert_raises MyModule::MyError do
    get :show
  end
end

After r8115, I get an exception "uninitialized constant MyModule::MyError"

Looks like you have an unconventional setup. Normally I'd put such an exception in my_module.rb or my_module/my_error.rb. What's the problem though? Is init.rb not being run? What plugin is it?

Replying to technoweenie:

Looks like you have an unconventional setup. Normally I'd put such an exception in my_module.rb or my_module/my_error.rb. What's the problem though? Is init.rb not being run? What plugin is it?

I was baffled by the fact that init.rb IS being run yet the exceptions are not available in tests. After a bit of logging, I determined that exceptions.rb IS NOT being loaded despite a very clear "require 'exceptions'" in init.rb. Furthermore, I don't get any "MissingSourceFile exceptions either! After a bit more research...

Rails is loading exceptions.rb, but from another plugin

Yup -after some logging I determined that Rails is loading a same-named file but from a different plugin. In fact, it's loading the other exceptions.rb twice!

I hope this explanation is clear and helps pinpoint the source of the problem.

-Chris

It sounds like this would be because all plugins are available on the $LOAD_PATH now, and the other plugin was loaded before the one in question. So, this could be a namespacing issue?

No, it's not a namespacing issue -both plugins with their respective lib/exceptions.rb file are in difference namespaces (named after the plugin and with a simple single-level namespace. The exception classes in the exceptions.rb files are in subordinate namespaces within those differing namespaces. Keep in mind that I never had a problem with either of these plugins until r8115.

FYI, the two names spaces from the two exceptions.rb files are as follows:

module Authorize
  class AuthorizationError < StandardError; end
  class TrusteeDoesntImplementRoles < AuthorizationError; end  
  ...
end

module Authenticate
  class AuthenticationError < StandardError; end
  class InvalidTokenExpiry < AuthenticationError; end
  ...
end

Yes, but the files are called 'exceptions.rb'. You should name them differently by following my previous suggestion to prevent clashes.

cch1: Are both the files called exceptions.rb, and in the root lib dir of each plugin?, i.e.

plugin_a/lib/exceptions.rb:
  module PluginA
    class ExceptionA
    end
  end

plugin_b/lib/exceptions.rb:
  module PluginB
    class ExceptionB
    end
  end

... if this is the case, there's no way for ruby to 'know' which file you mean when you call require 'exceptions' other than precidence (ordering) in the $LOAD_PATH. So, chances are that the file from plugin_a will get loaded in preference to the one in plugin_b.

To get around this, your exceptions.rb files will need to be in paths that are distinct, such as technoweenie suggests:

plugin_a/lib/plugin_a/exception_a.rb
plugin_b/lib/plugin_b/exception_b.rb

(actually it's the plugin_a/plugin_b part of the path that's important, the files could still be called exception.rb)

Does this help? If not, feel free to email me (james.adam@gmail.com) with more details about the plugin layout, and I'll see what I can do.

My previous good fortune with everything being loaded as needed was apparently happenstance. I've now taken the advice of lazyatom and technoweenie and change the directory structure of my plugins to ensure that the load namespace is not ambiguous. Effectively, I no longer require 'exception' but rather 'my_plugin/exceptions'.

I sincerely hope I am the only one who needed to learn this lesson and that, in the name of greater flexibility, this changeset quietly becomes "Rails."

-Chris