Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mounted engines don't get route segment values #529

Closed
samuelkadolph opened this issue May 12, 2011 · 8 comments
Closed

Mounted engines don't get route segment values #529

samuelkadolph opened this issue May 12, 2011 · 8 comments
Assignees
Milestone

Comments

@samuelkadolph
Copy link
Contributor

So I've been playing with the 3.1 beta and I really want to package our code as an engine in a gem and also release a multitenant version that lets you mount the engine to /:username. This works fine but it doesn't pass the value of the segment into to controller that the route maps to inside of the engine.

Rails::Application.routes.draw do
  mount Foo::Engine => "/:username"
end

Foo::Engine.routes.draw do
  root :to => "home#home"
end

module Foo
  class HomeController < ApplicationController
    def home
      render :text => params[:username].inspect
    end
  end
end

$ curl http://localhost:3000/username
nil
@ghost ghost assigned drogus May 12, 2011
@samuelkadolph
Copy link
Contributor Author

I've found the problem and got a solution.
The cause is that Rack::Mount::RouteSet#call is called twice and does not consider that env[@parameters_key] could have been set previously (which it is and correctly contains :username => "blah" the first time).

If you merge in env[@parameters_key] to the new params, everything works as expected.

diff --git a/lib/rack/mount/route_set.rb b/lib/rack/mount/route_set.rb
index bf168be..11959c2 100644
--- a/lib/rack/mount/route_set.rb
+++ b/lib/rack/mount/route_set.rb
@@ -139,6 +139,9 @@ module Rack::Mount
       request = nil
       req = @request_class.new(env)
       recognize(req) do |route, matches, params|
+        # merge existing params
+        params.merge!(env[@parameters_key]) if env[@parameters_key]
+
         # TODO: We only want to unescape params from uri related methods
         params.each { |k, v| params[k] = Utils.unescape_uri(v) if v.is_a?(String) }

@josh, do you want me to open pull request for rack-mount to fix it?

@josh
Copy link
Contributor

josh commented May 12, 2011

Its not a simple as merging. We'd need to maintain a stack of params.

Router A -> Router B -> App 1
         -> Router C -> App 2

Its possible to have a request match A -> B -> 1 then returns X-Cascade: pass forcing A to try to the next route. In that case we'd need to clear away the nested params that were merged in B.

@samuelkadolph
Copy link
Contributor Author

Right. Do you want some help to implement it? And if so, where would be the best place to implement it? In call or recognize?

@josh
Copy link
Contributor

josh commented May 12, 2011

Please do

You should just have to modify call https://github.com/josh/rack-mount/blob/master/lib/rack/mount/route_set.rb#L149

Open a pull request and I'll walk you through the edge cases.

@samuelkadolph
Copy link
Contributor Author

Pull request created, closing this issue.

@josevalim
Copy link
Contributor

@samuelkadolph, could you also please add a test to Rails? One of the appropriated places to put this test would be in the file below. You just need to nest to route sets.

https://github.com/rails/rails/blob/master/actionpack/test/dispatch/mount_test.rb

@josevalim josevalim reopened this May 15, 2011
@arunagw
Copy link
Member

arunagw commented May 15, 2011

@josevalim I have added test here for the main problem in engine_test

e72a6f8

if we need then can add here also

@josevalim
Copy link
Contributor

Ah, no need then. Thanks @arunagw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants