[PP-1] Disallow removing a language when there is content in that language

HI,
Switched on 8.3.x and checked the file : /core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php

  if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
            $view_langcode = $entity->language()->getId();
          }

The condition $entity->isTranslatable() is already present, that should be enough? We need to add some extra check here?
I am working on this and will attach a patch.

Thanks

I have added an extra condition for default translation check in the patch.

I'm trying to reproduce the issue above on Drupal 8.4.x-dev, but I don't see an error.

I followed the steps below:

1. Enable all multilingual modules
2. Go to /admin/config/regional/language
3. Click Add Language
4. Select French and click Add Language
5. Go to /admin/config/regional/content-language
6. Select Content and then Basic page and click Save Configuration
7. Go to /node/add/page
8. Fill in Title and Body and select English
9. Click the Translate tab
10. Add a French Translation
11. Change the text and click Save
12. Test that /admin/content works without an error
13. Go to /admin/config/regional/language
14. Next to French, select Delete and then confirm
15. I go to /admin/content

Not sure if removing a language if you still have content in that language is something we should even allow. Like other things, it leads to a mess and inconsistent data. Of course, we do need to allow to actually remove all translations then.

Thanks @pixelite for helping triage this issue. Adding issue credit for the triage.

Let's rescope this to what @Berdir and @ndlarge have suggested. Also moving the issue to 8.4.x since such a change could be disruptive for usecases that expect to be able to remove languages when there is already content in them. (I couldn't think of a reasonable one but people do all kinds of things so we should avoid breaking it in a patch release.)

(Just triaging...)

Comment from Gábor Hojtsy in duplicate #2540876-16: WSOD with listing content after removing language:

we need a system first to identify all "content" (taxonomy terms, users, products, rules, views, etc.) in the language you are deleting. Introducing such a system would be the next step.

I am experiencing this issue now. I applied patch #6 and it caused another error:
Error: Call to a member function getId() on null in Drupal\views\Plugin\views\field\BulkForm->calculateEntityBulkFormKey() (line 456 of core/modules/views/src/Plugin/views/field/BulkForm.php).

In my case, a language was removed via configuration import and caused this. Not sure if this need a configuration management tag too.

Hi,
This patch will disallow users to delete the language if the translations are available in the corresponding language.

Suggestion: We can warn users that deleting the language will delete all translations for the corresponding language. If the user confirms to proceed, delete the translations and then the language.

at the moment there is a message that deleting a language will set the content to language netural if you try to delete the content from the administravie form. But if the language gets removed during configuration import, I believe the clean up does not run.

The current patch only considers the "node" entity type, but instead we need to iterate over each translatable content entity type and check if there is an entity having such a translation.

Instead of loading all the entities we could perform an entity query to check if there is an entity having a translation with the langcode of the language we attempt to remove.

Additionally what about letting the form to be built by the parent and then set the access to the submit button to FALSE?

Something like this:

$langcode = $this->entity->getId();
$entity_type_manager = \Drupal::entityTypeManager();
$used_by = [];
foreach ($entity_type_manager->getDefinitions() as $entity_type_id => $entity_type) {
  if ($entity_type instanceof ContentEntityType && $entity_type->isTranslatable()) {
    $results = $entity_type_manager->getStorage($entity_type_id)->getQuery()
      ->condition($entity_type->getKey('langcode'), $langcode)
      ->allRevisions()
      ->execute();
    if ($results) {
      $used_by[] = $entity_type->getLabel();
    }
  }
}

This when deleting the entity through the entity Form API, but language entities might be removed through the entity API as well. I think we need to prevent the deletion also in this case.

@hchonov does this take into account deleting a language via configuration import? If an additional language was added via the UI and then a a configuration synchronization was performed (drush cim) the new language will be removed and the content will remain. Is my understanding correct? if that's the case is there a way to prevent the language from being removed during synchronization ?

I am attempting to document this process for a stop gap solution before the process is resolved in core. I ran into this issue yesterday and the solution (currently) seems to be a bit vague. Here is my first kick at the process:

Of the language to be removed...

1. Delete translated nodes.
2. Delete translations from menu items.
3. Delete translations from Views.
4. Delete translations from string overrides (string translations).
5. Delete the language from the region languages page.

This isn't complete. What should be added?

