Skip to content
This repository
Browse code

Fix handling SCRIPT_NAME from within mounted engine's

When you mount your application at a path, for example /myapp, server
should set SCRIPT_NAME to /myapp. With such information, rails
application knows that it's mounted at /myapp path and it should generate
routes relative to that path.

Before this patch, rails handled SCRIPT_NAME correctly only for regular
apps, but it failed to do it for mounted engines. The solution was to
hardcode default_url_options[:script_name], which is not the best answer
- it will work only when application is mounted at a fixed path.

This patch fixes the situation by respecting original value of
SCRIPT_NAME when generating application's routes from engine and the
other way round - when you generate engine's routes from application.

This is done by using one of 2 pieces of information in env - current
SCRIPT_NAME or SCRIPT_NAME for a corresponding router. This is because
we have 2 cases to handle:

- generating engine's route from application: in this situation
  SCRIPT_NAME is basically SCRIPT_NAME set by the server and it
  indicates the place where application is mounted, so we can just pass
  it as :original_script_name in url_options. :original_script_name is
  used because if we use :script_name, router will ignore generating
  prefix for engine

- generating application's route from engine: in this situation we
  already lost information about the SCRIPT_NAME that server used. For
  example if application is mounted at /myapp and engine is mounted at
  /blog, at this point SCRIPT_NAME is equal /myapp/blog. Because of that
  we need to keep reference to /myapp SCRIPT_NAME by binding it to the
  current router. Later on we can extract it and use when generating url

Please note that starting from now you *should not* use
default_url_options[:script_name] explicitly if your server already
passes correct SCRIPT_NAME to rack env.

(closes #6933)
  • Loading branch information...
commit 5b3bb61f3fb82c7300d4dac374fe7aeafff6bda0 1 parent f255711
Piotr Sarnacki authored August 10, 2012
10  actionpack/lib/action_controller/metal/url_for.rb
@@ -30,9 +30,15 @@ def url_options
30 30
         :_recall => request.symbolized_path_parameters
31 31
       ).freeze
32 32
 
33  
-      if _routes.equal?(env["action_dispatch.routes"])
  33
+      if (same_origin = _routes.equal?(env["action_dispatch.routes"])) ||
  34
+         (script_name = env["ROUTES_#{_routes.object_id}_SCRIPT_NAME"]) ||
  35
+         (original_script_name = env['SCRIPT_NAME'])
34 36
         @_url_options.dup.tap do |options|
35  
-          options[:script_name] = request.script_name.dup
  37
+          if original_script_name
  38
+            options[:original_script_name] = original_script_name
  39
+          else
  40
+            options[:script_name] = same_origin ? request.script_name.dup : script_name
  41
+          end
36 42
           options.freeze
37 43
         end
38 44
       else
17  actionpack/lib/action_dispatch/routing/route_set.rb
@@ -163,9 +163,9 @@ def length
163 163
         private
164 164
 
165 165
           def define_named_route_methods(name, route)
166  
-            define_url_helper route, :"#{name}_path", 
  166
+            define_url_helper route, :"#{name}_path",
167 167
               route.defaults.merge(:use_route => name, :only_path => true)
168  
-            define_url_helper route, :"#{name}_url", 
  168
+            define_url_helper route, :"#{name}_url",
169 169
               route.defaults.merge(:use_route => name, :only_path => false)
170 170
           end
171 171
 
@@ -465,7 +465,7 @@ def current_controller
465 465
         def use_recall_for(key)
466 466
           if @recall[key] && (!@options.key?(key) || @options[key] == @recall[key])
467 467
             if !named_route_exists? || segment_keys.include?(key)
468  
-              @options[key] = @recall.delete(key) 
  468
+              @options[key] = @recall.delete(key)
469 469
             end
470 470
           end
471 471
         end
@@ -574,7 +574,8 @@ def generate(options, recall = {}, extras = false)
574 574
       end
575 575
 
576 576
       RESERVED_OPTIONS = [:host, :protocol, :port, :subdomain, :domain, :tld_length,
577  
-                          :trailing_slash, :anchor, :params, :only_path, :script_name]
  577
+                          :trailing_slash, :anchor, :params, :only_path, :script_name,
  578
+                          :original_script_name]
578 579
 
579 580
       def mounted?
580 581
         false
@@ -594,7 +595,13 @@ def url_for(options)
594 595
 
595 596
         user, password = extract_authentication(options)
596 597
         recall  = options.delete(:_recall)
597  
-        script_name    = options.delete(:script_name).presence || _generate_prefix(options)
  598
+
  599
+        original_script_name = options.delete(:original_script_name).presence
  600
+        script_name = options.delete(:script_name).presence || _generate_prefix(options)
  601
+
  602
+        if script_name && original_script_name
  603
+          script_name = original_script_name + script_name
  604
+        end
598 605
 
599 606
         path_options = options.except(*RESERVED_OPTIONS)
600 607
         path_options = yield(path_options) if block_given?
18  actionpack/test/dispatch/prefix_generation_test.rb
@@ -166,18 +166,6 @@ def setup
166 166
       assert_equal "/generate", last_response.body
167 167
     end
168 168
 
169  
-    test "[ENGINE] generating application's url includes default_url_options[:script_name]" do
170  
-      RailsApplication.routes.default_url_options = {:script_name => "/something"}
171  
-      get "/pure-awesomeness/blog/url_to_application"
172  
-      assert_equal "/something/generate", last_response.body
173  
-    end
174  
-
175  
-    test "[ENGINE] generating application's url should give higher priority to default_url_options[:script_name]" do
176  
-      RailsApplication.routes.default_url_options = {:script_name => "/something"}
177  
-      get "/pure-awesomeness/blog/url_to_application", {}, 'SCRIPT_NAME' => '/foo'
178  
-      assert_equal "/something/generate", last_response.body
179  
-    end
180  
-
181 169
     test "[ENGINE] generating engine's url with polymorphic path" do
182 170
       get "/pure-awesomeness/blog/polymorphic_path_for_engine"
183 171
       assert_equal "/pure-awesomeness/blog/posts/1", last_response.body
@@ -200,12 +188,6 @@ def setup
200 188
       assert_equal "/something/awesome/blog/posts/1", last_response.body
201 189
     end
202 190
 
203  
-    test "[APP] generating engine's route should give higher priority to default_url_options[:script_name]" do
204  
-      RailsApplication.routes.default_url_options = {:script_name => "/something"}
205  
-      get "/generate", {}, 'SCRIPT_NAME' => "/foo"
206  
-      assert_equal "/something/awesome/blog/posts/1", last_response.body
207  
-    end
208  
-
209 191
     test "[APP] generating engine's url with polymorphic path" do
210 192
       get "/polymorphic_path_for_engine"
211 193
       assert_equal "/awesome/blog/posts/1", last_response.body
5  railties/CHANGELOG.md
Source Rendered
... ...
@@ -1,5 +1,10 @@
1 1
 ## Rails 4.0.0 (unreleased) ##
2 2
 
  3
+*   Correctly handle SCRIPT_NAME when generating routes to engine in application
  4
+    that's mounted at a sub-uri. With this behavior, you *should not* use
  5
+    default_url_options[:script_name] to set proper application's mount point by
  6
+    yourself. *Piotr Sarnacki*
  7
+
3 8
 *   The migration generator will now produce AddXXXToYYY/RemoveXXXFromYYY migrations with references statements, for instance
4 9
 
5 10
         rails g migration AddReferencesToProducts user:references supplier:references{polymorphic}
6  railties/lib/rails/engine.rb
@@ -494,7 +494,11 @@ def endpoint
494 494
 
495 495
     # Define the Rack API for this engine.
496 496
     def call(env)
497  
-      app.call(env.merge!(env_config))
  497
+      env.merge!(env_config)
  498
+      if env['SCRIPT_NAME']
  499
+        env.merge! "ROUTES_#{routes.object_id}_SCRIPT_NAME" => env['SCRIPT_NAME'].dup
  500
+      end
  501
+      app.call(env)
498 502
     end
499 503
 
500 504
     # Defines additional Rack env configuration that is added on each call.
48  railties/test/railties/engine_test.rb
@@ -1193,6 +1193,54 @@ def index
1193 1193
                    last_response.body.split("\n").map(&:strip)
1194 1194
     end
1195 1195
 
  1196
+    test "paths are properly generated when application is mounted at sub-path" do
  1197
+      @plugin.write "lib/bukkits.rb", <<-RUBY
  1198
+        module Bukkits
  1199
+          class Engine < ::Rails::Engine
  1200
+            isolate_namespace Bukkits
  1201
