Change the default `null` value for timestamps

As per discussion, this changes the model generators to specify
`null: false` for timestamp columns. A warning is now emitted if
`timestamps` is called without a `null` option specified, so we can
safely change the behavior when no option is specified in Rails 5.

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

@@ -56,6 +56,19 @@ def default_primary_key
       end
     end
 
+    module TimestampDefaultDeprecation # :nodoc:
+      def emit_warning_if_null_unspecified(options)
+        return if options.key?(:null)
+
+        ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc)
+        `timestamp` was called without specifying an option for `null`. In Rails
+        5.0, this behavior will change to `null: false`. You should manually
+        specify `null: true` to prevent the behavior of your existing migrations
+        from changing.
+        MESSAGE
+      end
+    end
+
     # Represents the schema of an SQL table in an abstract way. This class
     # provides methods for manipulating the schema representation.
     #
@@ -77,6 +90,8 @@ def default_primary_key
     # The table definitions
     # The Columns are stored as a ColumnDefinition in the +columns+ attribute.
     class TableDefinition
+      include TimestampDefaultDeprecation
+
       # An array of ColumnDefinition objects, representing the column changes
       # that have been defined.
       attr_accessor :indexes
@@ -276,6 +291,7 @@ def index(column_name, options = {})
       # <tt>:updated_at</tt> to the table.
       def timestamps(*args)
         options = args.extract_options!
+        emit_warning_if_null_unspecified(options)
         column(:created_at, :datetime, options)
         column(:updated_at, :datetime, options)
       end
@@ -405,6 +421,8 @@ def add_column(name, type, options)
     #   end
     #
     class Table
+      include TimestampDefaultDeprecation
+
       def initialize(table_name, base)
         @table_name = table_name
         @base = base
@@ -452,8 +470,9 @@ def rename_index(index_name, new_index_name)
       # Adds timestamps (+created_at+ and +updated_at+) columns to the table. See SchemaStatements#add_timestamps
       #
       #  t.timestamps
-      def timestamps
-        @base.add_timestamps(@table_name)
+      def timestamps(options = {})
+        emit_warning_if_null_unspecified(options)
+        @base.add_timestamps(@table_name, options)
       end
 
       # Changes the column's definition according to the new options.
@@ -559,6 +578,5 @@ def native
           @base.native_database_types
         end
     end
-
   end
 end

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

@@ -838,9 +838,9 @@ def columns_for_distinct(columns, orders) #:nodoc:
       #
       #   add_timestamps(:suppliers)
       #
-      def add_timestamps(table_name)
-        add_column table_name, :created_at, :datetime
-        add_column table_name, :updated_at, :datetime
+      def add_timestamps(table_name, options = {})
+        add_column table_name, :created_at, :datetime, options
+        add_column table_name, :updated_at, :datetime, options
       end
 
       # Removes the timestamp columns (+created_at+ and +updated_at+) from the table definition.

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -764,8 +764,8 @@ def remove_index_sql(table_name, options = {})
         "DROP INDEX #{index_name}"
       end
 
-      def add_timestamps_sql(table_name)
-        [add_column_sql(table_name, :created_at, :datetime), add_column_sql(table_name, :updated_at, :datetime)]
+      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)

activerecord/lib/rails/generators/active_record/migration/templates/create_table_migration.rb

@@ -9,7 +9,7 @@ def change
 <% end -%>
 <% end -%>
 <% if options[:timestamps] %>
-      t.timestamps
+      t.timestamps null: false
 <% end -%>
     end
 <% attributes_with_index.each do |attribute| -%>

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

@@ -105,7 +105,7 @@ def test_remove_timestamps
     with_real_execute do
       begin
         ActiveRecord::Base.connection.create_table :delete_me do |t|
-          t.timestamps
+          t.timestamps null: true
         end
         ActiveRecord::Base.connection.remove_timestamps :delete_me
         assert !column_present?('delete_me', 'updated_at', 'datetime')

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

@@ -105,7 +105,7 @@ def test_remove_timestamps
     with_real_execute do
       begin
         ActiveRecord::Base.connection.create_table :delete_me do |t|
-          t.timestamps
+          t.timestamps null: true
         end
         ActiveRecord::Base.connection.remove_timestamps :delete_me
         assert !column_present?('delete_me', 'updated_at', 'datetime')

activerecord/test/cases/adapters/postgresql/timestamp_test.rb

@@ -87,7 +87,7 @@ def test_timestamp_data_type_with_precision
 
   def test_timestamps_helper_with_custom_precision
     ActiveRecord::Base.connection.create_table(:foos) do |t|
-      t.timestamps :precision => 4
+      t.timestamps :null => true, :precision => 4
     end
     assert_equal 4, activerecord_column_option('foos', 'created_at', 'precision')
     assert_equal 4, activerecord_column_option('foos', 'updated_at', 'precision')
@@ -95,7 +95,7 @@ def test_timestamps_helper_with_custom_precision
 
   def test_passing_precision_to_timestamp_does_not_set_limit
     ActiveRecord::Base.connection.create_table(:foos) do |t|
-      t.timestamps :precision => 4
+      t.timestamps :null => true, :precision => 4
     end
     assert_nil activerecord_column_option("foos", "created_at", "limit")
     assert_nil activerecord_column_option("foos", "updated_at", "limit")
@@ -104,14 +104,14 @@ def test_passing_precision_to_timestamp_does_not_set_limit
   def test_invalid_timestamp_precision_raises_error
     assert_raises ActiveRecord::ActiveRecordError do
       ActiveRecord::Base.connection.create_table(:foos) do |t|
-        t.timestamps :precision => 7
+        t.timestamps :null => true, :precision => 7
       end
     end
   end
 
   def test_postgres_agrees_with_activerecord_about_precision
     ActiveRecord::Base.connection.create_table(:foos) do |t|
-      t.timestamps :precision => 4
+      t.timestamps :null => true, :precision => 4
     end
     assert_equal '4', pg_datetime_precision('foos', 'created_at')
     assert_equal '4', pg_datetime_precision('foos', 'updated_at')

activerecord/test/cases/ar_schema_test.rb

@@ -14,6 +14,7 @@ def setup
       @connection.drop_table :fruits rescue nil
       @connection.drop_table :nep_fruits rescue nil
       @connection.drop_table :nep_schema_migrations rescue nil
+      @connection.drop_table :has_timestamps rescue nil
       ActiveRecord::SchemaMigration.delete_all rescue nil
     end
 
@@ -88,5 +89,61 @@ def test_normalize_version
       assert_equal "017", ActiveRecord::SchemaMigration.normalize_migration_number("0017")
       assert_equal "20131219224947", ActiveRecord::SchemaMigration.normalize_migration_number("20131219224947")
     end
+
+    def test_timestamps_without_null_is_deprecated_on_create_table
+      assert_deprecated do
+        ActiveRecord::Schema.define do
+          create_table :has_timestamps do |t|
+            t.timestamps
+          end
+        end
+      end
+    end
+
+    def test_timestamps_without_null_is_deprecated_on_change_table
+      assert_deprecated do
+        ActiveRecord::Schema.define do
+          create_table :has_timestamps
+
+          change_table :has_timestamps do |t|
+            t.timestamps
+          end
+        end
+      end
+    end
+
+    def test_no_deprecation_warning_from_timestamps_on_create_table
+      assert_not_deprecated do
+        ActiveRecord::Schema.define do
+          create_table :has_timestamps do |t|
+            t.timestamps null: true
+          end
+
+          drop_table :has_timestamps
+
+          create_table :has_timestamps do |t|
+            t.timestamps null: false
+          end
+        end
+      end
+    end
+
+    def test_no_deprecation_warning_from_timestamps_on_change_table
+      assert_not_deprecated do
+        ActiveRecord::Schema.define do
+          create_table :has_timestamps
+          change_table :has_timestamps do |t|
+            t.timestamps null: true
+          end
+
+          drop_table :has_timestamps
+
+          create_table :has_timestamps
+          change_table :has_timestamps do |t|
+            t.timestamps null: false, default: Time.now
+          end
+        end
+      end
+    end
   end
 end

