/rails

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

Fix inconsistency with changed attributes when overriding AR attribute reader #28661

Merged
merged 1 commit into from Apr 14, 2017

Conversation

Reviewers

@eavgerinos eavgerinos

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
5 participants
@bogdanvlviv @matthewd @eavgerinos @maclover7 @tenderlove
@bogdanvlviv
Contributor

bogdanvlviv commented Apr 4, 2017 •
edited

Fixes #28660.

@maclover7 maclover7 added activemodel activerecord labels Apr 4, 2017

@bogdanvlviv bogdanvlviv changed the title from Fix dirty attributes if override attribute accessor to Fix dirty attributes if override AR attribute accessor Apr 4, 2017

@bogdanvlviv bogdanvlviv changed the title from Fix dirty attributes if override AR attribute accessor to Fix inconsistency with changed attributes when overriding AR attribute accessor Apr 4, 2017

@bogdanvlviv

This comment has been minimized.

Sign in to view
Contributor

bogdanvlviv commented Apr 7, 2017

Please review.
activerecord/test/cases/dirty_test.rb Outdated
@@ -671,6 +671,25 @@ def test_datetime_attribute_doesnt_change_if_zone_is_modified_in_string
assert binary.changed?
end
test "changes is correct if override attribute accessor" do

This comment has been minimized.

Sign in to view
Copy link
@eavgerinos

eavgerinos Apr 11, 2017

Contributor

I'd rewrite this example message as well as the commit message to reference reader or getter explicitly, since writter/setter overrides do not affect Dirty hash negatively.

This comment has been minimized.

Sign in to view
Copy link
@bogdanvlviv

bogdanvlviv Apr 12, 2017

Contributor

Thank you

This comment has been minimized.

Sign in to view
Copy link
@eavgerinos

eavgerinos Apr 13, 2017

Contributor

:)

@bogdanvlviv bogdanvlviv changed the title from Fix inconsistency with changed attributes when overriding AR attribute accessor to Fix inconsistency with changed attributes when overriding AR attribute reader Apr 12, 2017

@bogdanvlviv

This comment has been minimized.

Sign in to view
Contributor

bogdanvlviv commented Apr 12, 2017 •
edited

Failed tests aren't related to changes
@bogdanvlviv

This comment has been minimized.

Sign in to view
Contributor

bogdanvlviv commented Apr 13, 2017

Thanks for restart tests! Any thoughts about this PR?

@tenderlove tenderlove merged commit e447c8c into rails:master Apr 14, 2017
2 checks passed

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:fix-dirty-attributes-if-override-attr_accessor branch Apr 14, 2017

@matthewd

This comment has been minimized.

Sign in to view
Member

matthewd commented Apr 14, 2017

For posterity, also note follow-up in #28760

@shioyama shioyama referenced this pull request Nov 29, 2017

Closed

Fix AM/AR::Dirty plugin compatibility issues with AR 5.2 #115

shioyama added a commit to shioyama/mobility that referenced this pull request Nov 30, 2017

@shioyama
Override _read_attribute in Dirty plugin 
This is required due to a fix in AR 5.2:

rails/rails#28661

Previously, `__send__` was used to fetch the value of the attribute
before changes, which worked fine with locale accessors like
`title_en`. However, with the fix above, `@attributes` is now used,
which does not contain translated attributes (let alone locale
accessors). Since in general, locale accessors are an open-ended set, we
can't treat them the way normal attributes are treated, so we resort to
overriding `_read_attribute` to check if the attribute that has changed
is one that Mobility changed, and if so use `__send__`. This fixes the
issue, although overriding a private method is not ideal.
Verified
This commit was signed with a verified signature.
@shioyama shioyama Chris Salzberg
GPG key ID: 3F77FCF15E284672 Learn about signing commits
Loading status checks…
466d11e

shioyama added a commit to shioyama/mobility that referenced this pull request Nov 30, 2017

@shioyama
Override _read_attribute in Dirty plugin 
This is required due to a fix in AR 5.2:

rails/rails#28661

Previously, `__send__` was used to fetch the value of the attribute
before changes, which worked fine with locale accessors like
`title_en`. However, with the fix above, `@attributes` is now used,
which does not contain translated attributes (let alone locale
accessors). Since in general, locale accessors are an open-ended set, we
can't treat them the way normal attributes are treated, so we resort to
overriding `_read_attribute` to check if the attribute that has changed
is one that Mobility changed, and if so use `__send__`. This fixes the
issue, although overriding a private method is not ideal.
Verified
This commit was signed with a verified signature.
@shioyama shioyama Chris Salzberg
GPG key ID: 3F77FCF15E284672 Learn about signing commits
Loading status checks…
7899d05

shioyama added a commit to shioyama/mobility that referenced this pull request Nov 30, 2017

@shioyama
Override _read_attribute in Dirty plugin 
This is required due to a fix in AR 5.2:

rails/rails#28661

Previously, `__send__` was used to fetch the value of the attribute
before changes, which worked fine with locale accessors like
`title_en`. However, with the fix above, `@attributes` is now used,
which does not contain translated attributes (let alone locale
accessors). Since in general, locale accessors are an open-ended set, we
can't treat them the way normal attributes are treated, so we resort to
overriding `_read_attribute` to check if the attribute that has changed
is one that Mobility changed, and if so use `__send__`. This fixes the
issue, although overriding a private method is not ideal.
Verified
This commit was signed with a verified signature.
@shioyama shioyama Chris Salzberg
GPG key ID: 3F77FCF15E284672 Learn about signing commits
Loading status checks…
2c39333

shioyama added a commit to shioyama/mobility that referenced this pull request Nov 30, 2017

@shioyama
Override _read_attribute in Dirty plugin 
This is required due to a fix in AR 5.2:

rails/rails#28661

Previously, `__send__` was used to fetch the value of the attribute
before changes, which worked fine with locale accessors like
`title_en`. However, with the fix above, `@attributes` is now used,
which does not contain translated attributes (let alone locale
accessors). Since in general, locale accessors are an open-ended set, we
can't treat them the way normal attributes are treated, so we resort to
overriding `_read_attribute` to check if the attribute that has changed
is one that Mobility changed, and if so use `__send__`. This fixes the
issue, although overriding a private method is not ideal.
Verified
This commit was signed with a verified signature.
@shioyama shioyama Chris Salzberg
GPG key ID: 3F77FCF15E284672 Learn about signing commits
Loading status checks…
f2cb9e6

shioyama added a commit to shioyama/mobility that referenced this pull request Nov 30, 2017

@shioyama
Override _read_attribute in Dirty plugin 
This is required due to a fix in AR 5.2:

rails/rails#28661

Previously, `__send__` was used to fetch the value of the attribute
before changes, which worked fine with locale accessors like
`title_en`. However, with the fix above, `@attributes` is now used,
which does not contain translated attributes (let alone locale
accessors). Since in general, locale accessors are an open-ended set, we
can't treat them the way normal attributes are treated, so we resort to
overriding `_read_attribute` to check if the attribute that has changed
is one that Mobility changed, and if so use `__send__`. This fixes the
issue, although overriding a private method is not ideal.
Verified
This commit was signed with a verified signature.
@shioyama shioyama Chris Salzberg
GPG key ID: 3F77FCF15E284672 Learn about signing commits
Loading status checks…
fe8c794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment