Let link_to infer link name from Model#to_s

[olivierlacan]

Summary

Ever since I've started using Rails I've yearned to be able to link to models like this:

link_to @profile

And have the following rendered:

<a href="/profiles/1">Eileen</a>

This PR makes this a reality. And you'll no longer have to type the following if your model defines a to_s method:

link_to @profile, @profile

Or even:

link_to @profile, @profile.name

Other Information

I initially implemented this a Railtie plugin gem but the idea of touching the internals of url_for in a gem gives me the heebee jeebees as you might understand. I'd much rather this exist as an option, even if rarely used, within Action View than within a likely soon-to-be out-of-date gem.

I'm aware this amounts to me telling the Rails team to maintain this neato feature, but I'll gladly support maintenance.

[zzak]

I'm not sure I understand the use of "pithier" here but perhaps my English skills are deteriorating 🤔

[olivierlacan]

Pithy means concise or forcefully expressive. Something concise and full of meaning, which IMO is exactly what a model is in relation to a URL helper. 🤗

[olivierlacan]

That said we can change it to "concise" to be more ESL friendly. I'll just point out I'm the ESL learner and you're the native English speaker. 😛

[olivierlacan]

Mind you I think I just used the same adjective I found earlier in the docs:

rails/actionview/lib/action_view/helpers/url_helper.rb

Line 124 in 61fb58f

# or the even pithier

[olivierlacan]

@zzak I changed this from:

+to_s+ method returning a name attribute or a default value

To this:

+to_s+ method returning a default value or a model instance attribute

I feel like the mention of a "name attribute" could have been misunderstood as the +name+ argument to link_to when really I meant a completely different model instance attribute called name. That was too confusing, the implementation of whatever custom #to_s is irrelevant.

[pixeltrix]

In principle I'm 👍🏻 on the feature, but not keen on overloading to_s for this since models may be using it for other purposes already. We have to_param for the id used in a url so seems like we should come up with a similar convention - unsure what this should be of the top of my head so open to ideas.

[olivierlacan]

@pixeltrix Good point, I'll change the docs to reference to_param.

[olivierlacan]

Actually @pixeltrix, I don't think I agree. I read your response too fast.

to_s is the convention for implicitly converting a Ruby object into its String representation and it is already used in the tests as you can see here:

https://github.com/rails/rails/blob/3f90726ecf07505356c89541206e713b6ea0ea83/actionview/test/template/url_helper_test.rb#L18-L20

In my experience with Rails apps, most often there is no implementation of to_s on many models and people are instead verbosely telling link_to how to represent their model as a string by typing: link_to(@article, @article.title) over and over again, instead of a much more straightforward link_to(@article) where link_to can abstract away the fetching of the model route to url_for while we can rely on the model itself to find its string/display representation.

We could recommend alternatives but there's nothing in my implementation that calls to_s. I'm simply relying on implicit to_s behavior which is common throughout Rails and Ruby. In my opinion, alternatives would be breaking convention, which is not what we should do. If people have their own implementations of to_s that are specific, they can always opt out of this behavior by using the common explicit two-argument form of link_to.

[tenderlove]

If people have their own implementations of to_s that are specific, they can always opt out of this behavior by using the common explicit two-argument form of link_to.

I don't think the problem is with link_to necessarily. The issue is that other parts of the system could depend on the implementation of to_s without even calling the to_s method. For example:

irb(main):001:0> class Foo; def to_s; "bar"; end; end
=> :to_s
irb(main):002:0> "foo #{Foo.new}"
=> "foo bar"

In this case, interpolation has a dependency on the to_s method and we can't really "see" it in the code. I think this is the type of implicit dependency that @pixeltrix (and I) am worried about.

Maybe we could add a link_text method? For example link_to(model) would be equivalent to link_to(model, model.link_text).

[matthewd]

I think to_s is the right thing here, exactly because of that implicit behaviour.

<%= my_model %> will already call to_s; having <%= link_to(my_model) %> produce a link with the same text seems entirely natural to me.

[tenderlove]

<%= my_model %> will already call to_s; having <%= link_to(my_model) %> produce a link with the same text seems entirely natural to me.

Ah, that's a good point and does seem very natural. I'm sold on to_s.

[pixeltrix]

Ah, that's a good point and does seem very natural. I'm sold on to_s.

I guess that's a consensus then 🙃