activerecord/test/cases/migration/change_schema_test.rb

@@ -176,8 +176,11 @@ def test_create_table_raises_when_redefining_custom_primary_key_column
       end
 
       def test_create_table_with_timestamps_should_create_datetime_columns
-        connection.create_table table_name do |t|
-          t.timestamps
+        # FIXME: Remove the silence when we change the default `null` behavior
+        ActiveSupport::Deprecation.silence do
+          connection.create_table table_name do |t|
+            t.timestamps
+          end
         end
         created_columns = connection.columns(table_name)
 

activerecord/test/cases/migration/change_table_test.rb

@@ -88,8 +88,8 @@ def test_remove_references_column_type_with_polymorphic_and_type
 
       def test_timestamps_creates_updated_at_and_created_at
         with_change_table do |t|
-          @connection.expect :add_timestamps, nil, [:delete_me]
-          t.timestamps
+          @connection.expect :add_timestamps, nil, [:delete_me, null: true]
+          t.timestamps null: true
         end
       end
 

activerecord/test/cases/migration/helper.rb

@@ -22,7 +22,7 @@ def setup
         super
         @connection = ActiveRecord::Base.connection
         connection.create_table :test_models do |t|
-          t.timestamps
+          t.timestamps null: true
         end
 
         TestModel.reset_column_information

activerecord/test/cases/migration_test.rb

@@ -561,7 +561,7 @@ def test_adding_multiple_columns
           t.string :qualification, :experience
           t.integer :age, :default => 0
           t.date :birthdate
-          t.timestamps
+          t.timestamps null: true
         end
       end
 

activerecord/test/schema/schema.rb

@@ -138,7 +138,7 @@ def except(adapter_names_to_exclude)
     t.integer :engines_count
     t.integer :wheels_count
     t.column :lock_version, :integer, null: false, default: 0
-    t.timestamps
+    t.timestamps null: false
   end
 
   create_table :categories, force: true do |t|
@@ -537,7 +537,7 @@ def except(adapter_names_to_exclude)
     t.references :best_friend_of
     t.integer    :insures, null: false, default: 0
     t.timestamp :born_at
-    t.timestamps
+    t.timestamps null: false
   end
 
   create_table :peoples_treasures, id: false, force: true do |t|
@@ -548,7 +548,7 @@ def except(adapter_names_to_exclude)
   create_table :pets, primary_key: :pet_id, force: true do |t|
     t.string :name
     t.integer :owner_id, :integer
-    t.timestamps
+    t.timestamps null: false
   end
 
   create_table :pirates, force: true do |t|
@@ -726,13 +726,13 @@ def except(adapter_names_to_exclude)
     t.string   :parent_title
     t.string   :type
     t.string   :group
-    t.timestamps
+    t.timestamps null: true
   end
 
   create_table :toys, primary_key: :toy_id, force: true do |t|
     t.string :name
     t.integer :pet_id, :integer
-    t.timestamps
+    t.timestamps null: false
   end
 
   create_table :traffic_lights, force: true do |t|

railties/test/generators/model_generator_test.rb

@@ -222,7 +222,7 @@ def test_model_with_polymorphic_belongs_to_attribute_generates_belongs_to_associ
 
   def test_migration_with_timestamps
     run_generator
-    assert_migration "db/migrate/create_accounts.rb", /t.timestamps/
+    assert_migration "db/migrate/create_accounts.rb", /t.timestamps null: false/
   end
 
   def test_migration_timestamps_are_skipped