Merge pull request #17651 from CLUSTERfoo/fix/adding_timestamps_migration_not_reversible

Failure to rollback t.timestamps when within a change_table migration

8 files changed, 16 insertions, 11 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index e43fa2b4cc..857b48d6f1 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -5,4 +5,9 @@
 
     *Yves Senn*
 
-Please check [4-2-stable](https://github.com/rails/rails/blob/4-2-stable/activerecord/CHANGELOG.md) for previous changes.
+*   `add_timestamps` and `remove_timestamps` now properly reversible with
+    options.
+
+    *Noam Gagliardi-Rabinovich*
+
+Please check [4-1-stable](https://github.com/rails/rails/blob/4-1-stable/activerecord/CHANGELOG.md) for previous changes.
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
index d2236d046b..537e21029e 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -509,8 +509,8 @@ module ActiveRecord
       # Removes the timestamp columns (+created_at+ and +updated_at+) from the table.
       #
       #  t.remove_timestamps
-      def remove_timestamps
-        @base.remove_timestamps(name)
+      def remove_timestamps(options = {})
+        @base.remove_timestamps(name, options)
       end
 
       # Renames a column.
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index aa7c406050..fd52cdf716 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -854,7 +854,7 @@ module ActiveRecord
       #
       #  remove_timestamps(:suppliers)
       #
-      def remove_timestamps(table_name)
+      def remove_timestamps(table_name, options = {})
         remove_column table_name, :updated_at
         remove_column table_name, :created_at
       end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index bf16804aa0..a741314ac6 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -779,7 +779,7 @@ module ActiveRecord
         [add_column_sql(table_name, :created_at, :datetime, options), add_column_sql(table_name, :updated_at, :datetime, options)]
       end
 
-      def remove_timestamps_sql(table_name)
+      def remove_timestamps_sql(table_name, options = {})
         [remove_column_sql(table_name, :updated_at), remove_column_sql(table_name, :created_at)]
       end
 
diff --git a/activerecord/test/cases/adapters/mysql/active_schema_test.rb b/activerecord/test/cases/adapters/mysql/active_schema_test.rb
index 413f67da26..6577d56240 100644
--- a/activerecord/test/cases/adapters/mysql/active_schema_test.rb
+++ b/activerecord/test/cases/adapters/mysql/active_schema_test.rb
@@ -107,7 +107,7 @@ class ActiveSchemaTest < ActiveRecord::TestCase
         ActiveRecord::Base.connection.create_table :delete_me do |t|
           t.timestamps null: true
         end
-        ActiveRecord::Base.connection.remove_timestamps :delete_me
+        ActiveRecord::Base.connection.remove_timestamps :delete_me, { null: true }
         assert !column_present?('delete_me', 'updated_at', 'datetime')
         assert !column_present?('delete_me', 'created_at', 'datetime')
       ensure
diff --git a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb
index 3d97182356..e87cd3886a 100644
--- a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb
+++ b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb
@@ -107,7 +107,7 @@ class ActiveSchemaTest < ActiveRecord::TestCase
         ActiveRecord::Base.connection.create_table :delete_me do |t|
           t.timestamps null: true
         end
-        ActiveRecord::Base.connection.remove_timestamps :delete_me
+        ActiveRecord::Base.connection.remove_timestamps :delete_me, { null: true }
         assert !column_present?('delete_me', 'updated_at', 'datetime')
         assert !column_present?('delete_me', 'created_at', 'datetime')
       ensure
diff --git a/activerecord/test/cases/migration/change_table_test.rb b/activerecord/test/cases/migration/change_table_test.rb
index 6613783fba..7010af5434 100644
--- a/activerecord/test/cases/migration/change_table_test.rb
+++ b/activerecord/test/cases/migration/change_table_test.rb
@@ -95,8 +95,8 @@ module ActiveRecord
 
       def test_remove_timestamps_creates_updated_at_and_created_at
         with_change_table do |t|
-          @connection.expect :remove_timestamps, nil, [:delete_me]
-          t.remove_timestamps
+          @connection.expect :remove_timestamps, nil, [:delete_me, { null: true }]
+          t.remove_timestamps({ null: true })
         end
       end
 
diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb
index e955beae1a..8cba777fe2 100644
--- a/activerecord/test/cases/migration/command_recorder_test.rb
+++ b/activerecord/test/cases/migration/command_recorder_test.rb
@@ -237,8 +237,8 @@ module ActiveRecord
       end
 
       def test_invert_remove_timestamps
-        add = @recorder.inverse_of :remove_timestamps, [:table]
-        assert_equal [:add_timestamps, [:table], nil], add
+        add = @recorder.inverse_of :remove_timestamps, [:table, { null: true }]
+        assert_equal [:add_timestamps, [:table, {null: true }], nil], add
       end
 
       def test_invert_add_reference