My other concern is that what's the out-of-the-box experience? Because currently they'll just get #<Post:0x00007f9e0e866470>, so do we need some kind of default implementation that looks for certain attribute names?

[matthewd]

@pixeltrix to me the argument still holds: that's what you'd already get from <%= model %>.

I don't really see this as providing new deep link-label-guessing magic, and more just about making link_to(model) equivalent to existing link_to(model, model) -- if you're following the right conventions (defining a sensible to_s), then you can type a slightly-shorter, slightly-less-redundant, thing to get the same result.

[olivierlacan]

My other concern is that what's the out-of-the-box experience? Because currently they'll just get #<Post:0x00007f9e0e866470>, so do we need some kind of default implementation that looks for certain attribute names?

Speaking of good points. 😃

Maybe exposing a default to_s in ApplicationRecord could help make this feature discoverable. Otherwise if that feels a bit too intrusive, ActiveRecord::Base could have def to_s; "#{self} #{id}"; end. It'll print the class name and the ID attribute (if there's one, Post 1 or Post <uuid>). That would take care of the concern raised about people having their own to_s overload, since theirs would take precedence still.

It's not perfect for printing the identity of non-AR-models or models with no primary keys (or one not called id) but it feels like a slightly better default than printing the hex Ruby object ID. That said, @matthewd has me thinking it will be too backward incompatible for anyone who relied on the previous default to_s behavior.

It's almost like scaffolds if you think about it. The default behavior isn't great but you can easily improve it by bringing your own implementation.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[ghiculescu]

Could you add a CHANGELOG entry?

[olivierlacan]

@ghiculescu Sure thing.

[olivierlacan]

@ghiculescu Pending CI but I think we're good to merge after this. Added the CHANGELOG entry at the top. Let me know if you think the entry is helpful:

*   Allow `link_to` helper to infer link name from `Model#to_s` when it 
    is used with a single argument:

        link_to @profile
        #=> <a href="/profiles/1">Eileen</a>

    Previously you had to supply a second argument even if the `Profile`
    model implemented a `#to_s` method that called the `name` method.

        link_to @profile, @profile.name
        #=> <a href="/profiles/1">Eileen</a>

    *Olivier Lacan*

[olivierlacan]

I doubt the failure in actionmailer(master) is related to this PR: https://buildkite.com/rails/rails/builds/80300#634c5085-65ce-4310-98b3-44a3ba1f6e2d/1086-1104

Ditto for railties(master): https://buildkite.com/rails/rails/builds/80300#87580cb6-7e4c-4231-ab54-d4fc0994a4b2/1330-1332

[ghiculescu]

Maybe the changelog should show how to_s is implemented on the model, to complete the picture.

[olivierlacan]

Maybe the changelog should show how to_s is implemented on the model, to complete the picture.

Yeah I think that's helpful. Updated to:

*   Allow `link_to` helper to infer link name from `Model#to_s` when it 
    is used with a single argument:

        link_to @profile
        #=> <a href="/profiles/1">Eileen</a>

    This assumes the model class implements a `to_s` method like this: 

        class Profile < ApplicationRecord 
          # ...
          def to_s
            name
          end
        end

    Previously you had to supply a second argument even if the `Profile`
    model implemented a `#to_s` method that called the `name` method.

        link_to @profile, @profile.name
        #=> <a href="/profiles/1">Eileen</a>

    *Olivier Lacan*

[olivierlacan]

Thanks @rafaelfranca 😊

[grantbdev]

Funny story: an app that is just getting upgraded to Rails 7 actually relied on the old behavior of link_to(active_record_model) where Model#to_s was used for the link name and the URL was set to url_for({}) which returns the current path. I didn't see any mention of this in the upgrade guide or release notes, but I found this by looking at the git history for link_to. 😆

[rafaelfranca]

It is in the changelog. Not everything from the CHANGELOG makes to those notes. Only things that would impact most of people and that need explanation how to solve. But, I think we can include this one. Do you want to open a PR to add it to the release notes?

[grantbdev]

@rafaelfranca I think the current judgment is sound, this probably doesn't impact many people as there wasn't really a good reason to intentionally do link_to(active_record_model) before and expect the current path instead of the model's path. Also Rails 7 is several years old now, I think if no one else encountered this first it's unlikely to come up again.

I really was just trying to share what I thought was a funny moment about working with weird legacy code, not trying to criticize any decision made by Rails. Thank you for your work!