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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate read_attribute(:id) returning the primary key #49019

Conversation

adrianna-chang-shopify
Copy link
Contributor

Motivation / Background

This PR deprecates read_attribute(:id) returning the primary key if the model's primary key is not the id column. Starting in Rails 7.2, read_attribute(:id) should return the value of the id column.

This commit also changes read_attribute(:id) for composite primary key models to return the value of the id column, not the composite primary key.

read_attribute should not translate "id" to mean "primary key" -- this method ought to return the raw attribute value, so that applications have an easy way to access the id attribute on a model, regardless of whether that is the model's primary key.

Detail

This pull request changes read_attribute to raise a deprecation warning if we are reading :id, but id is not the primary key. Additionally, it changes the behaviour for composite primary key models to start returning the id column value. Note that this will also result in cpk_record["id"] returning the id column, rather than the composite primary key.

I'm recommending #id be used instead. There have been other discussions about deprecating #id returning the primary key (example), but that would require introducing a new getter method for retrieving the PK value. As it stands, those changes are a long ways off 馃槃

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

This commit deprecates `read_attribute(:id)` returning the primary key
if the model's primary key is not the id column. Starting in Rails 7.2,
`read_attribute(:id)` will always return the value of the id column.

This commit also changes `read_attribute(:id)` for composite primary
key models to return the value of the id column, not the composite
primary key.
@rafaelfranca rafaelfranca merged commit 59542e8 into rails:main Aug 23, 2023
4 checks passed
@adrianna-chang-shopify adrianna-chang-shopify deleted the ac-deprecate-read-attribute-id-pk branch August 24, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants