Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Deprecate the behavior of AR::Dirty inside of after_(create|update|save) callbacks #25337
Conversation
|
/cc @matthewd @rafaelfranca This is not ready to merge, but the remaining changes I have to make are to commit messages and where the deprecation warning is emitted, which shouldn't affect the discussion around this change. |
maclover7
added
activerecord
needs feedback
labels
Jun 9, 2016
olivierlacan
reviewed
Jun 9, 2016
| + end | ||
| + | ||
| + # Alias for `changed?` | ||
| + def has_changes_to_save? |
olivierlacan
Jun 9, 2016
Contributor
Wouldn't changes_to_be_saved? be more coherent with attribute_change_to_be_saved?
sgrif
Jun 13, 2016
Member
The goal was to make the difference between some of the methods more clear. Do you think that model.changes_to_be_saved? reads clearly enough? Compared to model.has_changes_to_save?
olivierlacan
reviewed
Jun 9, 2016
| + end | ||
| + | ||
| + # Alias for `changed_attributes` | ||
| + def attributes_in_database |
olivierlacan
Jun 9, 2016
Contributor
persisted_attributes seems a bit more straightforward as a method name, no?
sgrif
Jun 13, 2016
Member
I'm not happy with either one. Both names imply to me that they would return the opposite of changed (e.g. return an array of all of the attribute names which are not changed)
olivierlacan
reviewed
Jun 9, 2016
| + # Behaves similarly to +attribute_was+. This method is useful in after | ||
| + # callbacks to get the original value of an attribute before the save that | ||
| + # just occurred | ||
| + def attribute_before_last_save(attr_name) |
olivierlacan
Jun 9, 2016
Contributor
Will this return the attribute before the last save or the attribute before the last change?
sgrif
Jun 13, 2016
Member
Neither? It will return the value of the attribute in the database before the most recent save. This could definitely be more clear in the method name. Not sure how to express it without going full cocoa
|
I feel like I need to wrap my mind around the name changes in the public API outside of the code to fully focus on their semantics (I hope that's not weird and distracting from the PR convo). I hope this helps. Legend:
@sgrif Please feel free to edit the above to correct any incorrect meaning. |
|
Are those aliases intended to be public? Wasn't sure. If they are public, I agree, they were sort of hard to wrap my head around. |
This was referenced Jun 13, 2016
sgrif
closed this
Nov 1, 2016
sgrif
reopened this
Nov 1, 2016
|
Going to move forward with this change. If anyone has suggested improvements to the names of the new methods, please open a PR. |
sgrif
merged commit 29b3b5d
into
rails:master
Nov 1, 2016
sgrif
deleted the
sgrif:sg-changes-in-callbacks
branch
Nov 1, 2016
added a commit
that referenced
this pull request
Nov 1, 2016
jaredbeck
referenced this pull request
in airblade/paper_trail
Nov 22, 2016
Merged
Constrain AR < 5.1 #892
|
If you have a before_action callback that uses changes/attribute_changed?, then it'll emit a warning if that's triggered twice in a transaction, like on double save. Here's a failing test: test "changed? in before callback thats run twice in a transaction should not be deprecated" do
klass = Class.new(ActiveRecord::Base) do
self.table_name = "people"
@@save_counter = 0
before_update do
first_name_changed?
end
after_update do
@@save_counter = @@save_counter + 1
save if @@save_counter < 2
end
end
assert_not_deprecated do
person = klass.create!(first_name: "Sean")
person.update! first_name: 'Blue!'
end
end
|
|
I guess this isn't a big deal if people are just encouraged to switch to the specific checks against changes pending save / changes that have been saved. But don't feel like we're currently doing the best job of guiding in that direction. |
kamipo
referenced this pull request
Mar 18, 2017
Closed
Publish new dirty APIs to doc [ci skip] #28472
krtschmr
commented
Apr 19, 2017
•
|
@sgrif what happened to i can't figure the new naming. this looks ugly to me though: why we don't make
then we can use it `after_save :analyze_file_async, if: :file_was_updated? |
This was referenced May 8, 2017
dsandstrom
commented
May 19, 2017
|
Are the new method names documented anywhere besides here? Maybe update http://api.rubyonrails.org/classes/ActiveModel/Dirty.html . |
sgrif commentedJun 9, 2016
We pretty frequently get bug reports that "dirty is broken inside of after callbacks". Intuitively they are correct. You'd expect
Model.after_save { puts changed? }; model.saveto do the same thing asmodel.save; puts model.changed?, but it does not.However, changing this goes much farther than just making the behavior more intuitive. There are a ton of places inside of AR that can be drastically simplified with this change. Specifically, autosave associations, timestamps, touch, counter cache, and just about anything else in AR that works with callbacks have code to try to avoid "double save" bugs which we will be able to flat out remove with this change.
We introduce two new sets of methods, both with names that are meant to be more explicit than dirty. The first set maintains the old behavior, and their names are meant to center that they are about changes that occurred during the save that just happened. They are equivalent to
previous_changeswhen called outside of after callbacks, or once the deprecation cycle moves.The second set is the new behavior. Their names imply that they are talking about changes from the database representation. The fact that this is what we really care about became clear when looking at
BelongsTo.touch_recordwhen tests were failing. I'm unsure that this set of methods should be in the public API. Outside of after callbacks, they are equivalent to the existing methods on dirty.I am not married to any of the method names. Please bikeshed the shit out of them, I am open to alternatives.
Dirty itself is not deprecated, nor are the methods inside of it. They will only emit the warning when called inside of after callbacks. The scope of this breakage is pretty large, but the migration path is simple. Given how much this can improve our codebase, and considering that it makes our API more intuitive, I think it's worth doing.
Unresolved questions
Do we want the "new behavior" methods to be in the public API at all? They are straight aliases to the existing methods in dirty after 5.2/6.0. However, since we get these bug reports, someone probably does want the new behavior today.
Still left todo
I need to improve the commit messages, and move the deprecation warning up to the caller of
mutation_trackerto include the exact method to call instead. The current implementation is also emitting deprecation warnings in places where the calls are valid. This will be fixed by moving the warning to the right place.