[PATCH] replace outer_action_inner_path with action_outer_inner_path for named routes with nested resources

I don't think there has been a public release with the (broken) old behavior yet so i see no need to keep any kind of backwards compatibility. Very nice patch. I'll commit it once i get a consensus on weather or not to keep the backwards compatibility.

Great - I hadn't considered that it wasn't released yet. Thanks for looking into this.

Cheers, David

Hi Tobi - is this still on the radar?

Thanks, David

I have been meaning to merge this for a while now. The only work which remains to be done is to provide deprecation warnings when accessing the old methods. Is this something you can add to the patch? I can merge it right away then.

I'd be happy to, but I'm not sure the best way to do that. The patch as it is changes the named routes that get generated, but does not deal in any way with their access. To fire deprecations when accessing them, we'd have to add an option to their generation to indicate that they are deprecated and then intercept calls to NamedRouteCollection#get to see if a deprecated name is being used. I don't see a better way, but that strikes me as pretty invasive.

WDYT? I'm glad to do the work, but a bit of advice would be most appreciated.

Also - you indicated earlier in this thread that deprecating might not be necessary. Any chance that could be revisited? Just yank the old since it's not been released?

Thx, David

If we verify that the old broken behaviour was never in a public release, then I definitely think it makes sense to make the change.

I have a funny feeling it was in 1.2 though...

Sadly, the old names have been released. I don't know how far back they first came out, but they are in 1.2.3. I'll modify the patch to add deprecation warnings.

OK, here it is, deprecation warnings and all. The only problem I have is I can't figure out how to turn the deprecation warnings off when you run the tests. :( This does give you the benefit that you'll get to see them and make sure they make sense. But I do have to ask you to either turn them off or teach me how.

Thanks, David

You can wrap in an assert_deprecated(/message/) { ... } block to suppress the message while asserting that it exists.

improved names with deprecation warnings with assert_deprecated

Cool. Done. New patch (improve_prefixed_named_routes.patch) attached. Good?

Looks good to me. Tobi?

Should apply to both stable and trunk, but remove the deprecated stuff from trunk.

Would a separate patch with the deprecated stuff yanked help?

(In [7138]) Prefix nested resource named routes with their action name, e.g. new_group_user_path(@group) instead of group_new_user_path(@group). The old nested action named route is deprecated in Rails 1.2.4. Closes #8558.

A backport to branches/1-2-stable would be great. I started on it but nearly everything conflicts since the resource path conventions clash.

I'm a bit confused about the lay of the land here (would you think it a trademark infringement if I changed my nic to bitconfused?). Is 1-2-stable the real pre-cursor to 2.0?

Oh - I see - that's still got the old /resource;action convention.

What's the goal here? To get the new named routes to work with the old path conventions?

Trunk is the 2.0 precursor; 1-2-stable is the 1.2.x line.

The goal is to deprecate group_new_user_path in favor of new new_group_user_path in the 1.2.4 release (branches/1-2-stable) so it can be removed entirely from 2.0 (trunk).

Is the plan to have the new conventions in 1.2.4? If so, I can put together a patch that tackles both issues. Might be a few days or a week before I can get it done, but I'm happy to help.

Yes, the new conventions should be in 1.2.4, with the old ones generating deprecation warnings. That way people get warned about the pending change in 2.0, and can do something about it in their application.

I started to look at this, but 1-2-stable is pretty far behind edge - there's no uses_mocha, for example (which many of the tests in my original patch use). Is there actually a plan to release 1.2.4 before 2.0?

We definitely intend to release 1.2.4 before 2.0, it contains a number of valuable fixes, and more deprecation warnings.

If the tests have to be implemented differently, that's not such a big deal. You could also investigate backporting the mocha stuff.

The differences between the lay of the land in trunk and stable are extraordinary. I don't see a pragmatic way to approach this. I've tried applying the patch and following the errors and that leads down an endless trail of stuff that's missing. I tried simply moving over the relevant files and that leads down a similar trail.

How do you guys deal w/ this normally? And why are these branches not maintained together?

They're not kept in sync because stable-1-2 is the release branch and trunk contains a *lot* of changes not intended for 1.2.4, if it's impossible to backport the deprecations and changes, then we can't really push the change out in 2.0 and will have to revert it in trunk.

Jump on #rails-contrib and ask for a hand, there are probably some people there who can help.

This would be a mistake in this case because the behavior in trunk is a bug. We are essentially dealing with a bug fix here. Instead we can just leave the patch as is for rails 2.0 so people will see the deprecation warning after their update. in rails 2.1 we can then kill the old routes.

Sorry i mean that the current behavior in stable is a bug.

True, that is the other option. Leaving the deprecation around through 2.0.x, though is it really not possible to reproduce the behaviour in 1.2.x?

OK - attached is a patch for the stable branch that I created by replacing routes.rb, test_routes.rb, routing.rb and test_routing.rb with that from trunk and then following the errors, pulling in whatever is necessary to get things to pass. I have no idea what is supposed to be included in 1.2.4 and what is not. I also did not account for deprecating the "/resource/id;action" format in favor of "/resource/id/resource". This was another area that I didn't address in the original patch and have yet to figure out how that works. I'm hoping that someone else, perhaps whomever contributed those changes to trunk, can pick up the slack and handle those warnings.

The patch for stable does the following:

1. Adds a class variable to ActionController::Base called resource_action_separator defaulting to ';' 2. If a named route has changed it generates methods with the old names which redirect to the new name with a deprecation warning 3. Moves the format parameter to the end of the path for all urls. This has the added benefit of fixing caching for these actions

The patch for edge does pretty much the same except all the deprecation warnings and tests are removed, and the resource_action_separator defaults to '/'. The tests I've added cover just what has changed - if you need some more then I'll be happy to write them.

AFAICS the patches are good to go, the only remaining question I can see is whether the namespace option should come before everything, which only affects edge.

[7415]

This quietly added , (comma) as a routing separator which breaks many existing routes!

And the stable patch totally rearranges URLs.

(Stable patch was applied in [7414].)

Replying to bitsweat:

[7415] This quietly added , (comma) as a routing separator which breaks many existing routes!

The patch returns the routing separators to what they were in 1.2.3 - do you have an example where it works in 1.2.3 and breaks in edge?

Replying to bitsweat:

And the stable patch totally rearranges URLs.

Do you mean URLs or route names? The only url changes are to move the format after the collection or member action as caching was broken. The route names are changed to what they will be in 2.0 but the old names are still there with a deprecation warning.

Replying to pixeltrix:

Replying to bitsweat:

[7415] This quietly added , (comma) as a routing separator which breaks many existing routes!

The patch returns the routing separators to what they were in 1.2.3 - do you have an example where it works in 1.2.3 and breaks in edge?

Why? It was removed on trunk purposefully. The issue is that it's a major unannounced change, not that it worked in 1.2.3.

Replying to pixeltrix:

Replying to bitsweat:

And the stable patch totally rearranges URLs.

Do you mean URLs or route names? The only url changes are to move the format after the collection or member action as caching was broken. The route names are changed to what they will be in 2.0 but the old names are still there with a deprecation warning.

Moving the format breaks inbound links to stable apps.

Replying to bitsweat:

Moving the format breaks inbound links to stable apps.

And so will upgrading to 2.0 as the action separator has been changed to /

It should be possible to fix the inbound links with some mod_rewrite magic which should be the recommended way as the position of format is changed 2.0.

Replying to bitsweat:

The patch returns the routing separators to what they were in 1.2.3 - do you have an example where it works in 1.2.3 and breaks in edge?

Why? It was removed on trunk purposefully. The issue is that it's a major unannounced change, not that it worked in 1.2.3.

In a discussion[1] with nzkoz on rubyonrails-core I got the response that the action separator should be a config options. I assumed that if it was going to be an option then there should be some options to choose from. Maybe the comma should be taken out and just have ; and / as the choices.

[1] http://groups.google.com/group/rubyonrails-core/browse_thread/thread/0f679c330c8ab7a0/9a02bdad7b6d6acf?#9a02bdad7b6d6acf

Replying to pixeltrix:

Replying to bitsweat:

Moving the format breaks inbound links to stable apps.

And so will upgrading to 2.0 as the action separator has been changed to / It should be possible to fix the inbound links with some mod_rewrite magic which should be the recommended way as the position of format is changed 2.0.

Here I'm concerned for the incompatible change made to 1.2.4 (stable) not 2.0 (edge).

Replying to pixeltrix:

Replying to bitsweat:

The patch returns the routing separators to what they were in 1.2.3 - do you have an example where it works in 1.2.3 and breaks in edge?

