Merge pull request #17651 from CLUSTERfoo/fix/adding_timestamps_migration_not_reversible

Failure to rollback t.timestamps when within a change_table migration

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.

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -509,8 +509,8 @@ def remove_index(options = {})
       # 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.

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

@@ -854,7 +854,7 @@ def add_timestamps(table_name, options = {})
       #
       #  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

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -779,7 +779,7 @@ def add_timestamps_sql(table_name, options = {})
         [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
 

activerecord/test/cases/adapters/mysql/active_schema_test.rb

@@ -107,7 +107,7 @@ def test_remove_timestamps
         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

activerecord/test/cases/adapters/mysql2/active_schema_test.rb

@@ -107,7 +107,7 @@ def test_remove_timestamps
         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

activerecord/test/cases/migration/change_table_test.rb

@@ -95,8 +95,8 @@ def test_timestamps_creates_updated_at_and_created_at
 
       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
 

activerecord/test/cases/migration/command_recorder_test.rb

@@ -237,8 +237,8 @@ def test_invert_add_timestamps
       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