Ruby on Rails

Comments and changes to this ticket

• 

  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 April 27th, 2008 @ 12:44 PM

  Attaching a patch instead - gonna delete my branch.

  • default_url_options_named_routes.patch 4.6 KB
• 

  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 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 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 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 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 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 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 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 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.

  • fix_default_url_options_regression.diff 3.8 KB
• 

  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 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)

  • ticket22_bug.diff 782 Bytes
• 

  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 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 May 4th, 2008 @ 08:22 PM

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

• 

  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 May 4th, 2008 @ 10:52 PM

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

  I'll get to this today.

• 

  rick May 6th, 2008 @ 07:43 AM

  Too slow! I got it...

• 

  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 July 18th, 2008 @ 01:13 AM

  • → Tag changed from “” 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 July 18th, 2008 @ 05:55 PM

  • → Tag changed from “actionpack patch routing” to “actionpack routing”
  • → State changed from “resolved” to “open”
• 

  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.