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.

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 deprecation warnings in Rails 5.1 #44

Merged
merged 6 commits into from
Jun 22, 2017
Merged

Fix deprecation warnings in Rails 5.1 #44

merged 6 commits into from
Jun 22, 2017

Conversation

shioyama
Copy link
Owner

@shioyama shioyama commented Jun 21, 2017

There are tons of deprecation warnings currently in Rails/AR 5.1. This removes the warnings while still passing specs. However, it introduces some questionable changes to achieve this.

There are two problems:

  1. AR::Dirty now requires you to declare all attributes upfront rather than in an open-ended way with attr_will_change! (see Check for forced attribute changes rails/rails#27963). To get around this, I've overridden has_attribute? to make locale accessors (title_en etc) act like real attributes. We can't actually define them all since the list of locales is open-ended.
  2. You can't call changes from within after save callbacks. This conflicts with code used to support previous_changes in the Mobility Dirty backend module. This is fixed using a before_save hook rather than a changes_applied override, but this may fail to work as expected if the model calls previous_changes in another before_save callback added after this.

The first solution is ok I think for now, the second one may be more problematic but at least is limited to previous_changes only.

@shioyama shioyama merged commit 2f6b237 into master Jun 22, 2017
@shioyama shioyama deleted the ar_dirty branch June 22, 2017 01:41
@shioyama
Copy link
Owner Author

For the record, I got around the second issue by overriding changes_internally_applied in AR >= 5.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant