Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

human_attribute_name inconsistently provides attributes #36916

Closed
jules2689 opened this issue Aug 12, 2019 · 8 comments
Closed

human_attribute_name inconsistently provides attributes #36916

jules2689 opened this issue Aug 12, 2019 · 8 comments

Comments

@jules2689
Copy link
Contributor

jules2689 commented Aug 12, 2019

Steps to reproduce

https://gist.github.com/jules2689/e6cc70793b3ee9c758ab1c1f293d9d0f

Run this script with and without RAILS_NEXT=1

Expected behavior

Be given a consistent data type (symbol in Rails 5).

Actual behavior

In Rails 5, we are consistently given symbol objects. In Rails 6, we are inconsistently given strings and symbols.

System configuration

Rails version: Rails 5.2.3 and Rails 6.0.0.rc2

Ruby version: Ruby 2.3.7

Other Info

Commit that broke this behavior:
8d2f317

PR that introduced this API change:
#33615

Concern

Existing applications may not run a .to_s or a .to_sym, and in fact #33615 uses a .to_s while code I'm reviewing uses a .to_sym. Alternatively, some StackOverflow posts like this one have accepted answers that do not type cast at all.

Therefore I'd think that this will introduce breaking API changes for a number of applications in production.

Solution

Here are two options I can see:

  • Option 1: Revert 8d2f317
  • Option 2:
    • Document the breaking change
    • Change all call locations to give a string instead
    • Change any docs to suggest apps run a .to_s or .to_sym

If we choose option 2, we could potentially include a message in the upgrade script in the case that it finds an instance of human_attribute_name in the code base

@jules2689
Copy link
Contributor Author

cc @Larochelle @rafaelfranca

These changes were introduced by the i18n efforts at Shopify. I'm wondering if you could triage this / weigh in? There is an included repro script I hope helps!

🙏 thank you.

@rafaelfranca
Copy link
Member

rafaelfranca commented Aug 12, 2019

human_attribute_name accepts both strings and symbols by design

parts = attribute.to_s.split(".")
. The documentation even uses strings as example
# Person.human_attribute_name("first_name") # => "First name"
.

Why it is important to be consistent? Do you have any code that rely on the argument of human_attribute_name be a symbol?

@jules2689
Copy link
Contributor Author

Up until Rails 6, only symbols were provided by the API. While looking into how to change the attribute name, the examples I came across throughout the web assumed that symbols were given.

I do understand that docs may have provided string, but not everyone may have looked there. Also, the intention might have been to provide string/symbol, but that was not what the reality was. Implicitly the design was to expect symbols.

I think it might be worth a line in Rails 6 release docs to let people know that human_attribute_name was designed to be passed symbols or strings, and that it is now actually doing that. I think it might also be worth explicitly noting that strings or symbols are valid inputs to this method in the docs. WDYT?

I was lucky that I wrote a test that checked the error messages because of some other custom logic, otherwise I would not have caught the regression in our Rails 5->6 upgrade. I want to try and help others avoid that regression too.

At minimum, this issue serves as an indexed google-able reference point if someone else experiences this issue 😆

@rafaelfranca
Copy link
Member

Up until Rails 6, only symbols were provided by the API.

That is not true. Even before 8d2f317, if you called full_messages with a string (which is also supported) you would get a string in human_attribute_name. It was also already possible for users to pass string or symbols to human_attribute_name before Rails 6.

I think it might be worth a line in Rails 6 release docs to let people know that human_attribute_name was designed to be passed symbols or strings, and that it is now actually doing that.

Not sure about this. This was always the case, not only in Rails 6.

I think it might also be worth explicitly noting that strings or symbols are valid inputs to this method in the docs.

Agree. At least this confusion will not happen anymore in the future if we have the documentation to direct people to it.

@jules2689
Copy link
Contributor Author

That is not true. Even before 8d2f317, if you called full_messages with a string (which is also supported) you would get a string in human_attribute_name. It was also already possible for users to pass string or symbols to human_attribute_name before Rails 6.

This required separate input from the user though. In my case, I was surprised because the behaviour for built in validators changed.

In Rails 5, validates :foo, ... would always give you a symbol into human_attribute_name. Same with validates_presence_of and other similar methods.

The attached commit/PR changes that to make them strings, but the same attribute is sometimes a symbol still. While it may not have been documented to rely on it being a string or symbol, it was never explicitly stated so it was kind of assumed by some people (like me) to be a symbol given the behaviour of the validators in Rails 5 and under.

Even if you could pass a string in, and some areas do, some places might not have realized this was intended behaviour.

Adding copy like Calls to human_attribute_name now pass attributes as strings in some cases instead of symbols, which is in line with examples in our docs. It is cast the attribute input to your desired type as it may be a string or symbol to release notes can go a long way here

@rafaelfranca
Copy link
Member

Adding copy like Calls to human_attribute_name now pass attributes as strings in some cases instead of symbols, which is in line with examples in our docs. It is cast the attribute input to your desired type as it may be a string or symbol to release notes can go a long way here

I'm fine with that. Can you open a PR?

@jules2689
Copy link
Contributor Author

Sure! I'll check it out soon.

@jules2689
Copy link
Contributor Author

#36922 :)

@sikachu sikachu assigned sikachu and rafaelfranca and unassigned sikachu Aug 13, 2019
rafaelfranca pushed a commit that referenced this issue Aug 15, 2019
This was always the intended API as per discussion in #36916, however versions before Rails 6 always passed a symbol. This means that some apps relied on symbols to be passed, this change is intended to note the change in behaviour so that apps can respond in kind.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants