taxonomy_post_update_make_taxonomy_term_revisionable() and the menu link content equivalent fail when entities have no default translation

I ran into similar issue when upgrading from 8.6.16 to 8.7.8.
I found out that there were 3 fields on a vocabulary which were causing the issues in this function preUpdateEntityTypeSchema() in SqlContentEntityStorageSchema.php on line 468:
$temporary_table_names = array_combine($sandbox['new_table_mapping']->getTableNames(), $sandbox['temporary_table_mapping']->getTableNames());

The tables from $sandbox['new_table_mapping']->getTableNames() (argument 1) should match with $sandbox['temporary_table_mapping']->getTableNames()(argument 2) which was different in our case, So I need to figure out for which tables temporary_table_mapping is not able to generate revision tables. we didn't have any content for those fields so we deleted the fields, but if you have content in the field tables then you need to figure out why revision tables are not generated.

> This isn't enforced by the 8.6.x schema and it looks like potentially devel generate can create terms like this (going from reports below, not reproduced).

It is, though. Trying to create a term with a raw insert query based on the 8.0 filled dumps results in:

Drupal\Core\Database\IntegrityConstraintViolationException : SQLSTATE[23000]: Integrity constraint violation: 19 NOT NULL constraint failed: taxonomy_term_field_data.name: INSERT INTO {taxonomy_term_field_data} (tid, vid, langcode, name, weight, changed) VALUES (?, ?, ?, ?, ?, ?); Array
(
    [0] => 999
    [1] => tags
    [2] => en
    [3] => 
    [4] => 0
    [5] => 1579555999
)

Same with status really, although maybe combined with updating from a broken version when updating and coming from content_translation_status, you *might* have managed to end up with NULL. But I thought we learned our lesson from the block_content update and had considerably improved the initial value handling.

But actually, #3052464-107: Cannot update to 8.7.0 because of taxonomy_post_update_make_taxonomy_term_revisionable gave me a hint now at what might be going wrong. That reports that it can be fixed by setting default_language to 1. If I set my example data to default_language = 0 for single available translation, which obviously is not a valid situation then I can get it to fail on the update because it just doesn't know how to handle that data.

Basically, the data is then loaded in the wrong language key in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables(), I tried to fix that directly in there, but it fails weirdly in the update. Posting what I have so far, I'm very done for tonight.

Hm, I don't quite agree with that :)

The default_langcode column can be reliable derived, we have the default langcode also in the base field table, that's what my fix is doing now transparently on entity load.

That works for single and multi-language entities just fine, it would actually be harder to figure out if an entity has one or multiple translations because we need to see if there are multiple rows in the data table and if none of them have default_langcode set.

So a query to find them would be select b.nid,b.langcode from node b inner join node_field_data d on b.nid = d.nid and b.langcode = d.langcode and d.default_langcode = 0;. And you can do the same for the node_revision/node_field_revision tables. For mysql, you could probably even do that in an update query, but we don't support updates with joins in our API.

So an update function could loop over all translatable entity types with the default storage, run that query until we have no results left, and fix the default_langcode field for the ones where it doesn't match. We could also detect those records that have it incorrectly set on the wrong translation in theory.

That said, I still don't really understand how you can get in this situation in the first place, and I'm a bit wary of just fixing up the data without understanding the root cause, but in my opinion this shouldn't be possible through the API. *Maaaybe* with some weird migrations? A one-time data fix will not prevent it from happening again.

Tagging as a requirement for Drupal 9.0-beta1.

I had a few discussions about this with @Berdir, @amateescu and @catch.

TL;DR: We did not reach consensus on how to handle corrupt data yet.

We laid out a few possible solutions but it seems the main problem is that we do not agree on the "constraints " a valid solution should respect:

