kpumuk / meta-tags

Search Engine Optimization (SEO) for Ruby on Rails applications.
MIT License
2.73k stars 275 forks source link

[Text normalizer] Title, description, and keywords can be String-like objects #183

Closed kpumuk closed 5 years ago

kpumuk commented 5 years ago

Description

This PR is an extension of https://github.com/kpumuk/meta-tags/pull/175, adding support for #to_str to titles and keywords.

Closes #174

codyrobbins commented 5 years ago

I think it would make sense to change this to use to_s instead of to_str. I just started using this gem and was surprised to find that I couldn’t pass an Active Record model that implements to_s to title. In the meantime I’m just explicitly calling to_s but it would be much more elegant if that wasn’t needed. My understanding is that to_str is really only implemented on classes that for all intents and purposes are strings, which is quite uncommon.

By the way, while I’m writing, I just wanted to let you know that I love this gem and thank you for writing it!

kpumuk commented 5 years ago

The choice to use to_str was deliberate and thought through.

In ruby, to_s returns a string representation of an object, and is supposed to be used explicitly (in fact, it is never used implicitly by internal methods). For example,

ruby-2.5.1 >> 'donuts: ' + 5
TypeError (no implicit conversion of Integer into String)

On the other hand, to_str defines an implicit conversion, e.g. it says "this object behaves like a string", and that is exactly what the library needs – it expects strings to render as metadata. And since this object is used in a context where we expect string, we chose to use implicit conversion, and so to_str.

ruby-2.5.1 >> Integer.alias_method(:to_str, :to_s)
→ Integer < Numeric
ruby-2.5.1 >> 'donuts: ' + 5
→ "donuts: 5"

Using explicit to_s is unsafe, as it might expose internal data as metadata by accident. If you plan to pass ActiveRecord objects to meta-tags gems, the easiest way is to simply alias an existing column to to_str:

class User < ApplicationRecord
  alias_method :to_str, :email
end
codyrobbins commented 5 years ago

Thanks for the quick reply! In actual practice, I disagree that to_s is used explicitly—and I think that’s especially true in the context of Rails, which this gem is targeting. Rails implicitly calls to_s all over the place, especially in views, and this gem is largely a set of view helpers. Just as an example, you don’t typically do something like this:

<%= (1 + 1).to_s %>

Methods like puts and Ruby features like string interpolation also implicitly invoke to_s.

On the other hand, I think your example of defining to_str on an Active Record model is inherently unsafe—to_str is intended for classes that are effectively interchangeable with strings in all circumstances, and that is definitely not the case with an Active Record model. I’m not sure the exact consequences of doing so in all circumstances which is what makes me wary about it.