This project is archived and is in readonly mode.

#22 ✓stale
Deepak Jois

default_url_options is being ignored by named route optimisation

Reported by Deepak Jois | April 19th, 2008 @ 08:31 AM | in 2.1.1

This is basically the same as

http://dev.rubyonrails.org/ticke...

It seems that the reeason it happens is that the optimisation code for named routes uses the request.host_with_string and request.protocol without calling default_url_option at any time (as in the url_for method)

Comments and changes to this ticket

  • Cheah Chu Yeow

    Cheah Chu Yeow April 20th, 2008 @ 06:02 AM

    Here's a patch that checks for the existence of a non-blank #default_url_options and bypasses named route optimization: http://github.com/chuyeow/rails/...

  • Cheah Chu Yeow

    Cheah Chu Yeow April 27th, 2008 @ 12:44 PM

    Attaching a patch instead - gonna delete my branch.

  • Ian White

    Ian White May 1st, 2008 @ 08:36 AM

    +1 applies cleanly on 6f20efd (Wed Apr 30 23:30:50 2008 -0500)

    I've been bitten by this one, thanks for the patch

  • Daniel Guettler

    Daniel Guettler May 1st, 2008 @ 04:07 PM

    +1 looks good, better than my local solution to the referenced ticket :) Patch applied fine to 74436d (Thu May 1 10:21:46 2008 +0100) after removing the empty line changes in action_controller/base.rb

  • Repository

    Repository May 4th, 2008 @ 01:50 AM

    • State changed from “new” to “resolved”

    (from [6a6b4392c16c665eb713705f2b38e959a658eeef]) Ensure that default_url_options, if defined, are used in named routes.

    Signed-off-by: Michael Koziarski

    [#22 state:resolved]

    http://github.com/rails/rails/co...

  • Michael Koziarski

    Michael Koziarski May 4th, 2008 @ 01:51 AM

    For the next major release we will probably deprecate / remove default_url_options. But in the meantime nice patch.

  • Daniel Guettler

    Daniel Guettler May 4th, 2008 @ 02:04 AM

    Hmm, pity to see it go. I found it useful in cases where you want to add some parameters to the urls based on if some methods on a controller return a value or return nil.

    E.g. an url may need a section_id to identify the area it's accessed from, but this is not needed always and can change dynamically. In this case you can add { :section_id => SomeController.section_id } to the default_url_options and don't have to worry about it anymore in each generated url.

    Is there are better way to archive this?

    Daniel

  • Deepak Jois

    Deepak Jois May 4th, 2008 @ 05:33 AM

    Thanks for applying the patch.

    I agree with Daniel. I found the default_url_options quite useful for my app where I had to redirect from a subdomain to the main domain name for some pages. Without default_url_options, I would have to insert a hash with hostname and port into every url call.

    It would be better to fix the optimisation of routes to take into account values from the default_url_options hash.

  • Michael Koziarski

    Michael Koziarski May 4th, 2008 @ 05:41 AM

    We definitely need something else which is like default_url_options, which is why we haven't removed it yet :).

  • Ian White

    Ian White May 4th, 2008 @ 04:27 PM

    Since this commit, all of my named_routes are failing with

    ArgumentError: wrong number of arguments (1 for 0)
            from (eval):2:in `default_url_options'
    

    Example: (On a clean rails app, with rails at 6a6b439)

      # (in routes.rb)
      map.resources :things
    
    $ script/console
    
    >> include ActionController::UrlWriter
    => Object
    >> things_path
    ArgumentError: wrong number of arguments (1 for 0)
            from (eval):2:in `default_url_options'
            from (eval):2:in `things_path'
            from (irb):3
    

    I'm investigating a patch

  • Cheah Chu Yeow

    Cheah Chu Yeow May 4th, 2008 @ 04:59 PM

    Ack I see why that's a problem (and why it wasn't covered by existing tests nor my test in the applied patch) - default_url_options takes a single argument in AC::B but is simply a (hash) attribute in ActionController::UrlWriter.

    Ian thanks for spotting this!

    I'm attaching a patch that is a quick fix for this. Basically it gives AC::B#default_url_options a default argument. I didn't want to remove it since I imagine that would involve deprecation.

  • Cheah Chu Yeow

    Cheah Chu Yeow May 4th, 2008 @ 05:00 PM

    Btw, now I'm more convinced that default_url_options does need some cleaning up/re-thinking!

  • Ian White

    Ian White May 4th, 2008 @ 05:23 PM

    Here's a testcase that fails on 6a6b43 but passes on the previous commit 437f918

    (you have to paste the test into 437f918- the diff doesn't apply)

  • Ian White

    Ian White May 4th, 2008 @ 05:25 PM

    No worries, uploaded my test patch without seeing Cheah Chu Yeow's updates updates on this ticket, you can just ignore it

  • Ian White

    Ian White May 4th, 2008 @ 08:20 PM

    Does this need to re-opened as a new ticket? I notice that it's status is 'resolved'.

  • Ian White

    Ian White May 4th, 2008 @ 08:22 PM

    apostrophe man commin to get me... I wish I could edit lighthouse comments

  • Ian White

    Ian White May 4th, 2008 @ 08:24 PM

    Actually I see that Cheah Chu Yeow's patch doesn't include a regression test, so it might be useful to use the diff I attached for that.

  • Michael Koziarski

    Michael Koziarski May 4th, 2008 @ 10:52 PM

    • Milestone set to 2.1.1
    • State changed from “resolved” to “open”
    • Assigned user set to “Michael Koziarski”

    I'll get to this today.

  • Rick

    Rick May 6th, 2008 @ 07:43 AM

    Too slow! I got it...

  • Repository

    Repository May 6th, 2008 @ 07:46 AM

    • State changed from “open” to “resolved”

    (from [37599d16f2374179ebf001aeb79ff121e3d67519]) regression test for bug introduced in [6a6b4392c16c665eb713705f2b38e959a658eeef] [Ian White] [#22 state:resolved]

    http://github.com/rails/rails/co...

  • Phil Orwig

    Phil Orwig July 18th, 2008 @ 01:13 AM

    • Tag set to actionpack, patch, routing

    This problem still arises for named routes used in views instead of controllers. I have default_url_options defined in app/controllers/application.rb; routes that are generated from the controller seem to be fine, whereas routes generated from the views do not.

    Adding the following to app/helpers/application_helper.rb works around the issue:

    def default_url_options(options = nil)
        @controller.send(:default_url_options, options)
    end
    
  • Pratik

    Pratik July 18th, 2008 @ 05:55 PM

    • State changed from “resolved” to “open”
    • Tag changed from actionpack, patch, routing to actionpack, routing
  • Jeremy Kemper

    Jeremy Kemper August 30th, 2008 @ 02:18 AM

    • State changed from “open” to “stale”

    This looks like a simple fix, but it's growing stale here. Please write a test and submit a patch.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages