This project is archived and is in readonly mode.

#1251 ✓ resolved
iain

default_url_options can't be used with named routes

Reported by iain | October 22nd, 2008 @ 09:18 PM | in 2.x

You can't use default_url_options when you are trying to fill a variable in a route in front.

so if you have map.resources :posts, :path_prefix => '/:locale' and you use post_path(@post) then @post gets filled in on the :locale spot, instead of at it's normal point. After that it breaks because there is no :id

I think the solution would be that the options should be applied before the other variables supplied to the named routes.

I've tried switching :locale with :id in default_url_options but that brings you into trouble with nested routes.

Comments and changes to this ticket

  • Karel Minařík

    Karel Minařík October 22nd, 2008 @ 10:14 PM

    +1 on this one.

    This would be wonderful solution for persistent locale passing in URLs, as:

    
    def default_url_options(options={})
      { :locale => I18n.locale }
    end
    

    would pass locale set eg. in a before_filter to every route and to params[:locale].

    Karel

  • Pratik

    Pratik November 17th, 2008 @ 02:34 PM

    • Assigned user set to “Michael Koziarski”
  • Michael Koziarski

    Michael Koziarski November 17th, 2008 @ 02:36 PM

    • State changed from “new” to “resolved”

    This is actually fixed in 2.2 already.

    In fact I have that exact same snippet of code in my app :)

  • Karel Minařík

    Karel Minařík November 17th, 2008 @ 03:00 PM

    Hi Michael,

    thanks for the update!

    You got me thrilled :)

    I tried to use it, and the one major problem with restful routes still stands, at least in my test case.

    This declaration:

    @@ <%= book_path('cz', 1) %> @@

    is obviously fine, but:

    @@ <%= book_path(1) %> @@

    gives an error:

    @@ Exception while calling 'book_path(1)': #1, :controller=>"books", :action=>"show"}, expected: {:controller=>"books", :action=>"show"}, diff: {:locale=>1}> @@

    I have put the test app on Github, because frankly I don't know how to test/demonstrate it otherwise.

    http://github.com/karmi/test_def...

    Would you be so kind and have a look what I am doing wrong?

    Thanks!

    Karel

  • Michael Koziarski

    Michael Koziarski November 17th, 2008 @ 03:03 PM

    You can't use positional arguments with default_url_options sadly, you have to use

    book_path(:id=>2)

  • Karel Minařík

    Karel Minařík November 17th, 2008 @ 03:06 PM

    Ha! That does work!

    But it's kinda incovenient to use this syntax all over the place instead of "normal" book_path(1)...

    Do you think the positional arguments can be fixed or is it something too complex?

    --karmi

  • Michael Koziarski

    Michael Koziarski November 17th, 2008 @ 03:06 PM

    This is just a side-effect of how the positional arguments work, they fill in the arguments from left to right.

    so it's passing '2' for your locale. This has been like this for a while and is pretty much an inherent limitation unfortunately.

  • Karel Minařík

    Karel Minařík November 17th, 2008 @ 03:14 PM

    OK, thanks very much for this update!

    I think something like book_path(1) is something (at least) I can live with :)

    Updated the silly test app @ Github. Probably need some mention in the Guides/docrails.

    Thanks again!

    --karmi

  • Michael Koziarski

    Michael Koziarski November 17th, 2008 @ 03:16 PM

    It's also annoying as it breaks polymorphic_url sometimes, kinda frustrating. This would be something that could be fixed in another ticket if you wanted to help.

    But the positional arguments only have two options. Fill from right to left, or left to right.

    Using right to left would fix this problem, but would break people with values like locale at the right.

  • José Valim

    José Valim November 17th, 2008 @ 05:22 PM

    I attached a diff (not a patch yet) that allows us to do:

    map.user ':locale/user/:id'

    And then:

    I18n.locale = 'en-US' user_url(1) #=> http://host/en-US/user/1

    I18n.locale = 'pt-BR' user_url(1) #=> http://host/pt-BR/user/1

    I18n.locale = 'pt-BR' user_url('en-US', 1) #=> http://host/en-US/user/1

    It just adds some extra code to named routes helpers if those routes have a :locale as segment (very straightforward).

    Maybe it can be a start point to work in a solution that doesn't require us to write:

    user_url(:id => 1)

    Currently everything happens automagically, but we could use defaults key:

    map ':locale/user/:id', :defaults => { :locale => 'pt-BR' }

    So if it has a default value, it's not always required as positional argument. Finally, that wouldn't solve all our problems, so we could add proc handling to defaults:

    map ':locale/user/:id', :defaults => { :locale => proc{ I18n.locale } }

    Of course, just brainstorming. =)

  • Michael Koziarski

    Michael Koziarski November 17th, 2008 @ 06:56 PM

    Right, this works for :locale, but not for anything else.

    We can't fix this with a special case that only suits the people with this one particular problem. after all, people can use Accept-Language or cookies for locales too :)

  • Karel Minařík

    Karel Minařík November 17th, 2008 @ 07:29 PM

    Right, this works for :locale, but not for anything else.

    Yes, that is the problem with Josés solution. :locale isn't something which should be hardcoded in Rails codebase.

    people can use Accept-Language or cookies for locales too

    No no no no no, please don't mention that again :)

    As extensively debated, cookies/sessions are the worst storage for locale. The resource ("page") should display the same content for everyone, no matter if (s)he has a cookie or not.

    (And Accept-Language is only slightly better. It may work for "application" type of app, eg. Lighthouse, but not for a content-oriented app, such as a magazine, website, etc.)

  • José Valim

    José Valim November 17th, 2008 @ 07:41 PM

    Exactly, my solution is hardcoded but I would gladly implement something.

    We just need to define a syntax, for example:

    map.user ':locale/user/:id', :optional => :locale

    Or:

    map.page '/page/:page/:id', :optional => :page

    Then we could get the :locale or :page from default_url_options.

    What do you think?

  • iain

    iain January 10th, 2009 @ 01:34 AM

    • Tag changed from 2.2, l10n, routing, url_for to 2.3, l10n, routing, url_for

    I might have found the solution, tell me what you think: the keys provided by default_url_options are not filled by the options provided in the url_helper. Have a look at my patch, and tell me what you think.

    Sorry for it to take so long. Had to wait for this commit to be able to find the right eval ;)

  • José Valim

    José Valim January 10th, 2009 @ 08:29 AM

    It looks like it will brake some things because you are always removing the route segments. Example:

    
    class UsersController
      def default_url_options
        { :locale => 'en' }
      end
    
      def show
        # ...
        redirect_to user_url('en', 1)
      end
    

    Most application are doing like that right?

    But you are always removing the locale segment and then when you call zip in the route generation, 'en' will match the id segment and 1 will match nothing.

    A solution could be:

    
    if route_segments.size > args.size && respond_to?(:default_url_options) && default_url_options.respond_to?(:symbolize_keys)
      route_segments -= default_url_options.symbolize_keys.keys
    end
    

    Doing "route_segments.size > args.size" you will just remove the segment when not enough args are sent.

    Finally, I'm not sure if your implementation will work on views and helpers, because default_url_options method is not defined there. Or you will have to ask it for the controller or you will have to define default_url_options there.

  • iain

    iain January 10th, 2009 @ 01:30 PM

    Most application are doing like that right?

    You wouldn't need to, because it is already specified in the default_url_options. It's a choice: either specify it in the default_url_options, or specify it namelessly in the user_url. Besides, you can still override it, using user_url(1, :locale => 'fr'), when you're changing locales for instance. Doing user_url('en', 1) isn't very DRY, when needing to specify the locale every time you call a URL. I doubt many applications will go through the trouble of changing their URLs everywhere, and I'm sure everyone wants to have to have clean method_calls.

    Doing "route_segments.size > args.size" you will just remove the segment when not enough args are sent.

    In Rails 2.3, there are almost always more route_segments than args, because the format is a route_segment. formatted_user_url is depricated and one should do: user_url(1, :format => :js). Every route has the :format segment, which rarely gets used (only with non-html formats). Checking for route_segments.size would be useless.

    Finally, I'm not sure if your implementation will work on views and helpers.

    That is a big concern, that I've forgot. I'll look into it.

  • José Valim

    José Valim January 10th, 2009 @ 01:51 PM

    My point about "all applications are doing like that" is because your current implementation will break what our application are doing now, it breaks api.

    For example, in the moment I define default_url_options in my controller with :locale, all route methods that I'm passing the locale as parameter will break.

    Oh, and I totally forgot that Rails 2.3 changed to have :format as an optional parameter, great point.

  • iain

    iain January 10th, 2009 @ 04:06 PM

    I adjusted the patch a bit, to work in views too.

    Furthermore, I pass the options passed to user_url to default_url_options, so you can do something with that. The code in the eval gets a bit out of hand. It's now long and big. Blegh.

    Your problem didn't make any sense to me, but when I finally worked out a more sensible example, I finally began to see the problem ;)

    
    map.foo 'my_foo/:foo/:bar', :controller => 'users', :action => 'do_foo'
    
    def default_url_options(options = {})
      { :foo => 'bang' }
    end
    
    foo_url(foo, bar)
    

    This would raise an error, because foo would fill :bar and bar would be too much. But with the :format option in routes, I cannot check for it anymore. It's either working for custom routes, or for resources. If format is such a special case, I could leave it out of the equation:

    
    if (route_segments - [:format]).size > args.size .....
    

    Don't know how you feel about that though.

    I have included a new patch, including the changes I mentioned above.

  • José Valim

    José Valim January 10th, 2009 @ 05:26 PM

    Great, but why don't you try to use defined?. It would simplify things a little bit:

    
    default_segments = if defined?(default_url_options)
      default_url_options
    elsif defined?(controller.default_url_options)
      controller.default_url_options
    else
      nil
    end
    
    if default_segments.is_a?(Hash)
      route_segments -= default_segments.symbolize_keys.keys
    end
    

    But I'm not sure if it won't break anything. I've already tried to solve this ticket and my first attempt was to make :locale (and only :locale) as optional (you can see the patch in this thread). It was not accepted because it's a very specific approach.

    Then when I attempted to make it "generic", as the one you are doing now, I couldn't get it done quickly, I ran out of time. Our biggest problem is that we are trying to guess which parameters are optional and guessing at this point could be dangerous.

    Maybe the best solution would be to specify (not guess) that a segment is optional by assigning a default value. Rails has such API:

    
      map.connect ':controller/:action/:id', :path_prefix => ':locale', :defaults => { :locale => 'en' }
    

    Then we just think that we have to overwrite :locale using default_url_options and everytihg will be solved. But we face some problems:

    1. The hash defined in default_url_options will NOT overwrite the one specified in :defaults.

    2. The segment specified as default cannot be followed by a dynamic segment. This means that we can't do this:

    
      map.connect ':locale/users/:username', :path_prefix => ':locale', :defaults => { :locale => 'en' }
    
    1. map.resources does not accept :defaults option (as long as I tested it, it was ignored)

    However, I cannot say exactly how many work is needed to solve those problems.

    When Rails :format was made optional, it was just created a new class for it and they didn't had to handle such scenarios.

    That said, maybe you could also create a new class to handle :locale segment. It would probably take some time before it gets into core (if it does) but until there it could be used as plugin. :)

    Sorry for the long reply, I'm just trying to share what I discovered so far about this issue. :)

  • iain

    iain January 12th, 2009 @ 08:42 PM

    Great, but why don't you try to use defined?

    Breaks too much tests... (although I might be able to clean it up a bit)

    Sorry for the long reply

    Fantastic :-)

    I have been mulling it over, and what if the options passed to user_url() are passed to default_url_options as well? This way you can exclude it when needed.

    
    def default_url_options(method, *args)
      options = args.extract_options!
      if method == :user and args.size == 2
        {}
      else
        { :locale => I18n.locale }
      end
    end
    

    This way you can keep your way of doing it, with changing it in one special location, and people start new projects can start it anew, without the check. I can even check if you use this message by checking if you use *args (i.e. a negative arity).

    So, as long as you don't use the splat-operator, no args are removed, if you do, you opt-in for the new way of doing things: every key you specify in default_url_options can only be overridden when it is specified specifically in the user_url method.

    This is an API change though, but a sane one I think.

  • xdmx

    xdmx February 16th, 2009 @ 02:26 PM

    will this be included in the upcoming rails 2.3 or will we wait until the 3.0 or some 2.3.x?

  • Brendan Baldwin

    Brendan Baldwin February 18th, 2009 @ 12:53 AM

    Wonder if maybe we could extend the routes DSL to support exclusion of some route segment params from the list of positional arguments altogether?

  • Brendan Baldwin

    Brendan Baldwin February 18th, 2009 @ 01:03 AM

    Could do something like:

    map.with_options :prefix => ':locale',
                     :skip_arguments => [:locale] do |loc|
      loc.resources :things
    end
    
    

    Or force/rework the positional arguments order:

    map.with_options :prefix => ':locale' do |loc|
      loc.resources :things, :param_order => [:id]
    end
    
    

    In both of these cases, we would still use default_url_options to patch in the locale default, but methods like this would still work:

    thing_url(123)
    
    
  • Kunal Shah

    Kunal Shah June 10th, 2009 @ 04:44 PM

    I'm using default_url_options to persist a query_parameter throughout all actions and views of a particular controller when that query parameter is initially present. Just so you understand my use case if the parameter is present a popup layout gets rendered instead of the full application layout and some other magic occurs.

    I've had no problem using this EXCEPT for the new resource path named route as in:

    map.resources :posts

    new_post_path => does not retain the default url options

    Any updates on this?

  • machen

Create your profile

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

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Pages