the same problem after deleted languages

after adding the deleted languages back, the content admin view came back.

I've tried creating an entity with a non-existent language and then retrieve that language after entity save.

>>> $entity = entity_create("node", ["type" => "article", "title" => "testTitle", "langcode" => "bla"]);
>>> $entity->language()->getId();
=> "en"
>>> $entity->save();
=> 1
>>> $entity->language()->getId();
=> "en"
>>> $entity = entity_load("node", $entity->id(), TRUE);
>>> $entity->language()->getId();
=> "bla"

Currently it is possible to create an entity with non-existent language and retrieve that language without any errors. If we want to prevent removing a language if there are existent content entities referencing it, then we will also have to ensure that we cannot create an entity referencing a non-existent language through the entity API.

This "hack" works only for the default language. If we try to add a new translation for a non-existent language, then an exception is thrown:

>>> $entity->addTranslation("foo", []);
InvalidArgumentException with message 'Invalid translation language (foo) specified.'

The same exception should be thrown on creating an entity with an invalid default langcode.

About the inconsistency shown in the first code example: an entity is created for the invalid langcode "bla", but $entity->language()->getId() returns "en". I am not going to open a new issue, because if we prevent creating an entity with an invalid langcode, then the inconsistency will disappear.

The patch is based on the code from comment #23.

I've thought of one more use case - we might have Entity Reference fields

+++ b/core/modules/language/src/Form/LanguageDeleteForm.php
@@ -11,6 +13,38 @@
+        $results = $query->execute();

This should become a count query by simply adding "->count()". We would also need a test :).

What about Entity Reference fields? Why do you think they add more necessary checks?
What is more efficient, count() or range(0, 1)?

This is also related to path aliases, as they could be generated FOR a language.
When it's deleted, they will be left hanging.
I've landed here after having this on a project #3042390: Exception on node delete..

The root cause was due to deleted languages...

+1 on #11 and #14: preventing language deletion when translated data is present sounds definitely consistent with how we handle config dependencies elsewhere. For instance we do not allow to remove config entities acting as bundles, if they have content entity field data referencing them, because field data becomes invalid in that case.

The patch in #31 looks promising to me, although I'd add a ->range(0, 1) condition to make it more performant and disable access checks via ->accessCheck(FALSE). We should also implement config dependency validation (see ConfigImportValidateEventSubscriberBase), rather than addressing this only at form level, for the reasons outlined in #25 and #26. We should abstract out the code performing the actual check, so that it can be reused in both places.

I'm not sure about #30 TBH: you can create/save/load nodes referencing an invalid/non-existing content type. I'm not sure whether that's an oversight or whether that's "by design" to allow low-level entity data manipulation. Whatever route we take, I think we should be consistent here.

Finally, I agree with @Berdir that not having a way to bulk delete translations makes deleting languages more cumbersome after this change, however that does not sound like a hard blocker to me. At very least we should have a follow-up to address this, though.

As suggested, range(0, 1) and accessCheck(FALSE) have been added. Existing tests will fail without adapting to the form changes.

@plach for translations clean-up there's #2925532: Provide bulk action to delete all old revisions of an entity

Not sure it makes sense to ask query all translatable entity types, I think it could early exit when first entity found

