Updating an entity type from non-revisionable to revisionable fails if it has non-revisionable fields stored in dedicated tables

I came across this issue while trying to solve #3061950: SQLContentEntityStorage::saveToDedicatedTables() doesn't correctly use storage configuration. A similar issue was presented, which was the result of SQLContentEntityStorage::saveToDedicatedTables() using the incorrect field definitions while saving field data. The attached issue's patch may offer some assistance here.

The problem here is that the documentation for hook_entity_field_storage_info() doesn't say that the field storage definitions returned by that hook also have to run through the regular field storage life-cycle operations via the field storage definition listener (FieldStorageDefinitionListenerInterface::onFieldStorageDefinitionCreate(), ::onFieldStorageDefinitionUpdate() and ::onFieldStorageDefinitionDelete()). For example, configurable fields are doing it in these methods: FieldStorageConfig::preSaveNew(), ::preSaveUpdated() and ::postDelete().

Without doing that, the field storage definition is not tracked by the last installed schema repository so the new entity definition update API can't handle it.

My guess is that people who implemented this hook in custom code didn't think about these additional details (and rightfully so, since it's not documented anywhere), and that's why we've had such a big number of reports for broken updates to 8.7.0 :(

Looks like this is similar to this #3075774: Consider replacing EntityLastInstalledSchemaRepositoryInterface with EntityFieldManager in taxonomy_post_update_make_taxonomy_term_revisionable

My guess is that people who implemented this hook in custom code didn't think about these additional details (and rightfully so, since it's not documented anywhere), and that's why we've had such a big number of reports for broken updates to 8.7.0 :(

Do you have concrete proposals on how to proceed with this?

The only thing I can think of right now is to update the contrib modules to not use the hook in this way, but of course that still won't help custom modules :(

Tagging as a requirement for Drupal 9.0-beta1.

Could this be handled via a documentation improvement and a change notice?

So, something like this?

(skipped the test run because it's just a comment change)

.

After investigating this, I found out that this is definitely not a documentation issue: it's a straight bug caused by #3113639: The default table mapping and the SQL storage do not agree on whether non-revisionable bundle fields should get a revision table.

Currently when you have a revisionable entity type with a non revisionable storage definition, the table mapping thinks there should be no dedicated revision table, while the storage and storage schema handlers will expect it to be there and try to read from/wrote to it. Since the table mapping is used to generate the temporary schema, the system ends up missing the revision table when it starts copying data.

Moreover I realized that, despite missing docs, it’s very unlikely that someone misses the installation step, because you can no longer load or save an entity if a field table is missing: you get exceptions all over the place. Adding documentation is still a Good Thing, though :)

This should fix the issue. Working on test coverage now.

.

This should be ready for review, let's see whether the bot agrees.

And here is a test-only patch.

.

This provides some docs improvements and should hopefully be ready to go, reviews welcome :)

Just documentation nit picks, otherwise it looks good.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -615,15 +622,46 @@ protected function postUpdateEntityTypeSchema(EntityTypeInterface $entity_type,
+   * Gets a list of table names for this entity type, field storage and mapping.
...
+   * The table mapping may not return a dedicated revision table, if the field
+   * is not revisionable.

These are indicating only a single field, however multiple field storage definitions are passed. Also the comment is referring to the table mapping only, but should also indicate that this what the current method is "compensating" for.

Thanks, this should be better.

Much better, thank you.

Updated IS

I was reviewing this on Friday and got distracted, and it has since been marked RTBC. Chiming in to say that the patch looks great! Thanks all.
Updated credit field, please do not credit me!

@catch:

Let's say those docs improvements are not strictly needed to fix this issue, but IMO they don't hurt, since they should have been in place already. We could split them to a separate issue, but I'm not sure it's worth.

Reopening to discuss an 8.7.x backport.

Here's a backport for 8.7.

Green, back to RTBC.