Merge pull request #16481 from sgrif/sg-change-default-timestamps

Change the default `null` value for timestamps

14 files changed, 105 insertions, 27 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 e44ccb7d81..9e07e9a5c4 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -56,6 +56,19 @@ module ActiveRecord
       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 @@ module ActiveRecord
     # 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 @@ module ActiveRecord
       # <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 @@ module ActiveRecord
     #   end
     #
     class Table
+      include TimestampDefaultDeprecation
+
       def initialize(table_name, base)
         @table_name = table_name
         @base = base
@@ -452,8 +470,9 @@ module ActiveRecord
       # 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 @@ module ActiveRecord
           @base.native_database_types
         end
     end
-
   end
 end
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 4957e1ac80..7105df1ee4 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -839,9 +839,9 @@ module ActiveRecord
       #
       #   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.
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 e5417a9556..a1c370b05d 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -764,8 +764,8 @@ module ActiveRecord
         "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)
diff --git a/activerecord/lib/rails/generators/active_record/migration/templates/create_table_migration.rb b/activerecord/lib/rails/generators/active_record/migration/templates/create_table_migration.rb
index fd94a2d038..f7bf6987c4 100644
--- a/activerecord/lib/rails/generators/active_record/migration/templates/create_table_migration.rb
+++ b/activerecord/lib/rails/generators/active_record/migration/templates/create_table_migration.rb
@@ -9,7 +9,7 @@ class <%= migration_class_name %> < ActiveRecord::Migration
 <% end -%>
 <% end -%>
 <% if options[:timestamps] %>
-      t.timestamps
+      t.timestamps null: false
 <% end -%>
     end
 <% attributes_with_index.each do |attribute| -%>
diff --git a/activerecord/test/cases/adapters/mysql/active_schema_test.rb b/activerecord/test/cases/adapters/mysql/active_schema_test.rb
index 7c0f11b033..a84673e452 100644
--- a/activerecord/test/cases/adapters/mysql/active_schema_test.rb
+++ b/activerecord/test/cases/adapters/mysql/active_schema_test.rb
@@ -105,7 +105,7 @@ class ActiveSchemaTest < ActiveRecord::TestCase
     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')
diff --git a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb
index cefc3e3c7e..49cfafd7a5 100644
--- a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb
+++ b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb
@@ -105,7 +105,7 @@ class ActiveSchemaTest < ActiveRecord::TestCase
     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')
diff --git a/activerecord/test/cases/adapters/postgresql/timestamp_test.rb b/activerecord/test/cases/adapters/postgresql/timestamp_test.rb
index 3614b29190..eb32c4d2c2 100644
--- a/activerecord/test/cases/adapters/postgresql/timestamp_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/timestamp_test.rb
@@ -87,7 +87,7 @@ class TimestampTest < ActiveRecord::TestCase
 
   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 @@ class TimestampTest < ActiveRecord::TestCase
 
   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 @@ class TimestampTest < ActiveRecord::TestCase
   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')
diff --git a/activerecord/test/cases/ar_schema_test.rb b/activerecord/test/cases/ar_schema_test.rb
index 8700b20dee..3f5858714a 100644
--- a/activerecord/test/cases/ar_schema_test.rb
+++ b/activerecord/test/cases/ar_schema_test.rb
@@ -14,6 +14,7 @@ if ActiveRecord::Base.connection.supports_migrations?
       @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 @@ if ActiveRecord::Base.connection.supports_migrations?
       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
diff --git a/activerecord/test/cases/migration/change_schema_test.rb b/activerecord/test/cases/migration/change_schema_test.rb
index c66eaf1ee1..bd3dd29f4d 100644
--- a/activerecord/test/cases/migration/change_schema_test.rb
+++ b/activerecord/test/cases/migration/change_schema_test.rb
@@ -176,8 +176,11 @@ module ActiveRecord
       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)
 
diff --git a/activerecord/test/cases/migration/change_table_test.rb b/activerecord/test/cases/migration/change_table_test.rb
index 3e9d957ed3..777a48ad14 100644
--- a/activerecord/test/cases/migration/change_table_test.rb
+++ b/activerecord/test/cases/migration/change_table_test.rb
@@ -88,8 +88,8 @@ module ActiveRecord
 
       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
 
diff --git a/activerecord/test/cases/migration/helper.rb b/activerecord/test/cases/migration/helper.rb
index e28feedcf9..4dad77e8fd 100644
--- a/activerecord/test/cases/migration/helper.rb
+++ b/activerecord/test/cases/migration/helper.rb
@@ -22,7 +22,7 @@ module ActiveRecord
         super
         @connection = ActiveRecord::Base.connection
         connection.create_table :test_models do |t|
-          t.timestamps
+          t.timestamps null: true
         end
 
         TestModel.reset_column_information
diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb
index ef3f073472..11338e1fb6 100644
--- a/activerecord/test/cases/migration_test.rb
+++ b/activerecord/test/cases/migration_test.rb
@@ -561,7 +561,7 @@ if ActiveRecord::Base.connection.supports_bulk_alter?
           t.string :qualification, :experience
           t.integer :age, :default => 0
           t.date :birthdate
-          t.timestamps
+          t.timestamps null: true
         end
       end
 
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index a8b21904ac..98f2492ef8 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -138,7 +138,7 @@ ActiveRecord::Schema.define do
     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 @@ ActiveRecord::Schema.define do
     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 @@ ActiveRecord::Schema.define do
   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 @@ ActiveRecord::Schema.define do
     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|
diff --git a/railties/test/generators/model_generator_test.rb b/railties/test/generators/model_generator_test.rb
index b67cf02d7b..6daedc0c29 100644
--- a/railties/test/generators/model_generator_test.rb
+++ b/railties/test/generators/model_generator_test.rb
@@ -222,7 +222,7 @@ class ModelGeneratorTest < Rails::Generators::TestCase
 
   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