system update 10100 cache tables fail

How did this happen?

I've updated this to check if the table has an expire column. I got this error on a site which has a custom table with a name starting with cache_ but it does not have an expire column. It is used as a cache which never expires.

I left DiDebru's check there but I don't really see how the table could not exist. I think that would give a different error message.

We still would need steps that caused this to happen. I did a 9.5 to 10.1 update just yesterday funny enough but did not hit this error. I hit a separate issue and turned out one of the custom modules had a problem. So wonder if that's the case here too.

I added some steps to reproduce.

It was probably a bad choice of custom table name some time ago but I don't think it is set in stone that any table beginning with cache_ must have an expire column. That is what system_update_10100 assumes.

If an issue caused by core and not a custom module will need a test case also.

Our team ran into this issue and it hung after using the above patch. After looking into the update hook a bit further we compared changeField to watchdog's use of changefield and noticed a difference.

We changed

$schema->changeField('sessions', 'timestamp', 'timestamp', $new);

to

$connection->schema()->changeField('sessions', 'timestamp', 'timestamp', $new);

Attached is a patch for 10.1.6. I couldn't grab the branch that the issue was applied to, but at least our patch is here for others to bypass this problem.

I wanted to repost that our issue was more to do with the size of the tables. In our case the size of our queue tables were substantial, but after truncating the tables we were able to upgrade with no issues.

I don't think this needs explicit test coverage - we would have to make an additional database dump, with a custom/bogus cache_something table, then update that dump in the update test, to show that we don't get an error. Since we already test the update without such a table, we have implicit test coverage of the if statements in the usual case anyway.

However this could use the patch in #5 converted to an MR, and if possible I'd like to get this committed before 10.2.0's release next week.

I think we could build the list of cache bins from the container - or maybe even get it from there. And then we wouldn't be potentially touch other tables.

Turns out #13 was optimistic because cache.container is not tagged and therefore not returned by Cache::getBins() - I'm pretty sure this is on purpose... but considering this is possible going for something simliar to #5 seems good.

Posted an MR with a test.

Just posting here from the MR that alexpott ran

1) Drupal\Tests\system\Functional\Update\Y2038TimestampUpdateTest::testUpdate
The update failed with the following message: "Failed: Drupal\Core\Database\SchemaObjectDoesNotExistException: Cannot change the definition of field 'cache_bogus.expire': field doesn't exist. in Drupal\mysql\Driver\Database\mysql\Schema->changeField() (line 615 of /builds/project/drupal/core/modules/mysql/src/Driver/Database/mysql/Schema.php)."
/builds/project/drupal/core/tests/Drupal/Tests/UpdatePathTestTrait.php:59
/builds/project/drupal/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:115
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 36, Failures: 1.

So test coverage appears to be there, applied the MR locally and can run the update just fine. Hope it can make it to 10.2

Committed/pushed to 11.x and 10..2.x, thanks!

10.1.x has had its last security release, so we need to decide what to do there a bit.

Cherry-picked to 10.1.x, we can figure out if/when to release later.