Timestamp field items are affected by 2038 bug

I am facing this issue in a Drupal 7 to 8 migration where there is a timestamp in a custom table with a value of 4294967280. I will skip the issue by changing the value to 2147483647 at hook_migrate_prepare_row().

Here is the error:

[error]  Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'field_mpx_expiration_date_value' at row 1: INSERT INTO {media__field_mpx_expiration_date} (entity_id, revision_id, bundle, delta, langcode, field_mpx_expiration_date_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array
(
    [:db_insert_placeholder_0] => 101341
    [:db_insert_placeholder_1] => 4432
    [:db_insert_placeholder_2] => some_id
    [:db_insert_placeholder_3] => 0
    [:db_insert_placeholder_4] => en
    [:db_insert_placeholder_5] => 4294967280
)
 in Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToDedicatedTables() (line 1295 of /var/www/somesite/d8/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php). [0.99 sec, 55.67 MB] 
 [error]  SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'field_mpx_expiration_date_value' at row 1: INSERT INTO {media__field_mpx_expiration_date} (entity_id, revision_id, bundle, delta, langcode, field_mpx_expiration_date_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array
(
    [:db_insert_placeholder_0] => 101341
    [:db_insert_placeholder_1] => 4432
    [:db_insert_placeholder_2] => some_id
    [:db_insert_placeholder_3] => 0
    [:db_insert_placeholder_4] => en
    [:db_insert_placeholder_5] => 4294967280
)
 (/var/www/bravo/d8/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:783) [1.72 sec, 55.71 MB] 

It is really 4294967280.

Unsure why this is 9.x, we could attempt to do this now as this is a bug fix and shouldn't require a BC break?

Just out of sheer morbid curiosity...

Err, I mean..

Kind of conversion pointed by @larowlan in slack https://git.drupalcode.org/project/date_recur/blob/8.x-2.x/date_recur.in...

The patch in #16 applies cleanly but the timestamp field in UI was still set to 2037. Also database update did alter the existing field column having int to big.

In this patch there I have updated the UI to include max year as per bigint limit so as to allow date beyond 2037.

some basic questions:
1. Is the database schema update allowed for Drupal 8?
2. Can we have a rough estimate how long the update will run?
3. Is there some (node count) limit where running this update is not recommended?

Re-rolling patch

The patch in #17 does not work with 8.8.8, the patch fails to apply. Also the hook_update_N count needs to be updated since the system.install as per 8.8.8 version. The revision tables are not updated.

Closed #2911684: Dates cannot set before 1901-12-13 on 32bit as a duplicate so adding ProFire and mpdonadio for credit here.

Reroll and added an update test.

#26.
1. Changed according to https://en.wikipedia.org/wiki/Unix_time
3. Added test.

That leaves #2 doing this in a batch process.

1. +++ b/core/lib/Drupal/Core/Datetime/Plugin/Field/FieldWidget/TimestampDatetimeWidget.php
   @@ -33,7 +33,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -      '#date_year_range' => '1902:2037',
   +      '#date_year_range' => '1902:292277026596',
   

   Shouldn't 1902 also change here? 292277026596 seems a very big number :)

2. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
   @@ -86,6 +86,17 @@ public function testDateTime() {
   +    // bug. Set first the entity field to October 12th, 1492 (when the Italian
   +    // explorer Christopher Columbus officially set foot in the Americas) and
   

   I think we can do better than this for an interesting date in the past - perhaps something from https://www.quora.com/What-is-the-first-recorded-date-in-human-history

3. +++ b/core/modules/system/system.install
   @@ -1504,3 +1506,115 @@ function _system_advisories_requirements(array &$requirements): void {
   +/**
   + * Update TimestampItem field schema size to support > 2038 dates.
   + */
   +function system_update_9102() {
   +  // Build the list of field type we need to update. We know the field type
   +  // core defines, but we don't know if the current drupal instance has contrib
   +  // or custom modules extending the core classes to define custom field types.
   +  /** @var \Drupal\Core\Field\FieldTypePluginManagerInterface $fieldTypeManager */
   +  $fieldTypeManager = \Drupal::service('plugin.manager.field.field_type');
   +  $fieldDefinitions = $fieldTypeManager->getDefinitions();
   +  $fieldTypes = array_keys(array_filter($fieldDefinitions, function ($definition) {
   +    return is_a($definition['class'], TimestampItem::class, TRUE);
   +  }));
   

   I think we need to batch this update and we also should add some very very good logging to the 'update' channel of exactly what we did and why.

4. +++ b/core/modules/system/tests/fixtures/update/timestamp.php
   @@ -0,0 +1,17 @@
   +<?php
   +// phpcs:ignoreFile
   +
   +use Drupal\Core\Database\Database;
   +
   +$connection = Database::getConnection();
   +
   +// Set the schema version.
   +$connection->merge('key_value')
   +  ->condition('collection', 'system.schema')
   +  ->condition('name', 'update_test_semver_update_n')
   +  ->fields([
   +    'collection' => 'system.schema',
   +    'name' => 'update_test_semver_update_n',
   +    'value' => 'i:9000;',
   +  ])
   +  ->execute();
   
   +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,84 @@
   +      __DIR__ . '/../../../fixtures/update/timestamp.php',
   

   Is this needed?

5. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,84 @@
   +   * Tests that updates fail if the database does not meet the minimum version.
   

   Need adjusting.

1. Needs work. Note the docs for strtotime state, "For 64-bit versions of PHP, the valid range of a timestamp is effectively infinite, as 64 bits can represent approximately 293 billion years in either direction. "
2. Changed to the dates of Halley's comet.
3. That is an existing todo, #21, #26. It is something I don't know how to do.
4. No it is not needed. Removed.
5. Comment changed.

33.1 Still to do
33.3 This patch adds batching this update. Still to do here is the logging.

Also, in this patch the comments have been tweaked and a long if statement has been broken in to two if statements for readability.

Rrrrrrrrrrandom Test Failure #3214565: [random test failure] Random fail in BuildTestTest::testPortMany, back to NR to trigger a retest.

Please could we use empty() instead of simple verification?

From:

if (!$columnName) {
  return;
}

To:

if (empty($columnName)) {
  return;
}

33.1 Still to do
33.3 Added very basic logging and tests the count of logged 'success' messages. Needs works for "some very very good logging to the 'update' channel of exactly what we did and why". Ideas welcome.

#38.1 forgot to do. @RenatoG, what is gained by using empty() in this case?

1. +++ b/core/modules/system/system.install
   @@ -1504,3 +1506,179 @@ function _system_advisories_requirements(array &$requirements): void {
   +  $logger->notice("Start update of entity '$entityTypeId' field '$fieldName' to remove year 2038 limitation.");
   

   I don't think this log is that helpful. I think we need a log on all possible points where we return from this. Any exceptions that are thrown will get their own logs anyway.

2. +++ b/core/modules/system/system.install
   @@ -1504,3 +1506,179 @@ function _system_advisories_requirements(array &$requirements): void {
   +    return;
   ...
   +    return;
   ...
   +    return;
   

   These early returns need logging.

3. +++ b/core/modules/system/system.install
   @@ -1504,3 +1506,179 @@ function _system_advisories_requirements(array &$requirements): void {
   +  $items = array_slice($sandbox['items'], $sandbox['current'], 1);
   +  foreach ($items as $data) {
   +    [$entityTypeId, $fieldName] = $data;
   +    _system_update_field($entityTypeId, $fieldName);
   +    $sandbox['current']++;
   +  }
   

   Could this be?

   [$entityTypeId, $fieldName] = $sandbox['items'][$sandbox['current']]
   _system_update_field($entityTypeId, $fieldName);
   $sandbox['current']++;
   

   The foreach to loop around a single item array threw me for a bit.

4. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,96 @@
   +        $this->assertSame('int', $table['fields'][$name]['type']);
   +        if ($expected_value == 'int') {
   +          $this->assertArrayNotHasKey('size', $table['fields'][$name], "Array does not have key 'size' for $id");
   +        }
   +        else {
   +          $this->assertSame($expected_value, $table['fields'][$name]['size'], "Size not updated for $id");
   +        }
   

   We're testing the values in the schema we've stored but we're not testing that we've called $schema->changeField($table, $columnName, $columnName, $specification); correctly and updated the field on the database layer.

@quietone yes, using empty() according to this guide

Returns true if the variable does not exist or its value equals false in a loose comparison. Does not trigger an error if the variable does not exist.

$foo = 'bar';
var_dump(empty($foo));        -> false

$baz = null;
var_dump(empty($baz));        -> true

var_dump(empty($undefined));  -> true

So maybe isn't strongly necessary, but is a good practice to do. What do you think?

P.s. apart from that, according to the points reported on #40 I'm changing the status for needs work okay?

@alexpott, thank you.

#40
1. Yes, I know. I didn't mean to leave that in.
2. Logging added at those returns but I have yet to think about how to test those.
3. Oh dear, that would look odd. I had been playing with the batch size because I only know batch from migrate. And then when I set it to 1, I must have moved on to other bugs and forgot to tidy up. This is changed to your suggestion.
4. Added testing of the database columns.

@RenatoG, I must get to bed, will respond later.

Still to do:
Presumably, testing for 40.2
#41.

Great! Thank you so much

Added a new patch with a minor grammar fix in a comment

This patch adds a new timestamp field in order to prove that a 'big' timestamp field is not changed. During testing I was getting an error something like 'field needs to be updated' and then found SqlContentEntityStorageSchema::requiresFieldStorageSchemaChanges which does robust testing to determine if the schema needs to be changed. This update function is now using that methods. I suspect more if blocks can be removed but that can happen as test cases are added. I also did some clean up in _system_update_field() so that does as little as possible so it can return early if there is nothing to do.

The variables in the new functions in system.install have a mixture of lowerCamelCase and lower_case and I want to clean that up next, it is annoying.

This patch does the renaming mentioned in #44, includes work on the comments and changes so that temporary variables can be removed.

This gets the correct tables using $table_mapping->getDedicatedDataTableName which simplifies the code.

I am still a bit fuzzy on the test cases needed here. Are more needed?

And what is to be done to help those that have extended TimestampItem? Say they are not using 'value' as the column, or the timestamp is not the mainproperty?

1. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
   @@ -86,6 +86,16 @@ public function testDateTime() {
   +
   +    // Ensure field type storage allows bigint and so is not affected by the
   +    // 2038 bug. First, set the date to -467BC, the possible first siting of
   +    // Halley's comet. Then set the date to 2134, an expected return date of
   +    // Halley's comet.
   +    $entity->field_timestamp->value = -76904336343;
   +    $entity->save();
   +
   +    $entity->field_timestamp->value = 5182657201;
   +    $entity->save();
      }
    
   

   Should we also call $entity->validate() here to make sure that these timestamps pass validation and a negative test with a higher value that is not allowed? not sure we even can do that, FWIW.

2. +++ b/core/modules/system/system.install
   @@ -1504,3 +1507,152 @@ function _system_advisories_requirements(array &$requirements): void {
   +
   +/**
   + * Update TimestampItem field schema size to support dates greater than 2038.
   + */
   +function system_update_9301(&$sandbox) {
   

   well. this is going to be a scary update on big sites :-/.

   Also wondering if we can get away with making it a post update. entity updates usually are, and this shouldn't cause any problems when not updated beside entity update definition manager complaining.

3. +++ b/core/modules/system/system.install
   @@ -1504,3 +1507,152 @@ function _system_advisories_requirements(array &$requirements): void {
   +
   +      // Get the definitions for all timestamp fields on this entity by
   +      // intersecting all field definitions with the array of timestamp fields.
   +      $field_definitions = array_intersect_key($entity_field_manager->getFieldStorageDefinitions($entity_type_id), $fields);
   +
   +      // Save each entity, field name pair.
   +      foreach ($field_definitions as $field_definition) {
   +        $items[] = [$entity_type_id, $field_definition->getName()];
   +      }
   +    }
   

   the field map already has the field names too, I don't think we need to load the field definitions just to show that

   \Drupal::service('entity_field.manager')->getFieldMapByFieldType('timestamp');
   => [
        "user" => [
          "access" => [
            "type" => "timestamp",
            "bundles" => [
              "user" => "user",
            ],
          ],
          "login" => [
            "type" => "timestamp",
            "bundles" => [
              "user" => "user",
            ],
          ],
        ],
   

   So you really just need the keys of $fields, which you in fact already do with the array_intersect_key() here.

   Meaning, just:

   
   foreach (array_keys($fields) as $field_name) {
     $items[] = [$entity_type_id, $field_name];
   } 
   

4. +++ b/core/modules/system/system.install
   @@ -1504,3 +1507,152 @@ function _system_advisories_requirements(array &$requirements): void {
   +
   +  /** @var \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager */
   +  $entity_field_manager = \Drupal::service('entity_field.manager');
   +  $entity_field_manager->useCaches(FALSE);
   +  $storage_definitions = $entity_field_manager->getFieldStorageDefinitions($entity_type_id);
   +
   

   wondering if we should just once, in the update function init for example, clear caches instead of keep disabling them. The first time we save a change, they are cleared anyway.

5. +++ b/core/modules/system/system.install
   @@ -1504,3 +1507,152 @@ function _system_advisories_requirements(array &$requirements): void {
   +  }
   +
   +  // Update the tracked entity table schema, setting the size to 'big'.
   +  $schema_key = "$entity_type_id.field_schema_data.$field_name";
   +  $entity_storage_schema_sql = \Drupal::keyValue('entity.storage_schema.sql');
   +  $field_schema_data = $entity_storage_schema_sql->get($schema_key);
   +  foreach ($field_schema_data as $table_name => $field_schema) {
   +    $field_schema_data[$table_name]['fields'][$column_name]['size'] = 'big';
   +  }
   +  $entity_storage_schema_sql->set($schema_key, $field_schema_data);
   +
   

   Yes, as expected, scary stuff.

   I do wonder though. Could we bake support for changing the size, at least making it bigger than before, into \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema?

   Then we could just do a updateFieldStorageDefinition() and keep the dark magic contained within that class.

   That would be \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::updateDedicatedTableSchema() and \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::updateSharedTableSchema()

   We would need to get \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::hasColumnChanges() to be OK with an "upsize" of integer fields specifically and then implement the necessary logic for that below.

   The advantage is that the call to \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::saveFieldSchemaData() will take care of the installed schema and last installed definitions stuff.

   There are downsides to that, we can not specifically apply just size change, it will at least re-create the indexes, but that might be required anyway?

   I also haven't actually tried any of that.

6. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,155 @@
   +   */
   +  protected function setDatabaseDumpFiles() {
   +    $this->databaseDumpFiles = [
   +      __DIR__ . '/../../../fixtures/update/drupal-9.0.0.bare.standard.php.gz',
   +    ];
   +  }
   +
   

   should we maybe use the filled database dump here to have those updates tested with data in those tables?

> And what is to be done to help those that have extended TimestampItem? Say they are not using 'value' as the column, or the timestamp is not the mainproperty?

Well, I do hope nobody did that, but from what I see, the code does check whether the value property is there. Technically, we could check on the field definition if the value property is defined, but honestly, I think that's a very far-fetched edge case. Why would you extend a timestamp field if it doesn't have a timestamp? Nothing in TimestampItem is going to be of any value to you then.

@Berdir, thanks for taking the time to review.

#48.
1. Added validation via entityValidateAndSave. Added checks for the min and max values and the min value fails. This needs more work.
2. Moved to a post_update hook. No surprises doing that but then I have never done one before.
3. I suspected that was over complicated but it appears I never got back to investigating that. Thanks!
4. Adding clearing the cache in _system_update_get_timestamp_fields(). Is that what you meant?
5. Skipped the scary bit this time.
6. Changed to using the filled database fixture. That changed the list of fields, $timestampFieldStorage, in the test.

Todo.
48.1 - the test will fail
48.5

Another iteration.

48.1. The test fails when setting the timestamp to the minimum value. The value becomes a float instead of an int and that results in "This value should be of the correct primitive type.".
48.5. I am not convinced this is robust, it just checks the data type to allow changes for timestamp, in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::updateSharedTableSchema. It does seem a much better approach though. If this is the way to go a similar change in needed in updateDedicatedTableSchema (as mentioned in #48) and that will need tests. Haven't even thought about that yet.

Seems the following remaining tasks in the issue summary can be marked off:

• #19
• batch process the hook_update()
• write upgrade path tests

And I've added the following:

• 33.1 set minimum value for #date_year_range
• 47 are more test cases needed?
• 48.1 $entity->validate() tests
• 48.5

@mstrelan, thanks for updating the IS. It makes working on this easier.

This patch updates the min value for TimestampItem and adds a test of values out of range.

Validation of the entity was added in #49 by using $this->entityValidateAndSave($entity);. Updating the IS.

33.1. I am wondering if changing the date range on the widget should be in a followup because the widget only handle 4 digit years. It seems prudent to discuss in a separate issue how to handle 12 digit years.

edit: fix typo. s/I/It/

+++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
@@ -86,6 +87,54 @@ public function testDateTime() {
+   */
+  public function providerInvalidDate() {
+    return [
+      [
+        -9223372036854775807 - 1,
+        AssertionFailedError::class,
+        'This value should be <em class="placeholder">&quot;-9223372036854775807&quot;</em> or more. (code 76454e69-502c-46c5-9643-f447d837c4d5)',
+      ],
+      [
+        9223372036854775807 + 1,
+        \TypeError::class,
+        'DateTime::setTimestamp(): Argument #1 ($timestamp) must be of type int, float given',
+      ],
+    ];

not a huge fan of dataprovider tests in kernel tests. each one needs to go trough the setup again. Not a huge deal on kernel tests compared to browser tests, but still, quite a bit of overhead.

This also proves what I suspected, that it very weird to test the validation outside of those ranges, because they are not even integers anymore. strange that the negative value still works though. I think the reason for that is that we're actually using the wrong number here, the lowest int64 number is one lower: https://stackoverflow.com/questions/6003492/how-big-can-a-64bit-signed-i.... So, not that it matters, but strictly speaking, our validation of it is wrong ;)

I'd be fine to just drop that test then, sorry for the extra loops

No worries, removing test.

Yes, the minimum value behaves strangely. Using PHP_INT_MIN returns an int but using the value of PHP_INT_MIN returns a double.

php > var_dump(PHP_INT_MIN);
php shell code:1:
int(-9223372036854775808)
php > var_dump(-9223372036854775808);
php shell code:1:
double(-9.2233720368548E+18)
php > $x=PHP_INT_MIN;
php > var_dump($x);
php shell code:1:
int(-9223372036854775808)
php > $x = -9223372036854775808;
php > var_dump($x);
php shell code:1:
double(-9.2233720368548E+18)

Oops,

Also, decided to update the date range for the widget, which I got from this.

foreach ([PHP_INT_MIN + 1, PHP_INT_MAX] as $extreme) {
        $d = new DateTime('@' . $extreme);
        $date = $d->format('Y-m-d H:i:s');
        echo PHP_EOL . '[' . $extreme . '] ' . $date . PHP_EOL;
}

[-9223372036854775807] -29227700493-05-20 06:56:53
[9223372036854775807] 29227704433-08-14 17:03:07

I'm more concerned about having to find a new planet to use Drupal on (or a new universe?) than switching over to int128, that should be widely available by then :)

The parent issue was set to Critical before this was split to child issues. Marking this Critical as well.

Thanks for the patch.

First, you need to check whether the entity is stored by the standard SQL schema see https://www.drupal.org/files/issues/2021-07-05/838992-54.patch user_update_9301 first three lines.

Second, I really don't like how if (!in_array($type, ['timestamp', 'changed', 'created'])) { stays forever. Sure, it enables this update but it removes a protection for these types forever. I have a very hard time wrapping my head around the potential consequences. I filed #3222270: Allow bypassing the update validation in SqlContentEntityStorageSchema with my suggestion. I really am grateful for this issue to bring this problem up because it solves a rather big issue my module has.

Third, I love the Halley's Comet tests :) however I would love it even more if we were to reload the entity and checked the values are there.

@Charlie ChX Negyesi, thanks for the review! Thank you for the compliment on Halley's Comet. I don't follow what you mean by removing a protection from timestamp', 'changed', 'created'.

This patch makes changes for the first and third point in #63. It was all rather straightforward, except resetCache is now called inside the loop in _system_update_get_timestamp_fields.

> I don't follow what you mean by removing a protection from timestamp', 'changed', 'created'.

After this patch, the "The SQL storage cannot change the schema for an existing field" exception will no longer be thrown for these field types. That's what I meant.

Re-rolled patch in #64 as requested in #65, thanks!

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
@@ -0,0 +1,161 @@
+        $db_column_size = $this->connection->query("SELECT data_type FROM information_schema.columns WHERE table_schema = '$this->databaseName' and table_name='$table_name' and column_name='$column_name';")
+          ->fetchField();

This does not work for PostgreSQL and SQLite. This will get fixed in #301038: Add a cross-compatible database schema introspection API. Therefor postponing this issue.

That's a very old and I assume complex issue to postpone this critical issue on on. This is just a test, I think we can possibly find a way to verify this without requiring to query the information, we could for example just test implicitly by trying to write a value there or so. Will have a closer look also at the update field related changes.

Applied this patch to the 9.2.x version
Also, added try/catch to avoid exceptions for timestamp fields with data

Fixed code styles, please ignore #71 and use this one.

Fixed exception in update of field storage definition for fields with existing data

Ran this on decent sized database (largest entity table is half a million rows):

time drush updb -y
 -------- ----------- ------------- ------------------------------------------- 
  Module   Update ID   Type          Description                                
 -------- ----------- ------------- ------------------------------------------- 
  system   y2038       post-update   Update TimestampItem field schema size to  
                                     support dates greater than 2038.           
 -------- ----------- ------------- ------------------------------------------- 


 // Do you wish to run the specified pending updates?: yes.                   

>  [notice] Update started: system_post_update_y2038
>  [notice] Successfully updated entity 'user' field 'access' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'user' field 'login' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'comment' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'file' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'media' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'media' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'node' field 'revision_timestamp' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'node' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'redirect' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'taxonomy_term' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'user' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'menu_link_content' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'comment' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'file' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'media' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'node' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'taxonomy_term' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'user' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'menu_link_content' field 'changed' to remove year 2038 limitation.
>  [notice] Update completed: system_post_update_y2038
 [success] Finished performing updates.

real	0m38.110s
user	0m2.290s
sys	0m0.426s

Not bad at all. Probably worse if you get into tens of millions of entities, but those sites should be running updates on a local/staging environment before they get to production.

Added reroll of patch #73 on Drupal 9.4.x.

Changing status for the last patches added.

It seems the concerns I raised in #63 are still valid -- although it's possible of course I was communicating them badly: currently, there is an exception thrown when an attempt is made to change the SQL schema of a field. By adding if (!in_array($type, ['timestamp', 'changed', 'created'])) { if a future update attempts to change the SQL schema of a field of these types then the exception will not be thrown. I suggested a generic solution in a separate issue.

Needs a reroll for 10.1

Rerolled patch #75 for 10.1.x, please review it.

Fixing custom commands failure issues in #81, please review it.

The upgrade path test is still needed.

Needs a reroll

Forgot to run the code checks.

A first attempt at a test.

1. +++ b/core/modules/system/tests/src/Functional/Update/Y2038SchemaUpdateTest.php
   @@ -0,0 +1,166 @@
   +    $this->databaseDumpFiles = [
   +      // Start with a standard install of Drupal 9.3.0 with the following
   +      // enabled modules: forum, language, locale, statistics and tracker.
   

   Nit: 9.4.0

2. +++ b/core/modules/system/tests/src/Functional/Update/Y2038SchemaUpdateTest.php
   @@ -0,0 +1,166 @@
   +
   +      // The current specification is for a small int.
   +      $this->assertArrayHasKey('size', $specification_current, "Failed for '$field_data[0]' original specification '$field_data[1]'");
   

   Nit: big?

This patch makes the addresses #87, and fixes another comment as well.

Well, the test passes locally so a bit stuck on what to do next.

+++ b/core/modules/system/system.post_update.php
@@ -51,3 +56,132 @@ function system_post_update_linkset_settings() {
+ try {
+ \Drupal::service('entity.definition_update_manager')->updateFieldStorageDefinition($storage_definition);
+ }
+ catch (FieldStorageDefinitionUpdateForbiddenException $e) {
+ }
+
+ $logger->notice("Successfully updated entity '$entity_type_id' field '$field_name' to remove year 2038 limitation.");

If the exception is thrown, it just falls through to log the success message. That needs to be fixed. It should log a useful message when the exception is thrown. This failure is not tested and I am not sure how to test it. Anyone?

For background, the try/catch was added in #71 to "avoid exceptions".

+++ b/core/modules/system/system.post_update.php
@@ -51,3 +56,132 @@ function system_post_update_linkset_settings() {
+
+  // Update the tracked entity table schema, setting the size to 'big'.
+  /** @var \Drupal\Core\Entity\EntityDefinitionUpdateManager $mgr */
+  try {
+    \Drupal::service('entity.definition_update_manager')->updateFieldStorageDefinition($storage_definition);
+  }
+  catch (FieldStorageDefinitionUpdateForbiddenException $e) {
+  }

What happens if we just remove the try/catch?

Let's find out.

OK so it looks like it's not needed, in which case I'd leave it out - if we're not able to update the entity definition something is wrong. Not nice when updates throw exceptions but not nice when they silently fail and leave your site in an inconsistent state either. The other option would be to catch the exception but log it as and error I guess.

Re-roll #89 for 10.0.x

#98 worked for me on 10.1.1, PHP 8.2, and MySQL 8. Thank you!

Actually, here's a re-roll for 10.1.1. #98 didn't apply completely clean on 10.1.1 because of a couple offsets.

Here's a re-roll for 11.x

Test failures are real - are we missing a file adding that test field or similar? #93 has different test failures.

Sorry @catch, I didn't do my due diligence before posting the last 2 patches. I just grabbed the last patch posted and rerolled it for 10.1.x and 11.x. I grabbed patch #98, which was a reroll of #89, which means it missed the work you and @quietone did in #91 - #95.

Here is a reroll of #93 for 11.x

Looking at the remaining tasks in the issue summary, it looks like one of the main things left to do is address the issue mentioned in #63, #66, and #77. The way this was worked around in #93 was to bypass the result of hasColumnChanges() using this code:

if ($this->hasColumnChanges($storage_definition, $original)) {
  $type = $storage_definition->getType();
  if (!in_array($type, ['timestamp', 'changed', 'created'])) {
    throw new FieldStorageDefinitionUpdateForbiddenException('The SQL storage cannot change the schema for an existing field (' . $storage_definition->getName() . ' in ' . $storage_definition->getTargetEntityTypeId() . ' entity) with data.');
  }
}

The problem here is that 'timestamp', 'changed', and 'created' fields will forevermore be able to bypass hasColumnChanges().

hasColumnChanges() was added in issues #2544954 and #2542748 because SqlContentEntityStorageSchema cannot handle column changes. But in this case, the column changes in the database have already been handled in _system_update_process_timestamp_field() before getting to SqlContentEntityStorageSchema.

So what if we just set a flag on $storage_definition in _system_update_process_timestamp_field() to let SqlContentEntityStorageSchema know that the column changes have already been handled, like this:

foreach ($table_mapping->getAllFieldTableNames($field_name) as $table) {
  $schema->changeField($table, $column_name, $column_name, $specification);
}

// Set flag to let EntityDefinitionUpdateManager know the column changes
// have been handled.
$storage_definition->setSetting('column_changes_handled', TRUE);

// Update the tracked entity table schema, setting the size to 'big'.
/** @var \Drupal\Core\Entity\EntityDefinitionUpdateManager $mgr */
\Drupal::service('entity.definition_update_manager')->updateFieldStorageDefinition($storage_definition);

Then check for the flag in SqlContentEntityStorageSchema before hasColumnChanges() like this:

if (empty($storage_definition->getSetting('column_changes_handled')) && $this->hasColumnChanges($storage_definition, $original)) {
  throw new FieldStorageDefinitionUpdateForbiddenException('The SQL storage cannot change the schema for an existing field (' . $storage_definition->getName() . ' in ' . $storage_definition->getTargetEntityTypeId() . ' entity) with data.');
}

It seems like this issue of synchronizing changes in database schema with the field storage definition stored in config would also affect how custom modules update the schema for their custom entities. @catch mentions this in the last paragraph of 2542748-24. How is it that custom modules work around this hasColumnChanges() issue to change the columns of a field on a custom entity? Couldn't the same thing be done here to update these timestamp fields? (Sorry, I'm a longtime D7 developer, so I'm still getting up to speed on all things D10/11, like how things like custom entity updates work.)

About the failed tests, I took a look at them, but don't see why they would fail. The one test says it fails to find the success message in dblog. But that must mean the other tests that check for the new field specifications passed. It seems hard to believe that the code would succeed in setting the new field specs, but fail in setting a simple log message. I would be interested to see the html it fetched from $this->drupalGet("admin/reports/dblog/event/$event") to see if it actually got a dblog page or maybe a 404 page or something else.

The other test that fails just says 'Unknown', so I don't know what to make of that or what test it is even referring to.

So what if we just set a flag on $storage_definition in _system_update_process_timestamp_field() to let SqlContentEntityStorageSchema know that the column changes have already been handled

That seems like a good idea, let's try it :)

Great! That idea is implemented in patch #107 if you'd like to review it, although the tests failed on the patch because of those other separate issues with the tests (as described at the end of #107).

I'm not sure if it violates any standards/policies to use the setSetting()/getSetting() functionality on BaseFieldDefinition in this way. If so, we could look for another method of setting the flag.

I have run into a similar issue in terms of trying to get the stored schema definitions updated on a field with existing data, in my case for the Smart Date contrib module, which uses timestamps for date and time storage. IMHO it seems a little crazy that as a contrib author I can easily update the database tables to increase the storage size, but core's schema definition manager refuses to update the stored schema definition if there is existing data, even if the tables themselves have already been updated. This leaves me with a warning about the definitions needing to be updated that can't be fixed, at least using normal schema update methods.

I tend to think that the schema update restrictions shouldn't apply to updates that increase the storage, though I suppose that in practice this might be nontrivial to implement. I like that the patch in #107 introduces a flag that can be used for cases where the database have already been updated, so updating the stored definitions is needed to bring them up to date.

I'll take a look at the failures to see if I can get the tests to pass, since having this flag available to get schema definitions updated would be invaluable.

this is just an update of the patch.

Fix Y2038SchemaUpdateTest.php by searching the logs for the 'wid' of the first log message we want to check.

Remove unused use statement and tidy some use statements.

Y2038SchemaUpdateTest.php passes locally but is still failing on drupalci.

The other failure is because the new updates are looking to change the storage tables for all the instances of a timestamp it finds. The existing test TimestampFormatterSettingsUpdateTest.php adds a timestamp field but it does not create the association node and node_revision tables. Then, when the new update run it will fail because the tables do not exist. This change adds a node and node_revision table for that field in the fixture, drupal.timestamp-formatter-settings-2921810.php. That allows the test to pass locally.

Seems to have a test failure.

The fix was to rework the test to not rely on the order of the log messages. And instead of loading the the log page, admin/reports/dblog/event/N, it now just checks the value stored in the database table.

I converted to an MR and am hiding the patches. Removed the tag, "Needs upgrade path tests", because a test exists.

I think this item, "See #63, #66 and #77. We need a way to update schema on those fields with data, but without removing the checks from SqlContentEntityStorageSchema" from the remaining task is resolved by using the new flag. See #107 and #108

There is still #48.5

I was wrong #51 made the updates for #48.5. I will strike this and the item mentioned above out from the remaining tasks.

So, I think we are left to reviews and making sure testing is sufficient.

Add a change record

Applied the MR.

Ran updb and got

>  [notice] Update started: system_post_update_y2038
>  [notice] Successfully updated entity 'user' field 'access' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'user' field 'login' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'block_content' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'comment' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'file' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'media' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'media' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'menu_link_content' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'node' field 'revision_timestamp' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'node' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'taxonomy_term' field 'revision_created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'user' field 'created' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'block_content' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'comment' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'file' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'media' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'menu_link_content' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'node' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'taxonomy_term' field 'changed' to remove year 2038 limitation.
>  [notice] Successfully updated entity 'user' field 'changed' to remove year 2038 limitation.
>  [notice] Update completed: system_post_update_y2038

So that ran without issue.

Rebased so I can run the test-only feature and got

1) Drupal\Tests\field\Kernel\Timestamp\TimestampItemTest::testDateTime
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_timestamp.0.value:
    This value should be between <em class="placeholder">&quot;-2147483648&quot;</em> and <em class="placeholder">&quot;2147483648&quot;</em>. (code 04b91c99-a946-4221-afc5-e65ebac401eb)
/builds/issue/drupal-2885413/core/modules/field/tests/src/Kernel/FieldKernelTestBase.php:158
/builds/issue/drupal-2885413/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php:96
/builds/issue/drupal-2885413/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!

Think this is good.

I think we should add an additional change record for the 'column_changes_handled' setting - since that could potentially be used by contrib, including to fix 2038 bugs if there's a non-timestamp field type storing a timestamp out there.

Also some minor comments on the MR.

MR is still green and new CR reads well I think. Remarking.

I've added some questions to the MR. I think we need to deal with an empty $sandbox['items']

@Berdir I see you recommended a post update. I'm not sure. @catch and I debated this. See transcript below.

The tldr; we settled on moving this back to a hook_update_N() function.

alexpott: been looking at https://www.drupal.org/project/drupal/issues/2885413 - and I was intrigued to see we’d done the schema fix in a post update. Berdir asked for this in #48.2 - saying that entity updates usually are. But I’m not convinced that entity schema updates are that usual and really belong in post updates.

catch: I was looking at that too and trying to remember, but if I look at the code the update calls, it is IMO not safe in a hook_update_N().

catch: Also the actual schema update itself, is safe in a post update

catch: So it is the opposite of what is intuitive.

alexpott: Yeah but this means a post update can’t create an entity and with a date in 2038.

catch: Yeah they also can't do that now.

alexpott: Feels really odd.

catch: If it's a hook_update_N(), what happens if a different hook_update_N() affects getting the entity field map?

catch: Or any of the field storage definitions.

alexpott: It’ll need to use update dependencies

alexpott: Not nice but this problem has always existed.

catch: We load but do not save the field storages, that makes it right on the line for me.

catch: If it was saving the field storages -> definitely post update.

alexpott: Yeah… this one is tricky.

alexpott: The sorting of post update is fun :slightly_smiling_face:
    // Ensure that the update order is deterministic.
    sort($updates);

alexpott: We could put system ones first…

alexpott: #3129231: Allow altering the post_update run order

catch: I am trying to think what that means.

catch: It would fix the 'post update that wants to save an entity with a post-2038 timestamp' because whatever happened, it would run after this system one.

alexpott: It would match hook_update_N where we run system updates first by default. And post updates are not supposed to have precedence. If you need to fix the system then it should be hook_Update_N() (hence my original why is this a post update)

catch: hmm so the actual argument is that because it's fixing something that would allow a later post update to run successfully (that doesn't now), it should go in hook_update_N() so that all post updates inherently depend on it.
vs for me making it a hook_update_N() makes it a bit more fragile because it's more likely another hook_update_N() affects config entity loading. BUT... it doesn't save any config entities so that is not really very likely to be a real issue.

catch: I'm agnostic now :P

alexpott: Well my first thought is this is a db schema change and they belong in hook_update_N().

catch: Yeah but it's transparent, a bit like adding indexes, if it was changing the column name then definitely.

catch: I can go for hook_update_N(), bit of luck it is just renaming the method and moving it to the right place, and no other changes.

I'm ok with keeping it as a regular update. I think beside the usual things reasons for post update or not, my thought was that technically a side could have a workflow that would allow it to respond to requests again while it's running post updates. But I'm not even sure that's possible.

I didn't review the MR since a long time, but I'm still concerned that the update, post or not, is going to to be a pain on big sites with hundreds of thousands of nodes and other entities. It's a lot of fields to update and each is going to take minutes. What are we going to do if that causes an hour downtime or something?

What I'd propose is as settings flag to skip this update. Either custom for this, or a more generic system, like $settings['skip_update'] = ['system_post_update_y2038', 'system_update_12000').

We could be nice and provide this as a custom console command (not sure if we'd need to add batch support for that), or there could be a module that provides a drush command. Then you can update, and then run this in the background. it's going to slow down your site and you might will get locks/long waits on table writes, but at least you're not in a maintenance mode for an hour or more.

@Berdir I agree - having a way to opt out of the update and manage it on yourself seems sensible. I think we need to provide this mechanism. I also think the update should bail after an amount of time and error telling people to use this setting and options for doing the alters themselves. I know that Percona has a tool for altering tables for example. I think that this means we also need to provide something in the status report to tell you that your timestamp fields have been fixed.

As a contrib maintainer, I've been following this issue closely, because the ability to allow stored definitions to be updated after database columns have already been upsized would be invaluable for me. It sounds like there is still some distance to cover before the proposed changes here can be merged. Would it be possible to introduce that specific change, to introduce the optional "column_changes_handled" setting, in a separate issue more quickly?

Or, looking at this another way, is there a precedent for the "opt out" update path? If so I'd be happy to help adapt an existing example to what's needed here.

The possibility of an empty $sandbox['items'] was cited as the key reason to move this to Needs Work, so moving this back to Needs Review, since the code has been updated to handle this possibility.

I have also created #3406172: Provide a flag to allow updates to stored schema for fields as a smaller change to only introduce the column_changes_handled setting.

This still needs to move back to a hook_update_N() with a $settings opt-out per #126-128.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Add items form #128 to the remaining tasks.

Reassigned the change record to the dedicated issue that introduces the special flag only in #3406172: Provide a flag to allow updates to stored schema for fields.

Added a setting to allow skipping the update. The skip is done in the update function. Should it be in update_do_one instead?

I am not sure how to implement a timeout as asked for in #126. How to do that?

There is an item to add an item to the status report indicating if the timestamp fields have been fixed or not. Before adding that I'd like to know if the existing Error from the Entity system sufficient for this or not. Attached is a screenshot of those errors from a site where the update has not been run.

The skip is done in the update function. Should it be in update_do_one instead?

In the update function is fine I think, keeps the logic together.

The 'mismatched field definitions' status report items seems plenty to me - sites will only get that if they explicitly skipped the update, so they ought to be able to put two and two together.

made an attempt at adding a timeout.

Updated CR slightly to include additions to the default.settings.php

Looking at the open threads appears @quietone has addressed them all.

Applying MR locally update hook runs fine.

Believe this one is finally ready

I've added some review comments to address. I think we can make the timeout handling a bit simpler. And we can inform the user better when things are skipped.

I have responded to all the feedback from alexpott. Time for another set of eyes.

Feedback appears to be addressed

Only question I have is the update hook does it need to be 10301 if going into 10.3? Does it matter?

This won't go back to 10.3, it might only get committed to 11.1 at this point.

So just update hook needs to be updated?

I didn't mean to set this to needs work, that was a mistake, but actually yes.

Also this is going to be soft-postponed on #3108658: Handling update path divergence between 11.x and 10.x - we can't commit it until that issue lands and might have to use the helper method from that issue in this one, in case we ever did backport it to 10.4 for some reason.

Sounds good.

Adding issue this is postponed on to the issue summary per, https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

The update path divergence issue just landed so this is unpostponed, but it will need to implement the new API (even if we only commit it to 11.1.x) so moving to needs work.

I am assuming this is for the next minor only, 11.1.0. Therefore changed to system_update_11001 with no equivalent update.

Time for reviews again.

Since this was previously RTBC before the blocker, and the one update hook version updated. All threads were previously reviewed. So going to re-mark it.

I re-reviewed and have one question on the MR still, looks RTBC to me apart from that though.

Testing out if we can remove that line, if it breaks I'll revert back

Also IDE picked up a variable that was never used so removed that too.

So I tried by removing the cache clear and it caused a few test failures https://git.drupalcode.org/issue/drupal-2885413/-/pipelines/262767

Added back and back to green so appears it's needed.

Those test failures look unrelated to me? https://git.drupalcode.org/issue/drupal-2885413/-/jobs/2534394 is FileDownloadTest which doesn't involve the update path, and the other one I saw was Nightwatch. Which relevant tests are failing?

Swear thought I re-ran it but guess I was wrong.

Thanks, looks like we can just do without that - cache will be cleared after running updates anyway.

Committed/pushed to 11.x, thanks!

We still need to decide whether we're going to backport this to 10.4.x.

The advantage of backporting is that sites would be able to get this update over and done with before they need to update from 10.4 to 11.2 or something like that. The disadvantage is that if they have a massive site, it could be unexpected to get such a long running update in a minor release.

Added a release note to https://docs.google.com/document/d/1Df_zLpRIoM44pq4vVGxjPPA6G0m0wgocIAly...

Published the CR.

The change records states that the default value for timestamp_field_update_y2038 is TRUE, which would make sense for most sites. But in system_update_11001 it uses the default FALSE, which means that the update will be skipped.

@jurgenhaas I think it's OK, but it relies on a double negative:

function system_update_11001(&$sandbox) {
  // Execute only if allowed setting, 'timestamp_field_update_y2038', is true.
  if (!Settings::get('timestamp_field_update_y2038', FALSE)) {
    return t("Update '11001 - Update TimestampItem field schema size to support dates greater than 2038' skipped due to setting of                              'timestamp_field_update_y2038' in settings.php");
  }

We could probably change that to

  if (Settings::get('timestamp_field_update_y2038', TRUE)

I think the reason it uses the double negative is because we never expect anyone to actually set $settings[''timestamp_field_update_y2038'] = TRUE but only to FALSE if they do something custom.

@catch I'm not sure, there is a double negative here.

Settings::get('timestamp_field_update_y2038', FALSE)

This returns FALSE if timestamp_field_update_y2038 is not set in the settings.php, which is what we have in all Drupal installations by default.

Because of that, this if-statement is giving us TRUE:

if (!Settings::get('timestamp_field_update_y2038', FALSE)) {

And therefore, it outputs the message, exits from that update hook and Drupal marks the schema as if the hook was executed. But it wasn't.

I believe, the correct statement should be this:

if (!Settings::get('timestamp_field_update_y2038', TRUE)) {
    return t("Update '11001 - Update TimestampItem field schema size to support dates greater than 2038' skipped due to setting of                              'timestamp_field_update_y2038' in settings.php");
  }

@jurgenhaas yes you're right - pushed a new branch. https://git.drupalcode.org/project/drupal/-/merge_requests/9349

I don't understand why the tests are passing with the wrong logic though.

Went ahead and pushed the follow-up since it's a trivial change, but we should improve the test coverage here before a 10.4.x backport.

I don't understand why the tests are passing with the wrong logic though.

The tests seem to be testing the schema. Since the test is running in a Drupal site which got installed with the latest schema already, there is no need to execute system_update_11001, I guess. So that is never tested, but all the rest of it is.

@catch the commit from #164 would not fix the issue. It should be this:

if (Settings::get('timestamp_field_update_y2038', TRUE) === FALSE) {

Y2038SchemaUpdateTest explicitly tests the before/after so ought to have picked this up.

Oof you're right, default needs to be changed for sites that don't have anything set. edit: pushed another follow-up commit.

The backport is fails on this update when run in test, TimestampFormatterSettingsUpdateTest.php.

Drupal\Tests\system\Functional\Update\TimestampFormatterSettingsUpdateTest::testPostUpdateTimestampFormatter
The update failed with the following message: "Failed: Drupal\Core\Database\SchemaObjectDoesNotExistException: Cannot change the definition of field 'node__field_foo.field_foo_value': field doesn't exist. in Drupal\mysql\Driver\Database\mysql\Schema->changeField() (line 632 of core/modules/mysql/src/Driver/Database/mysql/Schema.php)."

core/tests/Drupal/Tests/UpdatePathTestTrait.php:61
core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:206
core/modules/system/tests/src/Functional/Update/TimestampFormatterSettingsUpdateTest.php:60
vendor/phpunit/phpunit/src/Framework/TestResult.php:729

Storage is created for field 'foo' in, drupal.timestamp-formatter-settings-2921810.php, but not an instance.

Re #172This is valid. A storage can be around without instances. There is an option on field storages to persist even if no instances exist. We do this for the body field for example. It is rarely used but it is possible. So the code should cope with this.

Re #172 and #173 that sounds like a real bug, we could be more defensive and check the column exists before trying to change it, but it would be better to filter out storages without instances before we get there.

Went ahead and reverted the three 11.x commits here so we can fix that issue in 11.x first, then backport it all together to 10.4.x.

Added testing of skipping the update as well as testing with a field that does not have a database column. The test first tries to skip the update but that fails with

1) Drupal\Tests\system\Functional\Update\Y2038SchemaUpdateTest::testUpdate
Content block: The <em class="placeholder">Revision create time</em> field needs to be updated.

because \Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem::schema has changed but not the database columns. This is the same type of error given when running the update with a field that does not have database columns.

What are your ideas on how to proceed?

reroll #98 for 10.3.x

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The CR for this was still published. Unpublished and added a release note snippet

Just been bitten by the Y2038 bug by a credit card expiry date being beyond the INT range. Using Drupal Commerce - fully up to date - and Drupal core 10.5.6. Will the patch on #179 work until we see this in core?

#188 shows that sites are already getting bitten by this even though 2038 is 13 years away. This would be good to get done for 11.4/12.0 so adding the release priority tag.

We've also encountered this bug. With the Acquia DAM (acquia_dam) module, we cannot utilize assets that have expiration dates set beyond 2038, which impacts many of them, as our use case often requires assets to expire in 2040.

@marcos_lima while this is a core bug, I'd be interested to know if you've filed a support request with Acquia DAM considering it's a paid subscription service. This issue nearly got fixed once and only needs a bit more help to get fixed properly, but the people who most recently pushed it forward are quite overstretched.