[Feature] Move the "to_param" logic out of ActiveRecord

63 views
Skip to first unread message

Romain Goyet

unread,
May 9, 2019, 7:29:37 PM5/9/19
to Ruby on Rails: Core
Hi everyone!

I've been using rails for a while (since rails 2) but never had the opportunity to contribute. I'd like to take this opportunity to thank all contributors to Rails :)

Anyway, I'm running into some issue with to_param. Consider the case where you have a resource that you'd like to route using identifiers that are *not* the :id. Currently, the recommended way to achieve that is by overriding the #to_param method on the model. I think that's questionable design.

Let's consider a simple example : you want to expose the same resource at two different routes. For instance, on a blog, you'd want to route articles using a slug for SEO on public URLs, and using an id on the admin interface because you use like to use ids internally.

/articles/2019-05-my-breaking-news
/admin/articles/12

Problem : there's no easy solution to achieve this, because overriding to_param is global.

Suggested solution : that's actually a routing topic, so it should be solve in the routing. I suggest leveraging the "param" routing option to do this, and leave the Models alone :)

routes.draw do
  resources :articles, param: :slug
  namespace :admin do
    resources :articles
  end
end

Currently, this changes the name of the parameter used in the route. That's a good start. But it has *no* impact on the named route helpers.
Namely, url_for(@article) will still use the :id, even though ":slug" has been specified.
I suggest to also change the named route helpers to call "@article.#{param}" if a routing param has been specified.

I hope this suggestion makes sense, just let me know :-)

Rafael Mendonça França

unread,
May 9, 2019, 7:49:01 PM5/9/19
to Ruby on Rails: Core
> Problem : there's no easy solution to achieve this, because overriding to_param is global.

Have you considered wrapping the object in a decorator that changes the `to_param` method to what you want?

url_for(@article)
# => /articles/2019-05-my-breaking-news

url_for(AdminArticleDecorator.new(@article))
# => /admin/articles/12

class AdminArticleDecorator
  def initialize(article)
    @artile = article
  end

  def to_param(*)
    @article.id
  end
end


--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at https://groups.google.com/group/rubyonrails-core.
To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-core/eac44ecb-df8d-45f2-ae8d-cb1ad04f94a1%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Romain Goyet

unread,
May 10, 2019, 7:27:00 AM5/10/19
to Ruby on Rails: Core
Actually, you're right, there are several ways to solve this. But I think all of them have their drawbacks.

 - The one you mentionned makes it mandatory to write `form_for(AdminArticleDecorator.new(@article))` instead of the more standard and shorter `form_for([:admin, @article])`
 - One could also simply write a custom helper (e.g. my_admin_article_path(@article)). Problem, this doesn't work well with form_for (especially if the record exists or not). And you still cannot do `form_for([:admin, @article])`.
 - Overriding the base helper, and don't touch to_param
def article_path(@article)
  super(@article.slug)
end

Problems: Overriding the helpers is a bit annoying because you have to keep loading the file they're defined in (every time the routing helpers are normally automatically included).

And I still think this is a routing concern, and that ActiveRecord#to_param is a bit awkward.

Namely, I think one should be able to write `form_for([:admin, @article])` and `form_for(@article)` and have those routed using different ids :-)


Le vendredi 10 mai 2019 01:49:01 UTC+2, Rafael Mendonça França a écrit :
> Problem : there's no easy solution to achieve this, because overriding to_param is global.

Have you considered wrapping the object in a decorator that changes the `to_param` method to what you want?

url_for(@article)
# => /articles/2019-05-my-breaking-news

url_for(AdminArticleDecorator.new(@article))
# => /admin/articles/12

class AdminArticleDecorator
  def initialize(article)
    @artile = article
  end

  def to_param(*)
    @article.id
  end
end


On Thu, May 9, 2019 at 7:29 PM Romain Goyet <romai...@numworks.com> wrote:
Hi everyone!

I've been using rails for a while (since rails 2) but never had the opportunity to contribute. I'd like to take this opportunity to thank all contributors to Rails :)

Anyway, I'm running into some issue with to_param. Consider the case where you have a resource that you'd like to route using identifiers that are *not* the :id. Currently, the recommended way to achieve that is by overriding the #to_param method on the model. I think that's questionable design.

Let's consider a simple example : you want to expose the same resource at two different routes. For instance, on a blog, you'd want to route articles using a slug for SEO on public URLs, and using an id on the admin interface because you use like to use ids internally.

/articles/2019-05-my-breaking-news
/admin/articles/12

Problem : there's no easy solution to achieve this, because overriding to_param is global.

Suggested solution : that's actually a routing topic, so it should be solve in the routing. I suggest leveraging the "param" routing option to do this, and leave the Models alone :)

routes.draw do
  resources :articles, param: :slug
  namespace :admin do
    resources :articles
  end
end

Currently, this changes the name of the parameter used in the route. That's a good start. But it has *no* impact on the named route helpers.
Namely, url_for(@article) will still use the :id, even though ":slug" has been specified.
I suggest to also change the named route helpers to call "@article.#{param}" if a routing param has been specified.

I hope this suggestion makes sense, just let me know :-)

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonra...@googlegroups.com.

DHH

unread,
May 10, 2019, 10:51:35 AM5/10/19
to Ruby on Rails: Core
I'd use direct routes: https://guides.rubyonrails.org/routing.html#direct-routes

And form_for has been deprecated for a while. Use form_with url: my_admin_article_path(@article).

to_param is for the most common case. You use named/direct routes for the rest.

Rob Biedenharn

unread,
May 10, 2019, 11:14:26 AM5/10/19
to rubyonra...@googlegroups.com

On 2019-May-10, at 10:51 , DHH <da...@heinemeierhansson.com> wrote:

I'd use direct routes: https://guides.rubyonrails.org/routing.html#direct-routes

And form_for has been deprecated for a while. Use form_with url: my_admin_article_path(@article).
Yet the guide you reference uses form_for in the very next section:

-Rob

Romain Goyet

unread,
May 11, 2019, 1:20:58 PM5/11/19
to Ruby on Rails: Core
Yes, I think that'd be another solution :-) One problem I have with direct routes is that they don't seem to appear when doing "rake routes" though… Also, you lose the conveniency and conciseness of RESTful routes.
 
And form_for has been deprecated for a while. Use form_with url: my_admin_article_path(@article).

Allright, I will. But correct me if I'm wrong: this won't handle the POST/PATCH method dispatch depending on wether @article is persisted, will it?
 
to_param is for the most common case. You use named/direct routes for the rest.

I guess that's fair enough :)

Still, I still think I had a point regarding the "param" option in restful routes. Let's consider a simple example : `resource :articles, param: :slug`. This will generate routes such as "edit_article GET    /articles/:slug/edit".Now in this case, I really think `edit_article_path(@article)` should generate "/articles/#{@article.slug}/edit" instead of "/articles/#{@article.id}/edit"

Would you guys consider merging a PR that would do this? Or do you guys think I'm just wrong?
Reply all
Reply to author
Forward
0 new messages