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.

Sign up for GitHub

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

Jump to bottom

Check whether the current attribute being read is aliased or not before reading #26529

Merged
merged 2 commits into from
Dec 8, 2016
Merged

Check whether the current attribute being read is aliased or not before reading #26529

merged 2 commits into from
Dec 8, 2016

Conversation

prathamesh-sonpatki
Copy link
Member

Summary

@prathamesh-sonpatki
Copy link
Member Author

r? @sgrif

Sean, should we apply same change to write_attribute as well?

@matthewd
Copy link
Member

matthewd commented Oct 3, 2016

should we apply same change to write_attribute as well?

👍 read & write should work the same way

@prathamesh-sonpatki
Copy link
Member Author

Thanks, I will update it.

@prathamesh-sonpatki
Copy link
Member Author

@matthewd done, please review.

esparta
esparta reviewed Oct 5, 2016
View reviewed changes
activerecord/lib/active_record/attribute_methods/write.rb
@@ -29,7 +29,13 @@ def __temp__#{safe_name}=(value)
# specified +value+. Empty strings for Integer and Float columns are
# turned into +nil+.
def write_attribute(attr_name, value)
write_attribute_with_type_cast(attr_name, value, true)
name = if self.class.attribute_alias?(attr_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not exactly the same code done in attributes_method/read.rb?

@sgrif
Copy link
Contributor

sgrif commented Oct 5, 2016

I think this is fine short term. Longer term I want to push this down to AtrributeSet

@prathamesh-sonpatki
Copy link
Member Author

@sgrif Sure, I can work on it now as well, should I move it to ActiveRecord::AttributeSet.

I see we have name = attribute_alias(name) if attribute_alias?(name) at lot of places. It would be good if that happens behind the scenes. Let me know your thoughts.

@sgrif
Copy link
Contributor

sgrif commented Dec 8, 2016

@prathamesh-sonpatki Can you rebase? This is fine for now to fix the bug regardless of future refactoring.

@prathamesh-sonpatki
Check whether the current attribute being read is aliased or not befo…
9cbf54c 
…re reading

- If aliased, then use the aliased attribute name.
- Fixes rails#26417.
@prathamesh-sonpatki
Check whether the current attribute being write is aliased or not bef…
44dc4f6 
…ore writing

- If aliased, then use the aliased attribute name.
@prathamesh-sonpatki
Copy link
Member Author

@sgrif done, I had started working on pushing the add_alias method to attribute set but did not finish it yet. Here is my WIP attempt so far: prathamesh-sonpatki@3928ac6

Right now lot of tests also fail so this is just my playing around the code :) But I tried to push the add_alias method onto the attribute set for every AR instance.

@sgrif
Copy link
Contributor

sgrif commented Dec 8, 2016

Using class_attribute for that is almost certainly wrong, but we can discuss in basecamp. :)

@sgrif sgrif merged commit 6e948f4 into rails:master Dec 8, 2016
@prathamesh-sonpatki prathamesh-sonpatki deleted the fix-alias-attribute-issue branch December 9, 2016 07:21
@kamipo kamipo mentioned this pull request Dec 11, 2016
Merged
@khiav223577 khiav223577 mentioned this pull request Jul 21, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esparta esparta esparta left review comments

Assignees

@sgrif sgrif

Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@prathamesh-sonpatki @matthewd @sgrif @esparta @maclover7