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 Needs review 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 Needs review .

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.