Skip to content

Commit

Permalink
Added app/[models|controllers|helpers] to the load path for plugins t…
Browse files Browse the repository at this point in the history
…hat has an app directory (go engines ;)) [DHH]
  • Loading branch information
dhh committed Nov 26, 2008
1 parent 133c349 commit 63d8f56
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 16 deletions.
2 changes: 2 additions & 0 deletions railties/CHANGELOG
@@ -1,5 +1,7 @@
*2.3.0 [Edge]*

* Added app/[models|controllers|helpers] to the load path for plugins that has an app directory (go engines ;)) [DHH]

* Add config.preload_frameworks to load all frameworks at startup. Default to false so Rails autoloads itself as it's used. Turn this on for Passenger and JRuby. Also turned on by config.threadsafe! [Jeremy Kemper]

* Add a rake task to generate dispatchers : rake rails:generate_dispatchers [Pratik]
Expand Down
25 changes: 22 additions & 3 deletions railties/lib/rails/plugin.rb
Expand Up @@ -28,13 +28,17 @@ def initialize(directory)
end

def valid?
File.directory?(directory) && (has_lib_directory? || has_init_file?)
File.directory?(directory) && (has_app_directory? || has_lib_directory? || has_init_file?)
end

# Returns a list of paths this plugin wishes to make available in <tt>$LOAD_PATH</tt>.
def load_paths
report_nonexistant_or_empty_plugin! unless valid?
has_lib_directory? ? [lib_path] : []

returning [] do |load_paths|
load_paths << lib_path if has_lib_directory?
load_paths << app_paths if has_app_directory?
end.flatten
end

# Evaluates a plugin's init.rb file.
Expand Down Expand Up @@ -68,7 +72,16 @@ def load_about_information

def report_nonexistant_or_empty_plugin!
raise LoadError, "Can not find the plugin named: #{name}"
end
end


def app_paths
[
File.join(directory, 'app', 'models'),
File.join(directory, 'app', 'controllers'),
File.join(directory, 'app', 'helpers')
]
end

def lib_path
File.join(directory, 'lib')
Expand All @@ -86,6 +99,11 @@ def init_path
File.file?(gem_init_path) ? gem_init_path : classic_init_path
end


def has_app_directory?
File.directory?(File.join(directory, 'app'))
end

def has_lib_directory?
File.directory?(lib_path)
end
Expand All @@ -94,6 +112,7 @@ def has_init_file?
File.file?(init_path)
end


def evaluate_init_rb(initializer)
if has_init_file?
silence_warnings do
Expand Down
10 changes: 7 additions & 3 deletions railties/lib/rails/plugin/loader.rb
Expand Up @@ -33,6 +33,7 @@ def load_plugins
plugin.load(initializer)
register_plugin_as_loaded(plugin)
end

ensure_all_registered_plugins_are_loaded!
end

Expand All @@ -45,12 +46,15 @@ def add_plugin_load_paths
plugins.each do |plugin|
plugin.load_paths.each do |path|
$LOAD_PATH.insert(application_lib_index + 1, path)
ActiveSupport::Dependencies.load_paths << path

ActiveSupport::Dependencies.load_paths << path

unless Rails.configuration.reload_plugins?
ActiveSupport::Dependencies.load_once_paths << path
end
end
end

$LOAD_PATH.uniq!
end

Expand All @@ -59,9 +63,9 @@ def add_plugin_load_paths
# The locate_plugins method uses each class in config.plugin_locators to
# find the set of all plugins available to this Rails application.
def locate_plugins
configuration.plugin_locators.map { |locator|
configuration.plugin_locators.map do |locator|
locator.new(initializer).plugins
}.flatten
end.flatten
# TODO: sorting based on config.plugins
end

Expand Down
8 changes: 4 additions & 4 deletions railties/test/initializer_test.rb
Expand Up @@ -209,23 +209,23 @@ def test_only_the_specified_plugins_are_located_in_the_order_listed
def test_all_plugins_are_loaded_when_registered_plugin_list_is_untouched
failure_tip = "It's likely someone has added a new plugin fixture without updating this list"
load_plugins!
assert_plugins [:a, :acts_as_chunky_bacon, :gemlike, :plugin_with_no_lib_dir, :stubby], @initializer.loaded_plugins, failure_tip
assert_plugins [:a, :acts_as_chunky_bacon, :engine, :gemlike, :plugin_with_no_lib_dir, :stubby], @initializer.loaded_plugins, failure_tip
end

