Add support for Rails 5.1 #899

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

@sgrif
Contributor
sgrif commented Nov 30, 2016

This updates the code to work with Rails 5.1.0.alpha, as of commit
rails/rails@1bdc395.

The biggest change was to call the new methods provided by
ActiveRecord::Dirty when needed. In the next version of Rails after
5.1, dirty will be "cleared" before after_save callbacks are run. This
means that code in an after_save callback will behave the same as if it
was run after calling .save.

Since the concept of "changed" is fairly nebulous, two new method sets
were introduced with names that specify whether they're operating on
changes from what we think is in the database to what's in memory, or
the changes that were just persisted. Paper trail will now use the
latter set of methods if available and called from an after_ callback.

There were a few other unrelated changes required to get this working on
master. The first was the change from appear_as_new_record to
appear_as_unpersisted. This was due to a single test that broke as a
result of
rails/rails@c546a2b
where reifying a version with a nil has_one association was persisting the
change to the foreign key. I am actually unsure how that commit caused
the problem (or more specifically, I'm unsure how it was passing
before). However, as best as I can tell, the purpose of
appear_as_new_record was to attempt to prevent the callbacks in
AutosaveAssociation (which is the module responsible for persisting
foreign key changes earlier than most people want most of the time
because backwards compatibility or the maintainer hates himself or
something) from running. By also stubbing out persisted? we can
actually prevent those. A more stable option might be to use suppress
instead, similar to the other branch in reify_has_one.

The remaining breakage was due to the Song model relying on internals
that have changed from underneath it. From Rails 5 onwards there is a
public API available to do what it wants, so we can just use that
instead.

This commit is not expected to pass on Rails 3, as #898 makes it sound
like support might be dropped. If this needs to be ammended to support
Rails 3, I will do so, but I will also grumble very loudly about it.

@sgrif sgrif Add support for Rails 5.1
This updates the code to work with Rails 5.1.0.alpha, as of commit
rails/rails@1bdc395.

The biggest change was to call the new methods provided by
`ActiveRecord::Dirty` when needed. In the next version of Rails after
5.1, dirty will be "cleared" before after_save callbacks are run. This
means that code in an after_save callback will behave the same as if it
was run after calling `.save`.

Since the concept of "changed" is fairly nebulous, two new method sets
were introduced with names that specify whether they're operating on
changes from what we think is in the database to what's in memory, or
the changes that were just persisted. Paper trail will now use the
latter set of methods if available and called from an after_ callback.

There were a few other unrelated changes required to get this working on
master. The first was the change from `appear_as_new_record` to
`appear_as_unpersisted`. This was due to a single test that broke as a
result of
rails/rails@c546a2b
where reifying a version with a nil has_one association was persisting the
change to the foreign key. I am actually unsure how that commit caused
the problem (or more specifically, I'm unsure how it was passing
before). However, as best as I can tell, the purpose of
`appear_as_new_record` was to attempt to prevent the callbacks in
`AutosaveAssociation` (which is the module responsible for persisting
foreign key changes earlier than most people want most of the time
because backwards compatibility or the maintainer hates himself or
something) from running. By also stubbing out `persisted?` we can
actually prevent those. A more stable option might be to use `suppress`
instead, similar to the other branch in `reify_has_one`.

The remaining breakage was due to the `Song` model relying on internals
that have changed from underneath it. From Rails 5 onwards there is a
public API available to do what it wants, so we can just use that
instead.

This commit is not expected to pass on Rails 3, as #898 makes it sound
like support might be dropped. If this needs to be ammended to support
Rails 3, I will do so, but I will also grumble very loudly about it.
bbfeada
@sgrif
Contributor
sgrif commented Nov 30, 2016

This PR resulted in rails/rails@ef993d3 and rails/rails@1bdc395 upstream so thanks for that!

+ end
+
+ def rails_51?
+ ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1
@sgrif
sgrif Nov 30, 2016 Contributor

This can also be written as ActiveRecord.version >= Gem::Version.new("5.1.0.alpha") if we don't need to support Rails 3. Unsure which you'd prefer, so I opted for the non-allocating version since this seems like it's called from a semi-hotpath

sgrif added some commits Nov 30, 2016
@sgrif sgrif Oh right, this has to run against Rails 5.0
I had removed the second argument since I made it optional upstream, but
in Rails 5 it's still mandatory.
305167c
@sgrif sgrif Ruby 1.9 support
EOL software woo!
b6a5ef8
@jaredbeck
Collaborator

Please also update the gemspec and Appraisals. See #898 for new naming convention in Appraisals (eg. appraise "ar-5.0"). For example, you should add appraise "ar-5.1", please.

@sgrif
Contributor
sgrif commented Nov 30, 2016

I didn't add a new appraisal, as there is no released version of Rails 5.1 for it to use.

@jaredbeck
Collaborator

I didn't add a new appraisal, as there is no released version of Rails 5.1 for it to use.

Oh right, of course, and there's no 5.1-stable branch yet either. Plus, we already have an appraise "ar_master". Sounds good.

@sgrif
Contributor
sgrif commented Nov 30, 2016

CI looks to be in its expected state. ar3 failed (will fix if y'all decide not to drop support). Master with 2+ passes, fails with 1.9

@batter batter referenced this pull request Dec 2, 2016
Merged

Drop support for rails 3 #898

@jaredbeck
Collaborator
jaredbeck commented Dec 3, 2016 edited

Sean, we've dropped support for rails 3. Please rebase, thanks.

@jaredbeck
Collaborator

Rebased, preserving authorship, and merged as #900

Thanks, Sean.

To do:

  • Method-level documentation
  • Possible improvements to method names
  • Alphabetize methods
  • Add changelog entry
  • Release PT 6.0.0
@jaredbeck jaredbeck closed this Dec 3, 2016
@sgrif
Contributor
sgrif commented Dec 3, 2016
@jaredbeck
Collaborator

No worries. We really appreciate you going above and beyond your rails team duties on this one :)

@sgrif sgrif deleted the sgrif:sg-rails-5-1 branch Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment