prevent 'attribute_changed?' from returning nil

[Sen-Zhang]

a simple attempt fix for #24220

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jeremy]

Double ! is often frowned upon, so let's explain why we're doing it here.

[Sen-Zhang]

@jeremy The change is trying to fix this issue #24220. The issue is caused by the fact that method attribute_changed? could return nil. I have proposed a more comprehensive fix #24242, but it has been considered going too far because some changes are not desired. I also tried to fix it by using result.present?, but this solution is also not preferable(@pixeltrix left some comments here for that #24242).

[sgrif]

git blame gives more than enough answer for why we're doing it.

[sgrif]

Although I did just notice the commit message is pretty barren. @Sen-Zhang can you try to provide more context in your commit messages in the future?

[Sen-Zhang]

@sgrif Sorry about that. I will provide more context for sure.

[jaredbeck]

Can we add a line to the 5.0 changelog about this? This change (which I like) broke (a trivial thing in) my app and it was really hard to trace it back to this change. I think people should know this was an intentional change in 5.0 so they don't file a bug report. :)

[rafaelfranca]

But in Rails 4.2 it never returned nil, which version were your application using that relied on this behavior?

[jaredbeck]

But in Rails 4.2 it never returned nil, which version were your application using that relied on this behavior?

I'm upgrading from 4.2.7 to 5.0, so that's weird. I have a test like this:

      it "returns nil" do
        expect(RedactedModel.new.redacted_attribute_changed?).to eq(nil)
      end

Passes in 4.2.7, fails in 5.0.0. But, maybe there's something else going on.

[rafaelfranca]

Oh, it may be a regression in 4.2.7. Can you create a reproduction script and report the bug?

[jaredbeck]

Oh, it may be a regression in 4.2.7. Can you create a reproduction script and report the bug?

Well, I like this change though. I wish more functions returned actual booleans :) I'd just like to see this change acknowledged in a changelog, if we can figure out which changelog it belongs in. :)

[jaredbeck]

Rafael, Here's a script demonstrating that 4.2.7 returns nil.

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', '4.2.7'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :title
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_one
    assert_equal nil, Post.new.title_changed?
  end
end

So, this is a breaking change in 5.0.0 and it would be great if we could add it to the 5.0 changelog.

Do you want a PR updating the changelog?