• @Berdir and @catch are ok with manipulating user data: either manually fixing corrupt records before running the update (@catch) or as a consequence of an entity save operation following a "fix" on load (@Berdir's patch).
• I'm not comfortable with manipulating user data but I am ok with optionally "leaving it behind": I suggested to add an opt-in update mode that would catch exceptions thrown while writing entity data to the new storage (more on this below).
• @amateescu thinks user data should be all manually fixed before running the update and suggests implementing a "dry-run" mode that would allow people to attempt the update as many times as needed to fix everything manually.

Here are the reasons behind my proposal:

• I'm not comfortable with the idea of manipulating user data: any change to it could alter the site behavior in an unexpected way, even if data can be reliably restored. If it was previously being filtered out because of joins or filters on default_langcode = 1 (which is a quite common filter), people might rely on it not showing up, which would be problematic in case of sensitive data, for instance.
• OTOH not copying over broken data would still give people the opportunity to manually recover it from backup tables, without forcing them not to update until every single case is tracked down/resolved.
• Removing broken data is exactly the solution users tried to make updates complete both here and over at #3052318-100: Update from 8.6 to 8.7 fails due to corrupt menu_link_content or taxonomy_term entity data. AAMOF this proposal would allow to "address" not only this issue, but any corrupt data issue, including menu link ones. It seems that non-technical site owners could deem losing some broken data worth compared to being stuck with an unsupported Drupal version.

As a side note, the "skip broken data" mode would be compatible with the "dry-run" mode @amateescu is suggesting: people wishing to fix everything before running the update would simply not enable it and instead use a dry run to identify all broken data and fix it however they see fit.

For these reasons I'm posting a patch implementing my solution. Happy to go other ways if consensus arises around one of the other solutions, but at least we have something else to evaluate besides #8.

Small fix for test failures.

Discussed this issue with @plach today, and my opinion is that skipping entities that couldn't to be written to the new storage is very risky, because no matter how many warnings we display at the end of the update process, there will be cases when people will ignore them and potentially end up with lost data.

IMO, the easiest and non-controversial solution that can be implemented quickly is a "data integrity checker" system, which would check various things like:

- field storages that are marked as required in the schema but have NULL values in the database (we already introduced something like this for comments in #3052147: comment_update_8701 fails if there are comments without field_name)
- entities which don't have a default translation
- etc.

This system would work very much like the entity definition change list (EntityDefinitionUpdateManager::getChangeSummary() and ::getChangeList()), and we would show them in the status report page, as well as raising update requirement errors if there any problems found.

@catch: On single vs. multiple translations, we discussed that before in #10/#11, we can reliable figure out the correct default_langcode value also for translated entities. But I'm fine with that being done in a separate issue.

I'm still not exactly sure how to move forward here then. IMHO, the only thing that we could check in advance is the known default_langcode problem, but if we're going to hopefully fix that then I'm not sure what we end up with here. There could be other data integrity issues, but we don't even really know what they could be, so how could we detect for that apart from doing a full-blown dry-run of resaving all terms, which is a very time-consuming operation, I'm not sure where and how we'd do that.

Pretty much the only idea I still have is to improve the error that we display, point to a documentation page where we collect information on known issues and how to fix them manually. Still the user would have a half-updated database and would need to restore a backup?

The generic data-integrity-check-and-fix thing is interesting, but I don't really see how that would work for core. We wouldn't only need to backport it to 8.8, we would need to backport to 8.7, 8.6, 8.5, to help those stuck on those versions? Seems like it would make more sense to create that as a contrib module that these sites can install and fix their installation before updating to 8.8+.

I can live with #25 :)

I will likely work on this tomorrow, if none beats me to it.

On this

Here's a first attempt at implementing #25. If it looks good I will work on adding the menu link equivalent.

Expanding the scope of the title.

Yep, I'll look into them

Would be nice to link to the term here if we could, but is that getting risky to do during an update?

That would require a term load, I was trying to keep the process as lightweight as possible.

I agree with #36. We can mention the IDs and give them the instructions for fixing them in the CR.

This addresses #31.1 and should fix the test failures. Working on the menu link stuff.

Yep, I had some unwanted changes around due to branch switching and fs permissions. I'll get rid of those in the next patch.

This gets rid of the cruft that caused the last failure in #38 and adds support for custom menu links.

Unassigning in case someone else wishes to work on this while I'm asleep ;)

PHP docs/comment fixes + test-only patch.

+++ b/core/modules/menu_link_content/menu_link_content.post_update.php
@@ -99,5 +110,101 @@ function menu_link_content_post_update_make_menu_link_content_revisionable(&$san
+  $query->addExpression("MIN(b.{$id_key})", 'id');
+  $query->addExpression("MIN(b.{$langcode_key})", 'langcode');

I tend to call that pseudo-generic code, using the variable names here is imho longer and more complicated than just hardcoding id, especially since it's hardcoded just a few lines above anyway. Don't really care that much though.

Are we sure that using an alias with the same name as the column isn't going to cause an issue on some database or so?

I wonder if it's really faster to put both things (it being 0 for the default langcode and no other row having 1) in the same query, requiring group by/having as opposed to checking that when we update them as an additional query. I'm assuming that even sites that are affected by this only have a handful of rows affected by this, but maybe that's not true.

+++ b/core/modules/menu_link_content/menu_link_content.post_update.php
@@ -6,12 +6,23 @@
  */
 function menu_link_content_post_update_make_menu_link_content_revisionable(&$sandbox) {
+  $finished = _menu_link_content_post_update_make_menu_link_content_revisionable__fix_default_langcode($sandbox);
+  if (!$finished) {
+    $sandbox['#finished'] = 0;
+    return NULL;
+  }
+

Wouldn't it be easier to make this a regular update function? That's guaranteed to be called first then, and we're only doing raw database stuff, which should make it a safe operation?

Are we sure that using an alias with the same name as the column isn't going to cause an issue on some database or so?

This is a good question. I queued a few more.

Working on this again

@Berdir:

This should address #44 :)

Minor PHP doc tweak

Myself, from a Slack thread with @catch and @Berdir:

- I think the only possible concern is the schema inconsistency mentioned in #3
- @catch happy to revert to a single post update, if that’s preferable
- on a second thought, it’s not ok if this update runs on a revisionable schema because revision tables would not be cleaned up
- so, either we make it support also revision tables or we bundle it back into the post-update IMO
- OTOH if the update run successfully before this, there’s likely nothing to clean up
- :bomb-boom:
- +1 on bundling back into the post update to avoid introducing real mess for what is likely a very minor use case

And the usual missing bit...

Only have a nitpick on the assert method, looks pretty good to me now. The query makes sense to me, and going back to the "nested post update" approach is tricky but avoids the problems that were pointed out in #50 as discussed.

1. +++ b/core/modules/menu_link_content/tests/src/Functional/Update/MenuLinkContentUpdateTest.php
   @@ -101,6 +120,35 @@ public function testConversionToRevisionable() {
   +    $this->assertIdentical($title ?: '', $expected_title);
   

   Should use assertSame with reversed arguments.

2. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyTermUpdatePathTest.php
   @@ -207,6 +226,34 @@ public function testConversionToRevisionable() {
   +    $this->assertIdentical($name ?: '', $expected_name);
   

   same.

Addressed #54 1, 2.

Sorry, wrong interdiff file extension at #55, Tests cancelled. upload again.

Interdiff looks good to me, thanks!

RTBC +1

Ok, lets get this done then.

Hopefully this really is the issue that people have been struggling with in regards to those updates and not something else, so far I think nobody with an actually affected site confirmed or disproved that. This is the only possibly issue we identified so far. I suspect that many who were active enough to post in the issue found some manual workaround by now, but there might be those who are following the issue and are waiting on this to be resolved.

I was involved in identifying what the problem seems to be and posted some patches here, but that was a completely different approach, so I think it's OK if I RTBC it. AFAIK @catch also didn't have any other remarks left that would block this.

Added a dedicated CR for this and a release notes snippet.

Working on the backport

Here it is

Green, back to RTBC