def test_all_plugins_loaded_when_all_is_used
plugin_names = [:stubby, :acts_as_chunky_bacon, :all]
only_load_the_following_plugins! plugin_names
load_plugins!
failure_tip = "It's likely someone has added a new plugin fixture without updating this list"
assert_plugins [:stubby, :acts_as_chunky_bacon, :a, :gemlike, :plugin_with_no_lib_dir], @initializer.loaded_plugins, failure_tip
assert_plugins [:stubby, :acts_as_chunky_bacon, :a, :engine, :gemlike, :plugin_with_no_lib_dir], @initializer.loaded_plugins, failure_tip
end

def test_all_plugins_loaded_after_all
plugin_names = [:stubby, :all, :acts_as_chunky_bacon]
only_load_the_following_plugins! plugin_names
load_plugins!
failure_tip = "It's likely someone has added a new plugin fixture without updating this list"
assert_plugins [:stubby, :a, :gemlike, :plugin_with_no_lib_dir, :acts_as_chunky_bacon], @initializer.loaded_plugins, failure_tip
assert_plugins [:stubby, :a, :engine, :gemlike, :plugin_with_no_lib_dir, :acts_as_chunky_bacon], @initializer.loaded_plugins, failure_tip
end

def test_plugin_names_may_be_strings
Expand Down Expand Up @@ -299,7 +299,7 @@ def test_config_defaults_and_settings_should_be_added_to_i18n_defaults
File.expand_path("./test/../../activesupport/lib/active_support/locale/en.yml"),
File.expand_path("./test/../../actionpack/lib/action_view/locale/en.yml"),
"my/test/locale.yml",
"my/other/locale.yml" ], I18n.load_path
"my/other/locale.yml" ], I18n.load_path.collect { |path| path =~ /^\./ ? File.expand_path(path) : path }
end

def test_setting_another_default_locale
Expand Down
25 changes: 19 additions & 6 deletions railties/test/plugin_loader_test.rb
Expand Up @@ -48,16 +48,16 @@ def test_should_return_empty_array_if_configuration_plugins_is_empty
end

def test_should_find_all_availble_plugins_and_return_as_all_plugins
assert_plugins [:stubby, :plugin_with_no_lib_dir, :gemlike, :acts_as_chunky_bacon, :a], @loader.all_plugins.reverse, @failure_tip
assert_plugins [ :engine, :stubby, :plugin_with_no_lib_dir, :gemlike, :acts_as_chunky_bacon, :a], @loader.all_plugins.reverse, @failure_tip
end

def test_should_return_all_plugins_as_plugins_when_registered_plugin_list_is_untouched
assert_plugins [:a, :acts_as_chunky_bacon, :gemlike, :plugin_with_no_lib_dir, :stubby], @loader.plugins, @failure_tip
assert_plugins [:a, :acts_as_chunky_bacon, :engine, :gemlike, :plugin_with_no_lib_dir, :stubby], @loader.plugins, @failure_tip
end

def test_should_return_all_plugins_as_plugins_when_registered_plugin_list_is_nil
@configuration.plugins = nil
assert_plugins [:a, :acts_as_chunky_bacon, :gemlike, :plugin_with_no_lib_dir, :stubby], @loader.plugins, @failure_tip
assert_plugins [:a, :acts_as_chunky_bacon, :engine, :gemlike, :plugin_with_no_lib_dir, :stubby], @loader.plugins, @failure_tip
end

def test_should_return_specific_plugins_named_in_config_plugins_array_if_set
Expand All @@ -74,17 +74,17 @@ def test_should_respect_the_order_of_plugins_given_in_configuration

def test_should_load_all_plugins_in_natural_order_when_all_is_used
only_load_the_following_plugins! [:all]
assert_plugins [:a, :acts_as_chunky_bacon, :gemlike, :plugin_with_no_lib_dir, :stubby], @loader.plugins, @failure_tip
assert_plugins [:a, :acts_as_chunky_bacon, :engine, :gemlike, :plugin_with_no_lib_dir, :stubby], @loader.plugins, @failure_tip
end

def test_should_load_specified_plugins_in_order_and_then_all_remaining_plugins_when_all_is_used
only_load_the_following_plugins! [:stubby, :acts_as_chunky_bacon, :all]
assert_plugins [:stubby, :acts_as_chunky_bacon, :a, :gemlike, :plugin_with_no_lib_dir], @loader.plugins, @failure_tip
assert_plugins [:stubby, :acts_as_chunky_bacon, :a, :engine, :gemlike, :plugin_with_no_lib_dir], @loader.plugins, @failure_tip
end

