Skip to content
  • Sean Griffin's avatar
    16ae3db5
    Deprecate the behavior of AR::Dirty inside of after_(create|update|save) callbacks · 16ae3db5
    Sean Griffin authored
    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.save` to do the same thing as
    `model.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_changes` when 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_record` when 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.
    
    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.
    16ae3db5
    Deprecate the behavior of AR::Dirty inside of after_(create|update|save) callbacks
    Sean Griffin authored
    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.save` to do the same thing as
    `model.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_changes` when 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_record` when 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.
    
    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.
Loading