Base field purging is not handling translatable fields correctly

This is happening to me and it's resulting in content_moderation not being removable. After applying the patch I was able to uninstall content_moderation.

Thanks for filing this bug report. Bug fixing is very valuable. However in order in order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

1. https://www.drupal.org/docs/8/testing
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.7.x

Adds patch with failing test.

I don't know why these tests are passing. As far as I can tell the test runner isn't even running the test I added in the patch. Not sure what's going on, and I'm out of ideas. Test definitely fails for me locally though.

Bump on this? Seems like something we'd like to get in and failing/passing tests are created. If there's anything else needed to get this one committed let me know.

Looking at the query generated by \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSelectQueryForFieldStorageDeletion() from the issue summary, the problem seems to be that we're trying to populate the (deleted) revision field table with records from the data table, so that's what we need to fix :)

#12 fixed the issue for me (on 8.8) - thanks!

Bumping to critical and marking #3043650: Exception when uninstalling CT when there are existing translations as duplicate.

I looked a bit into this and it turns out there are two problems there:

1) the JOIN needs a langcode condition, as correctly discovered by @bgreco in the issue summary
2) the JOIN shouldn't exist when the two table names are the same (as mentioned in #15), i.e. when the entity type is revisionable but the field storage is non-revisionable => the field is stored only in the data table so we don't need to join it to retrieve the value for the bundle field column

Added two new test cases to \Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest::baseFieldDeleteWithExistingDataTestCases() that cover both scenarios. I also had to refactor testBaseFieldDeleteWithExistingData() a bit to allow a field to be made translatable, but in the end I think that whole test method is a bit more clear and easier to follow now.

Looks good to me!

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -840,8 +840,18 @@ protected function getSelectQueryForFieldStorageDeletion($table_name, array $sha
   +        if ($langcode = $this->entityType->getKey('langcode')) {
   

   We should use $this->entityType->isTranslatable() here: entity types can be language-aware (and thus have a langcode key) without necessarily being translatable.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -504,18 +505,22 @@ public function testBundleFieldCreateDeleteWithExistingEntities() {
   -  public function testBaseFieldDeleteWithExistingData($entity_type_id, $create_entity_revision, $base_field_revisionable) {
   +  public function testBaseFieldDeleteWithExistingData($entity_type_id, $create_entity_revision, $base_field_revisionable, $create_entity_translation, $base_field_translatable) {
   

   It seems that $create_entity_translation and $base_field_translatable have always an equal value. Do we really need both?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -504,18 +505,22 @@ public function testBundleFieldCreateDeleteWithExistingEntities() {
   +    // Enable an additional language.
   +    ConfigurableLanguage::createFromLangcode('ro')->save();
   

   Can we move this into the if ($create_entity_translation) { block?

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -526,10 +531,22 @@ public function testBaseFieldDeleteWithExistingData($entity_type_id, $create_ent
   +      $entity->isDefaultRevision(FALSE);
   

   Is this useful to check that the revision table is actually joined when it needs to?

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -626,45 +659,75 @@ public function testBaseFieldDeleteWithExistingData($entity_type_id, $create_ent
   -      'Revisionable entity type, non revisionable base field' => [
   +      'Revisionable, non-translatable entity type, non revisionable base field' => [
            'entity_test_mulrev',
            TRUE,
            FALSE,
   +        FALSE,
   +        FALSE,
          ],
   -      'Revisionable entity type, revisionable base field' => [
   +      'Revisionable, non-translatable entity type, revisionable base field' => [
            'entity_test_mulrev',
            TRUE,
            TRUE,
   +        FALSE,
   +        FALSE,
          ],
          'Non-translatable revisionable entity type, revisionable base field' => [
            'entity_test_rev',
            TRUE,
            TRUE,
   +        FALSE,
   +        FALSE,
          ],
          'Non-translatable revisionable entity type, non-revisionable base field' => [
            'entity_test_rev',
            TRUE,
            FALSE,
   +        FALSE,
   +        FALSE,
   +      ],
   

   It seems these cases are the same, aside from the entity type/schema. Maybe we should clarify it in the test case names?

Thanks for the review :)

Re #23:

1. Done!
2. We don't really need both, I wanted to be consistent with the other two arguments, but we can always add another one if we find the need for it, so removed the second one.
3. We can't :( This needs to be done before the first entity save, which happens just above the if ($create_entity_translation) { block.
4. Nope.. I did that so I can keep the order of the assertions consistent, i.e. to always have the values for the first revision in the data tables. If we would create a new default revision, the assertions below in that method would have to check the values for either the first or second revision, depending whether the test is creating a new revision or not.
5. Good point, a couple of test case names were even wrong, so I updated all of them... I hope they're clear enough now :)

Hey everyone,

I just confirmed this patch works on an 8.9.x install, where I had to uninstall "search_api_exclude" module, which defines a basefield.

Well done!

Patch in #24 addressed the error I had with uninstalling modules with base field definitions in multilingual sites.

We need a patch for 9.1.x before we can backport this to the other branches. Other than that this fix looks good.

Rerolled the patch for 9.1.x. #24 still applies on 8.9.x, but I'm re-uploading it anyway to have them both in the same comment.

Committed and pushed d97a2dc030 to 9.1.x and 31f278d2fb to 9.0.x. Thanks!
Committed 7727f4f and pushed to 8.9.x. Thanks!