def test_should_be_able_to_specify_loading_of_plugins_loaded_after_all
only_load_the_following_plugins! [:stubby, :all, :acts_as_chunky_bacon]
assert_plugins [:stubby, :a, :gemlike, :plugin_with_no_lib_dir, :acts_as_chunky_bacon], @loader.plugins, @failure_tip
assert_plugins [:stubby, :a, :engine, :gemlike, :plugin_with_no_lib_dir, :acts_as_chunky_bacon], @loader.plugins, @failure_tip
end

def test_should_accept_plugin_names_given_as_strings
Expand Down Expand Up @@ -112,6 +112,19 @@ def test_should_add_plugin_load_paths_to_Dependencies_load_paths
assert ActiveSupport::Dependencies.load_paths.include?(File.join(plugin_fixture_path('default/acts/acts_as_chunky_bacon'), 'lib'))
end


def test_should_add_engine_load_paths_to_Dependencies_load_paths
only_load_the_following_plugins! [:engine]

@loader.add_plugin_load_paths

%w( models controllers helpers ).each do |app_part|
assert ActiveSupport::Dependencies.load_paths.include?(
File.join(plugin_fixture_path('engines/engine'), 'app', app_part)
), "Couldn't find #{app_part} in load path"
end
end

def test_should_add_plugin_load_paths_to_Dependencies_load_once_paths
only_load_the_following_plugins! [:stubby, :acts_as_chunky_bacon]

Expand Down

24 comments on commit 63d8f56

@svenfuchs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wohoooooo :)

@lazyatom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ;)

@zargony
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I wonder if/how an engine plugin can add routes.

@Virtuaffinity
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see this making it into core, nice stuff!

@tekin
Copy link
Contributor

@tekin tekin commented on 63d8f56 Nov 26, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hallelujah!

@DefV
Copy link
Contributor

@DefV DefV commented on 63d8f56 Nov 26, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zargony
16:28 :: DHH:: It gets better shortly ;)
16:28 :: DHH:: almost have config/routes.rb working for plugins too

@yaroslav
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

@maxlapshin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!!! Waiting for Django-like apps!

@andrewroth
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m happy to see this change, but I am curious — what made you change your mind?

@matthewrudy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to find a good way to extend routing from a plugin.

@lazyatom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewrudy: it’s being taken care of.

@dpickett
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would migrations work for models like this? they’d probably still have to be written through a generator, right?

@boblmartens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I knew exactly what this did, I’d be excited.

@ioquatix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DHH: How does this integrate into GEM plugins? GEMs have a directory called “rails” so shouldn’t “app” and so on be in that directory to keep things in the correct namespace? How about rake tasks? You might want to check out my fork of engines which solves these problems and others: http://github.com/ioquatix/engines/tree/master

@stephankaag
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about views in a plugin?

@Roman2K
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for <plugin-root>/rails/app instead of <plugin-root>/app (following ioquatix’s comment above).

@matthewrudy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you’re on top of everything @lazyatom.

@oboxodo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on engines-like gems support, still supporting rails plugins, but discouraging them.

@m3talsmith
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for /rails/app instead of /app
+1 for bringing engines back

I love having all of the above and the gem dependency options open. I love where rails is headed. Keep it up core.

@ioquatix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m3talsmith: can you clarify your -1 comment? Since everything else for rails is in a subdirectory, why not put app there as well. Its much cleaner overall.

@m3talsmith
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ioquatix:

I’m more concerned about keeping a pure directory structure in the vendor/plugins/your_plugin_or_engine_name area. When I made the minus comment I hadn’t was in a rush on my way out the door. I overlooked the GEM’s part of the comment. Given that I take my -1 back because it does make sense to keep it in the same namespace. So ….

+1 for /rails/app for the GEMS directory
-2 for m3talsmith firing off a comment on the run.

@railsdog
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be like extensions in Radiant or Spree? Both of these projects already support views, controllers, routes, migrations, etc. in your extensions.

@xtoddx
Copy link

@xtoddx xtoddx commented on 63d8f56 Dec 4, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way to run migrations from plugins? Maybe being able to set the schema migrations table name from within the migrations would be enough. I’ve been doing it with monkeypatching and a custom rake task so far.

@dhh
Copy link
Member Author

@dhh dhh commented on 63d8f56 Dec 4, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xtoddx, we’re working on getting a good story for both migrations and files in public in before this is released.

Please sign in to comment.