Fix sorting of helpers from different paths

When more than one directory for helpers is provided to a controller, it
should preserver the order of directories. Given 2 paths:

    MyController.helpers_paths = ["dir1/helpers", "dir2/helpers"]

helpers from dir1 should be loaded first. Before this commit, all
helpers were mixed and then sorted alphabetically, which essentially
would require to rename helpers to get desired order.

This is a problem especially for engines, where you would like to be
able to predict accurately which engine helpers will load first.

(closes #6496)

5 files changed, 53 insertions, 2 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index ba66b525bc..2e7b3190fc 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,16 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   change a way of ordering helpers from several directories. Previously,
+    when loading helpers from multiple paths, all of the helpers files were
+    gathered into one array an then they were sorted. Helpers from different
+    directories should not be mixed before loading them to make loading more
+    predictable. The most common use case for such behavior is loading helpers
+    from engines. When you load helpers from application and engine Foo, in
+    that order, first rails will load all of the helpers from application,
+    sorted alphabetically and then it will do the same for Foo engine.
+
+    *Piotr Sarnacki*
+
 *   `truncate` now always returns an escaped HTMl-safe string. The option `:escape` can be used as
     false to not escape the result.
 
diff --git a/actionpack/lib/action_controller/metal/helpers.rb b/actionpack/lib/action_controller/metal/helpers.rb
index 86d061e3b7..66cdfd40ff 100644
--- a/actionpack/lib/action_controller/metal/helpers.rb
+++ b/actionpack/lib/action_controller/metal/helpers.rb
@@ -95,9 +95,9 @@ module ActionController
         helpers = []
         Array(path).each do |_path|
           extract  = /^#{Regexp.quote(_path.to_s)}\/?(.*)_helper.rb$/
-          helpers += Dir["#{_path}/**/*_helper.rb"].map { |file| file.sub(extract, '\1') }
+          names = Dir["#{_path}/**/*_helper.rb"].map { |file| file.sub(extract, '\1') }
+          helpers += names.sort
         end
-        helpers.sort!
         helpers.uniq!
         helpers
       end
diff --git a/actionpack/test/controller/helper_test.rb b/actionpack/test/controller/helper_test.rb
index 757661d8d0..deb234b04f 100644
--- a/actionpack/test/controller/helper_test.rb
+++ b/actionpack/test/controller/helper_test.rb
@@ -46,12 +46,42 @@ end
 class MeTooController < JustMeController
 end
 
+class HelpersPathsController < ActionController::Base
+  paths = ["helpers2_pack", "helpers1_pack"].map do |path|
+    File.join(File.expand_path('../../fixtures', __FILE__), path)
+  end
+  $:.unshift(*paths)
+
+  self.helpers_path = paths
+  helper :all
+
+  def index
+    render :inline => "<%= conflicting_helper %>"
+  end
+end
+
 module LocalAbcHelper
   def a() end
   def b() end
   def c() end
 end
 
+class HelperPathsTest < ActiveSupport::TestCase
+  def setup
+    @request    = ActionController::TestRequest.new
+    @response   = ActionController::TestResponse.new
+  end
+
+  def test_helpers_paths_priority
+    request  = ActionController::TestRequest.new
+    responses = HelpersPathsController.action(:index).call(request.env)
+
+    # helpers1_pack was given as a second path, so pack1_helper should be
+    # included as the second one
+    assert_equal "pack1", responses.last.body
+  end
+end
+
 class HelperTest < ActiveSupport::TestCase
   class TestController < ActionController::Base
     attr_accessor :delegate_attr
diff --git a/actionpack/test/fixtures/helpers1_pack/pack1_helper.rb b/actionpack/test/fixtures/helpers1_pack/pack1_helper.rb
new file mode 100644
index 0000000000..9faa427736
--- /dev/null
+++ b/actionpack/test/fixtures/helpers1_pack/pack1_helper.rb
@@ -0,0 +1,5 @@
+module Pack1Helper
+  def conflicting_helper
+    "pack1"
+  end
+end
diff --git a/actionpack/test/fixtures/helpers2_pack/pack2_helper.rb b/actionpack/test/fixtures/helpers2_pack/pack2_helper.rb
new file mode 100644
index 0000000000..cf56697dfb
--- /dev/null
+++ b/actionpack/test/fixtures/helpers2_pack/pack2_helper.rb
@@ -0,0 +1,5 @@
+module Pack2Helper
+  def conflicting_helper
+    "pack2"
+  end
+end