Why? It was removed on trunk purposefully. The issue is that it's a major unannounced change, not that it worked in 1.2.3.

In a discussion[1] with nzkoz on rubyonrails-core I got the response that the action separator should be a config options. I assumed that if it was going to be an option then there should be some options to choose from. Maybe the comma should be taken out and just have ; and / as the choices. [1] http://groups.google.com/group/rubyonrails-core/browse_thread/thread/0f679c330c8ab7a0/9a02bdad7b6d6acf?#9a02bdad7b6d6acf

Having an option is a great idea, but it needs to work well with SEPARATORS rather than hard-coding in three possibilities.

Replying to bitsweat:

Here I'm concerned for the incompatible change made to 1.2.4 (stable) not 2.0 (edge).

I've attached a patch that adds a regular route that recognizes the old format as well as the new format. The named routes still point to the new format so that over time they should migrate. Does this address your concerns regarding stable?

Replying to bitsweat:

Having an option is a great idea, but it needs to work well with SEPARATORS rather than hard-coding in three possibilities.

It turns out that SEPARATORS doesn't affect the generation and recognition of routes when specifying a custom action separator. Sorry - the routing code still a bit opaque to me at the moment (I've only been using Ruby and Rails for 8 months).

The patch I've just uploaded backs out the changes to the routing code but leaves the resource changes in place.

(In [7599]) Remove , and ; (comma and semicolon) from routing separators again. References #8558.

(In [7600]) Revert [7414] which rearranged stable resource URLs. References #8558.

Replying to bitsweat:

(In [7600]) Revert [7414] which rearranged stable resource URLs. References #8558.

Just a note that route recognition and route names will now break when upgrading to 2.0. Do you intend the old ones to be put back in to 2.0 and deprecated?

I'd expect the deprecation warnings and new functionality to make their way into stable. Launching 2.0 with some deprecated functionality would be a bit of a downer.

But our prior attempts at this have faltered ... repeatedly. So perhaps deprecated old-routes in 2.0 is the only option?

I started this whole thread and deprecated old-routes in 2.0 would make me sad. I simply don't have the cycles for the next few weeks to make me happy. What is the 1.2.4 and 2.0 timeline?

I reverted only to fix the resource URLs on the stable branch.

I agree we need this fixed up for 1.2.4 so there's a cleaner path to 2.0 from there, but first it needs to preserve existing resource URLs so upgrading a production app doesn't 404 all your incoming links.

Replying to bitsweat:

I agree we need this fixed up for 1.2.4 so there's a cleaner path to 2.0 from there, but first it needs to preserve existing resource URLs so upgrading a production app doesn't 404 all your incoming links.

The new patch I've uploaded recognizes all existing resource URLs (AFAICS). It also add the new style named routes with nested resources automatically getting a name prefix, but all the old route names are there with deprecation warnings. I've also added the resource action separator back in but this time defaulting to '/' so that any new routes generated are forward compatible (the old style routes are hardcoded to ';').

I can't see any way that existing apps will be broken - even if a developer has a hardcoded helper to generate old style routes they will still be recognized.

N.B. I uploaded a earlier version of this patch yesterday evening which had a couple of problems - I've replaced the patch this morning with a fixed version.

New patch to catch some edge cases in deprecating routes.

Added documentation fixes

(In [7713]) Change the resource seperator from ; to / change the generated routes to use the new-style named routes. e.g. new_group_user_path(@group) instead of group_new_user_path(@group). [pixeltrix] Closes #8558

I initially brought this up on Ticket #9794 and was asked to repost my thoughts here.

With the new routing updates, there was a change in how the :name_prefix is being used. We've been using :name_prefix => 'admin_', so that we can have routes that look like so: admin_hidden_comments_path. This allows us to use namespaced controllers like: app/controllers/admin/comments_controller.rb.

With 1.2.4, we're getting deprecation warnings that tell us to do something like: hidden_admin_comments_path as the new way to write this route.

What's confusing to me is that the option that we're provided to do this is called prefix, which... by definition means something that becomes. If the named route is the entire route, then the prefix should come at the begging of it. It's not obvious now if you use this option that you'll need to put the prefix in the middle of the named route.

From a name spacing concern, I'd prefer to see Rails allow us to continue to namespace our routes with name_prefix so that it's easier to scan through routes in our code and know what namespace it's in.