+          end
  1202
+        end
  1203
+      RUBY
  1204
+
  1205
+      app_file "app/controllers/bar_controller.rb", <<-RUBY
  1206
+        class BarController < ApplicationController
  1207
+          def index
  1208
+            render :text => bukkits.bukkit_path
  1209
+          end
  1210
+        end
  1211
+      RUBY
  1212
+
  1213
+      app_file "config/routes.rb", <<-RUBY
  1214
+        AppTemplate::Application.routes.draw do
  1215
+          get '/bar' => 'bar#index', :as => 'bar'
  1216
+          mount Bukkits::Engine => "/bukkits", :as => "bukkits"
  1217
+        end
  1218
+      RUBY
  1219
+
  1220
+      @plugin.write "config/routes.rb", <<-RUBY
  1221
+        Bukkits::Engine.routes.draw do
  1222
+          get '/bukkit' => 'bukkit#index'
  1223
+        end
  1224
+      RUBY
  1225
+
  1226
+
  1227
+      @plugin.write "app/controllers/bukkits/bukkit_controller.rb", <<-RUBY
  1228
+        class Bukkits::BukkitController < ActionController::Base
  1229
+          def index
  1230
+            render :text => main_app.bar_path
  1231
+          end
  1232
+        end
  1233
+      RUBY
  1234
+
  1235
+      boot_rails
  1236
+
  1237
+      get("/bukkits/bukkit", {}, {'SCRIPT_NAME' => '/foo'})
  1238
+      assert_equal '/foo/bar', last_response.body
  1239
+
  1240
+      get("/bar", {}, {'SCRIPT_NAME' => '/foo'})
  1241
+      assert_equal '/foo/bukkits/bukkit', last_response.body
  1242
+    end
  1243
+
1196 1244
   private
1197 1245
     def app
1198 1246
       Rails.application
15  railties/test/railties/mounted_engine_test.rb
@@ -163,24 +163,14 @@ def app
163 163
       end
164 164
     end
165 165
 
166  
-    def reset_script_name!
167  
-      Rails.application.routes.default_url_options = {}
168  
-    end
169  
-
170  
-    def script_name(script_name)
171  
-      Rails.application.routes.default_url_options = {:script_name => script_name}
172  
-    end
173  
-
174 166
     test "routes generation in engine and application" do
175 167
       # test generating engine's route from engine
176 168
       get "/john/blog/posts"
177 169
       assert_equal "/john/blog/posts/1", last_response.body
178 170
 
179 171
       # test generating engine's route from engine with default_url_options
180  
-      script_name "/foo"
181 172
       get "/john/blog/posts", {}, 'SCRIPT_NAME' => "/foo"
182 173
       assert_equal "/foo/john/blog/posts/1", last_response.body
183  
-      reset_script_name!
184 174
 
185 175
       # test generating engine's route from application
186 176
       get "/engine_route"
@@ -193,14 +183,11 @@ def script_name(script_name)
193 183
       assert_equal "/john/blog/posts", last_response.body
194 184
 
195 185
       # test generating engine's route from application with default_url_options
196  
-      script_name "/foo"
197 186
       get "/engine_route", {}, 'SCRIPT_NAME' => "/foo"
198 187
       assert_equal "/foo/anonymous/blog/posts", last_response.body
199 188
 
200  
-      script_name "/foo"
201 189
       get "/url_for_engine_route", {}, 'SCRIPT_NAME' => "/foo"
202 190
       assert_equal "/foo/john/blog/posts", last_response.body
203  
-      reset_script_name!
204 191
 
205 192
       # test generating application's route from engine
206 193
       get "/someone/blog/generate_application_route"
@@ -210,10 +197,8 @@ def script_name(script_name)
210 197
       assert_equal "/", last_response.body
211 198
 
212 199
       # test generating application's route from engine with default_url_options
213  
-      script_name "/foo"
214 200
       get "/someone/blog/generate_application_route", {}, 'SCRIPT_NAME' => '/foo'
215 201
       assert_equal "/foo/", last_response.body
216  
-      reset_script_name!
217 202
 
218 203
       # test polymorphic routes
219 204
       get "/polymorphic_route"

1 note on commit 5b3bb61

Carlos Antonio da Silva

And that's how to write a commit message :clap:

Please sign in to comment.
Something went wrong with that request. Please try again.