Timestamp field items are affected by 2038 bug

Trying to resurrect this at Drupal Global Contribution Weekend.

Applying and running the test locally, to see what can be done.

Re-testing #93. Also adding other DB types to validate this is not a DB type and we haven't tested this patch against other DB types for almost 1 year, so to create a newer baseline.

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 really - 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 issue with the tests.

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.

The last submitted patch, 111: 2885413-111.patch, failed testing. View results →

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: 
            
              
              
              ✨
              Allow altering the post_update run order
                Needs work
              
            

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 ✨ Provide a flag to allow updates to stored schema for fields Fixed 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 ✨ Provide a flag to allow updates to stored schema for fields Fixed .

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 📌 Handling update path divergence between 11.x and 10.x Fixed - 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.

catch → closed merge request !4441

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.

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

quietone → changed the visibility of the branch 2885413-hotfix to hidden.

quietone → changed the visibility of the branch 2885413-hotfix to active.

quietone → changed the visibility of the branch 2885413-11.x to active.

quietone → reopened merge request !4441

quietone → changed the visibility of the branch 2885413-11.x to hidden.