+++ b/core/modules/language/src/Form/LanguageDeleteForm.php
@@ -11,6 +13,38 @@
+    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
+      if ($entity_type instanceof ContentEntityType && $entity_type->isTranslatable()) {
...
+    if (!empty($used_by)) {
+      $caption = $this->t('The %language (%langcode) language is used by some content entities (%usage) on your site. You can not remove this language until you have removed such content.', ['%language' => $this->entity->label(), '%langcode' => $langcode, '%usage' => implode(', ', $used_by)]);
+      $form['#title'] = $this->getQuestion();
+      $form['#markup'] = $caption;
+      return $form;
...
+    return parent::buildForm($form, $form_state);

It should use `#type => list' inspead of plain string

Here's what I use

@sam-elayyoub, the status "Patch (to be ported)" is wrong. Please take a look at the page https://www.drupal.org/issue-queue/status

The issue is still needs work, because the latest patch does not switch the default translation - when it is removed then some other translation should become the default one. However a lot of things have been discussed already, which need to be considered.

I have modified the failed tests by deleting the languages through backend and attached the patch.

@sathish.redcrackle Please change status to Needs Review after patch apply.

Just ran into this issue myself, a simple cron run fixed the issue for anyone else this may help.

@astoker88 Running cron didn't work for me.

Patches for 8.9 are not doing anything (Drupal 8.9.2).

Have 4 languages where Dutch (NL) is the source language, FR translation added. When trying to delete the translation FR, get still warning both the source language NL and FR will get deleted.

There's still a bug in Drupal 9.2.x

Steps to reproduce:
- Install Umami
- Add an additional language
- Add a translation (in the new language you added) to an existing recipe
- Delete the language you added
- Go to /admin/content

Disallowing removing a language while there's content in that language would "fix" that bug.

Updated IS

This is blocking #1870454: [PP-1] The "delete" operation for the added languages is scary/confusing.

Rerolled patch in #48, thanks!

Fixed custom code failure at #59. Please review

Looks good, just a few nitpicks:

1. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -14,6 +16,38 @@ class LanguageDeleteForm extends EntityDeleteForm {
   +      if ($entity_type instanceof ContentEntityType && $entity_type->isTranslatable()) {
   

   I would use getGroup() == 'content' instead of checking instanceof.

2. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -14,6 +16,38 @@ class LanguageDeleteForm extends EntityDeleteForm {
   +        $query = $this->entityTypeManager->getStorage($entity_type_id)->getQuery()
   +          ->condition($entity_type->getKey('langcode'), $langcode)->accessCheck(FALSE);
   

   Nitpick: the line layout here is weird. I'd put each fluent call on the query on its own line.

   Or assign query. Then do fluent calls on $query, to match the later calls on it.

3. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -14,6 +16,38 @@ class LanguageDeleteForm extends EntityDeleteForm {
   +      $caption = $this->t('The %language (%langcode) language is used by some content entities (%usage) on your site. You can not remove this language until you have removed such content.', ['%language' => $this->entity->label(), '%langcode' => $langcode, '%usage' => implode(', ', $used_by)]);
   

   Nitpick: line break after the [ and each translation variable on its own line.

I would use getGroup() == 'content' instead of checking instanceof.

Just being curious, can you explain why? For me personally, I had never heard of getGroup(), and "group" is such a generic word that I couldn't guess what it means or what its return value represents. Searching in Drupal core I was able to find \Drupal\Core\Entity\EntityTypeInterface::getGroup, but even then it is terribly difficult to find where the value is set to "content" (is it \Drupal\Core\Entity\Annotation\ContentEntityType::$group ?) and the documentation is minimal.

On the other hand, seeing instanceof instantly means something to me, even if I have never seen the code or the class name before. (That said, IMO it's even better to use instanceof against an interface, so if ($entity_type instanceof ContentEntityTypeInterface ...)

getGroup() could definitely do with better documentation (see #3107500: EntityTypeInterface::getGroup() doesn't explain what the group is), but it serves to determine whether an entity type is content or config. To me it reads better than checking for an interface.

Similarly, I prefer doing $entity->getEntityTypeId() == 'node' rather than checking for NodeInterface.

In both cases, it's using names of things rather than code structures, which to me seems easier to understand.

OK, thanks, I see your point. Still there are some good reasons to check if $foo instanceof SomeInterface (besides the documentation issue of getGroup() specifically).

First of all, I like the type safety it provides. In the line if ($entity_type instanceof ContentEntityType && $entity_type->isTranslatable()) {, the instance check directly guarantees on the language level that we can call $entity_type->isTranslatable(), without the risk of a "Call to undefined method" fatal error. (Maybe getGroup() == 'content' guarantees the same on the application level, but you need detailed knowledge of the application to be sure.)

Second, if you compare $entity->getEntityTypeId() == 'node' with $entity instanceof NodeInterface, the latter provides the flexibility for developers to replace the Node class with an extension (class SpecialNode extends Node) while keeping all the benefits of nodes. That's what interfaces are all about.

But that's just my €0.05, I will shut up now and stop derailing this issue.

The 8.9 version patch doesn't seem to fix the issue on Drupal 8.9.14.

Hello,

2851029-60.patch tested on Drupal 9.3.13 and working correctly.

In order to remove the language I still had to manually delete content such as user and node.

But also update user_field_data table fields: langcode, preferred_langcode and preferred_admin_langcode to another language.

And, delete node_field_revision table records that were using the langcode I wanted to delete.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

new MR available - continuing the patches efforts

- Create helper method languageUsedByContent($langcode) for ConfigurableLanguage entity class
Used the code from patch ... but "stop searching" on the first "usage"
- Use this method to "block" Language deletion if there is a content for the the language
- Use this method to disable the LanguageDeleteForm if there is a content for the the language
- Fix the tests - by updating the content language (user) before deletion of "default english language"
- Complete the tests - core/modules/language/tests/src/Functional/LanguageConfigurationTest.php
To tests what happens if the content still has language in use.

Questions:
Are there other tests needed?
Is "change record" required for this?

Change record I think so.

Question bunch of tests had to be updated with

    foreach (User::loadMultiple() as $account) {
      $account->set('langcode', LanguageInterface::LANGCODE_NOT_SPECIFIED)->save();
    }

Is that because it broke? Could this cause a regression for existing sites if so.

Question bunch of tests had to be updated with

    foreach (User::loadMultiple() as $account) {
      $account->set('langcode', LanguageInterface::LANGCODE_NOT_SPECIFIED)->save();
    }

Is that because it broke? Could this cause a regression for existing sites if so.

This is about, actually, what happens ... we could delete the language ... but then we'll have issues with the content - in the language - left behind.
No regression here - we just make sure the solution works properly - please @see LanguageConfigurationTest
https://git.drupalcode.org/project/drupal/-/blob/ca516d9b7e1275bce90d1bd....
We cover what happens without those deletions ...

About change record ... this more like functional update (fix) - not API update ... not sure what to specify there ...
any suggestion/example

Think it could be as simple as just mentioning that you can't delete a language now. Maybe a screenshot of the error

Unfortunately i can't get error ... maybe it's not there anymore ...

So, please, anyone could replicate the error from summary ... or confirm the error is ... no more.

however i noticed something like there will be dirty content ... and if want to clean up ... first will delete FIRST actually the content in the new language.
then i could delete the "source"

i'll try to upload some images that should explain the "dirty content"

i'll put on "Needs review" ... just to get some feedback

then we'll see about the change record

Following the steps from the issue summary I get the error.

Applying the MR And trying to repeat the steps I get

Started the CR https://www.drupal.org/node/3367094

Sorry I didn't notice before but the proposed solution is TBD, that will have to be updated before being RTBC.

updated the summary ... i hope we're close ...

Lets get this in committers eyes

Good job!

I would normally agree with preventing deletion of things that leaves the site in an inconsistent state but am less sure here.

If I have one node, where the original language is English and French, and I want to remove French from my site entirely, then I get this message - how do I go about removing the French translation from the node, not only from the most recent version, but also every previous revision?

@catch would imagine you would need to do some bulk deletion for the language you are trying to delete. See as like nodes or paragraphs. You can't delete the bundles until the content is gone.

@catch do you mean like point the user to content that needs to be deleted?

if this is the case:
What if there are like thousands of content entities ... and not only node, from users to custom content entities?

Or do you mean, like a "button" that does this?
Here i would say it could a follow-up

So please, details a little "your desires".

For now put back to RTBC ... as there is no clear request of what's needed to be worked on ... imho

@vasike I mean that as far as I know, there's no way to remove old translation content.

For example:

1. Install Umami
2. Edit the spanish translation of node/9 a couple of times, your node_revision__field_summary table should look something like this:

MariaDB [db]> SELECT * FROM node_revision__field_summary WHERE entity_id = 9;
+--------+---------+-----------+-------------+----------+-------+--------------------------------------------------------------------------------------------------------------+----------------------+
| bundle | deleted | entity_id | revision_id | langcode | delta | field_summary_value                                                                                          | field_summary_format |
+--------+---------+-----------+-------------+----------+-------+--------------------------------------------------------------------------------------------------------------+----------------------+
| recipe |       0 |         9 |          17 | en       |     0 | A rich and fiery chili sauce. Take care when handling chili peppers. And serve sparingly!                    | basic_html           |
| recipe |       0 |         9 |          18 | en       |     0 | A rich and fiery chili sauce. Take care when handling chili peppers. And serve sparingly!                    | basic_html           |
| recipe |       0 |         9 |          18 | es       |     0 | Una rica y ardiente salsa de chile. Tenga cuidado al manejar chiles. ¡Y servir con moderación!               | basic_html           |
| recipe |       0 |         9 |          39 | en       |     0 | A rich and fiery chili sauce. Take care when handling chili peppers. And serve sparingly!                    | basic_html           |
| recipe |       0 |         9 |          39 | es       |     0 | Una rica y ardiente salsa de chile. Tenga cuidado al manejar chiles. ¡Y servir con moderación!               | basic_html           |
| recipe |       0 |         9 |          40 | en       |     0 | A rich and fiery chili sauce. Take care when handling chili peppers. And serve sparingly!                    | basic_html           |
| recipe |       0 |         9 |          40 | es       |     0 | <p>Una rica y ardiente salsa de chile. Tenga cuidado al manejar chiles. ¡Y servir con moderación! aaaa</p>   | basic_html           |
| recipe |       0 |         9 |          41 | en       |     0 | A rich and fiery chili sauce. Take care when handling chili peppers. And serve sparingly!                    | basic_html           |
+--------+---------+-----------+-------------+----------+-------+--------------------------------------------------------------------------------------------------------------+----------------------+
8 rows in set (0.001 sec)

And node_field_data should look something like this:

MariaDB [db]> SELECT * FROM node_field_data WHERE nid = 9;
+-----+-----+--------+----------+--------+-----+---------------------------+------------+------------+---------+--------+------------------+-------------------------------+----------------------------+------------------------------+
| nid | vid | type   | langcode | status | uid | title                     | created    | changed    | promote | sticky | default_langcode | revision_translation_affected | content_translation_source | content_translation_outdated |
+-----+-----+--------+----------+--------+-----+---------------------------+------------+------------+---------+--------+------------------+-------------------------------+----------------------------+------------------------------+
|   9 |  39 | recipe | en       |      1 |   4 | Fiery chili sauce         | 1686645158 | 1686645158 |       1 |      0 |                1 |                          NULL | und                        |                            0 |
|   9 |  39 | recipe | es       |      1 |   4 | Salsa de chile ardiente 2 | 1686645158 | 1687865049 |       1 |      0 |                0 |                             1 | und                        |                            0 |
+-----+-----+--------+----------+--------+-----+---------------------------+------------+------------+---------+--------+------------------+-------------------------------+----------------------------+------------------------------+

3. now delete the Spanish translation on https://drupal-dev.ddev.site/en/node/9/translations

4. Now query the table again - for me I get the same result for node_revision__field_summary - the Spanish language revision data is still there. Although node_field_data now just has one row for English as expected.

If we don't remove old translation content when we delete translations, how can we prevent people from removing languages until it's removed??

@catch: for me, this sounds for me a different issue

there was mentioned an issue about deleting revisions https://www.drupal.org/project/drupal/issues/2925532

which also has a related issue about Deleting a translation issue
https://www.drupal.org/project/drupal/issues/2815779

is this the issue you're talking about?

thanks

The MR does this:

public static function languageUsedByContent($langcode) {
    $entity_type_manager = \Drupal::entityTypeManager();
    foreach ($entity_type_manager->getDefinitions() as $entity_type_id => $entity_type) {
      if ($entity_type instanceof ContentEntityType && $entity_type->isTranslatable()) {
        $query = $entity_type_manager->getStorage($entity_type_id)->getQuery()
          ->condition($entity_type->getKey('langcode'), $langcode)->accessCheck(FALSE);
        if ($entity_type->isRevisionable()) {
          $query->allRevisions();
        }
        $results = $query->range(0, 1)->execute();

        if ($results) {
          return TRUE;
        }
      }
    }
    return FALSE;

That query checks all revisions. #2815779: Deleting a translation leaves behind orphaned revisions means deleting a translation leaves revisions in the a language - so it will be impossible to get to a point where the language can be removed.

I think that means this is postponed on #2815779: Deleting a translation leaves behind orphaned revisions, or if not, then we should add test coverage showing how the site admin is able to remove the necessary content in order to be able to delete the language.

added https://www.drupal.org/project/drupal/issues/2815779 as related issue