Differentiate between remove_column and remove_columns. Make remove_column reversible.

[#8267]

4 files changed, 30 insertions, 16 deletions

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 ade10081e3..b1ec33d06c 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -423,7 +423,7 @@ module ActiveRecord
       #  t.remove(:qualification)
       #  t.remove(:qualification, :experience)
       def remove(*column_names)
-        @base.remove_column(@table_name, *column_names)
+        @base.remove_columns(@table_name, *column_names)
       end
 
       # Removes the given index from the table.
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 a2feb04b77..f2d6eb9d27 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -321,14 +321,26 @@ module ActiveRecord
         execute(add_column_sql)
       end
 
-      # Removes the column(s) from the table definition.
+      # Removes the given columns from the table definition.
       #
-      #  remove_column(:suppliers, :qualification)
       #  remove_columns(:suppliers, :qualification, :experience)
-      def remove_column(table_name, *column_names)
-        columns_for_remove(table_name, *column_names).each {|column_name| execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{column_name}" }
+      def remove_columns(table_name, *column_names)
+        raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.empty?
+        column_names.each do |column_name|
+          remove_column(table_name, column_name)
+        end
+      end
+
+      # Removes the column from the table definition.
+      #
+      #  remove_column(:suppliers, :qualification)
+      #
+      # The +type+ and +options+ parameters will be ignored if present. It can be helpful
+      # to provide these in a migration's +change+ method so it can be reverted.
+      # In that case, +type+ and +options+ will be used by add_column.
+      def remove_column(table_name, column_name, type = nil, options = {})
+        execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{quote_column_name{column_name}}"
       end
-      alias :remove_columns :remove_column
 
       # Changes the column's definition according to the new options.
       # See TableDefinition#column for details of the options you can use.
@@ -677,7 +689,8 @@ module ActiveRecord
         end
 
         def columns_for_remove(table_name, *column_names)
-          raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.blank?
+          ActiveSupport::Deprecation.warn("columns_for_remove is deprecated and will be removed in the future")
+          raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.blank?
           column_names.map {|column_name| quote_column_name(column_name) }
         end
 
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index 8aa5707959..11e8197293 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -444,15 +444,11 @@ module ActiveRecord
         end
       end
 
-      def remove_column(table_name, *column_names) #:nodoc:
-        raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.empty?
-        column_names.each do |column_name|
-          alter_table(table_name) do |definition|
-            definition.columns.delete(definition[column_name])
-          end
+      def remove_column(table_name, column_name, type = nil, options = {}) #:nodoc:
+        alter_table(table_name) do |definition|
+          definition.columns.delete(definition[column_name])
         end
       end
-      alias :remove_columns :remove_column
 
       def change_column_default(table_name, column_name, default) #:nodoc:
         alter_table(table_name) do |definition|
diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb
index f37ec1feaa..0fca1444f9 100644
--- a/activerecord/lib/active_record/migration/command_recorder.rb
+++ b/activerecord/lib/active_record/migration/command_recorder.rb
@@ -73,7 +73,7 @@ module ActiveRecord
       [:create_table, :create_join_table, :change_table, :rename_table, :add_column, :remove_column,
         :rename_index, :rename_column, :add_index, :remove_index, :add_timestamps, :remove_timestamps,
         :change_column, :change_column_default, :add_reference, :remove_reference, :transaction,
-        :drop_join_table, :drop_table
+        :drop_join_table, :drop_table, :remove_columns,
       ].each do |method|
         class_eval <<-EOV, __FILE__, __LINE__ + 1
           def #{method}(*args, &block)          # def create_table(*args, &block)
@@ -114,7 +114,12 @@ module ActiveRecord
       end
 
       def invert_add_column(args)
-        [:remove_column, args.first(2)]
+        [:remove_column, args]
+      end
+
+      def invert_remove_column(args)
+        raise ActiveRecord::IrreversibleMigration, "remove_column is only reversible if given a type." if args.size <= 2
+        [:add_column, args]
       end
 
       def invert_rename_index(args)