promote_to_tip_admin_comment_path(comment) versus admin_promote_to_tip_comment_path.

I'm not convinced that the former is easier to read, but I know that I'm not convinced that keeping name_prefix is ideal, based on the definition of prefix.

Prefix - an affix that is added in front of the word

Thoughts on this?

err... forgot to reread.

"by definition means something that becomes."

be definition means... something that comes before. ;-)

My take on this, semantically, is that the prefix is for the noun (the resource) and the verb always precedes it.

Wait, that's syntax, not semantics ;)

Robby - I'm sure you also meant that it comes at the "beginning" instead of the "begging".

Bitsweat - whether it is syntax or semantics is a matter of semantics (not syntax), don't you think? Regardless (sans ir, thank you), the verb (or adjective in less active cases) preceeding the noun seems like a rational rationale (more semantic sugar?) to me.

Robby - what say you?

ps - pixeltrix - thank you for your efforts in making this work correctly for the 1.2.4 release.

ps - pixeltrix - thank you for your efforts in making this work correctly for the 1.2.4 release.

Pure self-interest really - didn't fancy manually writing out the 400+ routes in my ecommerce engine plugin to make it work with both 1.2.x and 2.0.

My take on this is that the map.namespace functionality should be backported from 2.0 and that the specified namespace should go before the verb, so for example the following routes:

map.namespace :admin do |admin|
  admin.resources :products do |product|
    product.resources :images
  end
end

gives the following results:

...
admin_new_product       -> '/admin/products/new'
admin_new_product_image -> '/admin/products/:product_id/images/new'
...

I would also extend the map.namespace functionality to work with standard routes as well, e.g:

map.namespace :admin do |admin|
  admin.root :controller => 'base'
  admin.connect 'login', :controller => 'base', :action => 'login'
end

This would allow developers currently using the name_prefix option to namespace their routes could migrate to the correct (IMHO) map.namespace and leave the name_prefix option for nesting resources. However as things stand in 2.0 the namespace is tacked onto the front of the name_prefix so this isn't possible.

It does need a resolution one way or the other though as we don't want to end up breaking route names again.

Replying to pixeltrix:

ps - pixeltrix - thank you for your efforts in making this work correctly for the 1.2.4 release.

It does need a resolution one way or the other though as we don't want to end up breaking route names again.

Agreed. I'd love to have a solution one way or another. I'd love to keep our routes working as we move forward. :-)

I'd argue that since the prefix is being prepended to the resource and NOT the whole named route/path, perhaps changing this option to :resource_prefix would be more fitting? name_prefix, in my humble opinion, implies that is prepends to the entire named route, which is confusing... especially in the examples that I've outlined.

Our team would be happy to submit patches for either route and would love to get some feedback before moving forward too far on that.

Just a crazy idea while we're discussing it...

admin::new_product_path for namespaced named routes?

I have a proposal for 2.0 to get past the ambiguous option.

:resource_prefix would prefix the resource itself.

:route_prefix would prefix the entire route.

This would be much more obvious, in my opinion.

If this sounds sane to at least one other person, I'd be happy to work on a patch.

I'm happy with the current names that get generated:

  admin_product_url
  admin_stamp_url
  admin_foo_url

Then you get the member or collection method at the start of the name, leaving the admin_product_url part unbroken.

    edit_admin_product_url
     new_admin_product_url
activate_admin_product_url

Makes it easy to do something like:

rake routes | grep admin_product_url

Replying to nzkoz:

Then you get the member or collection method at the start of the name, leaving the admin_product_url part unbroken.

Yeah, I understand that, but with namespaces this won't be so much of an issue. However, I'd like to point out that my concern is more focused on the option that you're defining in the route. A "name prefix" implies that it's at the begging of the name, which is what you're also calling it. "start of the name", so we have a prefix that is inserted after the member/collection and before the resource. It's ambiguous to the people reading the code and I believe can be cleared up by replacing the name of the option for 2.0. "name" on it's own is too ambiguous in this case, especially for those who learned that name_prefix previously added this at the front of the entire name.

name prefix is only an option for resources, you can't pass it to other named routes.Given that, it seems reasonable that the name prefix is the prefix to the resource's name, not the route as a whole.

This ticket has received a bunch of discussion, and the new named routes have been in edge for a long time. The names are good enough to ship with, and the horse has bolted already for changes like this.