Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

url_for ignores routing priority

#2510

It seems that 'url_for' sometimes ignores routing priority.

I'm using


$ ruby --version
ruby 1.8.6 (2008-08-11 patchlevel 287) [i686-linux]
$ rails --version
Rails 2.1.1

My application is


$ rails myapp
$ cd myapp
$ ./script/generate controller foo/qix index
$ # edit routes.rb and foo/qix_controller.rb

routes.rb


ActionController::Routing::Routes.draw do |map|
  map.connect 'qix/:action/:file.:extname', { :controller => 'foo/qix' }
  map.connect 'qix/:action/:id', { :controller => 'foo/qix' }
  map.connect ':controller/:action/:id.:format'
  map.connect ':controller/:action/:id'
end

foo/qix_controller.rb


class Foo::QixController < ApplicationController
  def index
    logger.debug { url_for :id => 3 }
  end
end

I think this 'url_for' should return "http://localhost:3000/qix/index/3". But, If I access "http://localhost:3000/qix/index/2", it returns "http://localhost:3000/foo/qix/index/3".

Method 'RouteSet#routes_for_controller_and_action_and_keys' returns unstable-sorted result. When I overwrite the method with not sorting version, 'url_for' seems working fine.


module ActionController
  module Routing
    class RouteSet #:nodoc:
      def routes_for_controller_and_action_and_keys(controller, action, keys)
        selected = routes.select do |route|
          route.matches_controller_and_action? controller, action
        end
        selected
      end
    end
  end
end

Reported by kik · April 16th, 2009 @ 11:55 AM

State: invalid
Milestone: 2.3.4
Assigned to: josh josh
Importance: Medium

Activity

  1. Colin MacKenzie IV
    Colin MacKenzie IV

    This issue appears to also be present in Rails 2.3.2 / Ruby 1.8.6 WinXP.

    May 8th, 2009 @ 12:57 PM

  2. Colin MacKenzie IV
    Colin MacKenzie IV

    This seems to have solved the problem for me.

    May 8th, 2009 @ 01:24 PM

  3. Colin MacKenzie IV
    Colin MacKenzie IV

    Erg, dumb error in previous code. That's what I get for trying to make things pretty. :) New attachment:

    May 8th, 2009 @ 01:30 PM

  4. CancelProfileIsBroken
  5. Gabe da Silveira
    Gabe da Silveira

    verified the bug on 2-3-stable.

    The problem is that the prioritization algorithm for routes, which depends on the number of matching components, does not preserve the order for routes of equal weight. I couldn't find an obvious place to test this, and frankly I don't care enough to put any more time into it, but I have a simple solution that makes the algorithm more robust and doesn't break any existing tests.

    I've attached a patch.

    August 9th, 2009 @ 12:01 PM

  6. Rizwan Reza
    Rizwan Reza

    verified

    +1 This patch applies cleanly to master as well.

    August 9th, 2009 @ 03:30 PM

  7. Tristan Dunn
    Tristan Dunn
    • Tag changed from bugmash to bugmash, verified

    +1

    Verified it applies cleanly to master and 2-3-stable with passing tests.

    August 9th, 2009 @ 10:15 PM

  8. Elad Meidar
    Elad Meidar

    +1 verified, applied and successfully tests (manual as well) on master and 2-3-stable

    August 10th, 2009 @ 12:55 AM

  9. Dan Croak
    Dan Croak

    +1 verified Gabe's patch applies to 2-3-stable and actionpack rake test runs green.

    August 10th, 2009 @ 02:12 AM

  10. josh
    josh
    • Assigned user set to josh
    • State changed from new to invalid
    • Tag cleared.
    • Milestone changed from 2.x to 2.3.4

    We are killing this sorting "feature" in 3.0. I'm working on a deprecation for 2.3 that will return the routes in the original priority ordering.

    August 10th, 2009 @ 02:17 AM

  11. Gabe da Silveira
    Gabe da Silveira

    I don't understand why you wouldn't just apply this in the meantime since it fixes an legitimate issue. It's not really invalid.

    August 10th, 2009 @ 04:46 AM

  12. josh
  13. Gabe da Silveira
    Gabe da Silveira

    So you deprecated it, but A) my patch would have still cleanly applied and B) the original issue still exists. At least it should be wontfix.

    August 10th, 2009 @ 04:27 PM