Problem/Motivation
When removing a language, some cleanup happens in Drupal but a lot of things are left behind. Nodes are "cleaned up" in a way that leaves node data in an unrecoverable half-updated state, revisions get updated but the base node data table or field tables do not get updated, so most node data still carries the language.
The easiest to reproduce WSOD fatal error from that is after removing a language that has at least one content translation created, any subsequent attempt to load /admin/content results in the following error:
Warning: Undefined array key "[the langcode]" in Drupal\Core\Entity\ContentEntityBase->language() (line 767 of /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityBase.php)
followed by:
Error: Call to a member function getId() on null in Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple() (line 272 of /var/www/html/web/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php).
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
Why is this happening and other related issues
When a language is removed, Drupal only removes/updates a few very specific things. See comment #92 for details:
- The one language config entity (
language.entity.[langcode]) gets removed - URL prefix and domain entry for this langcode (
language.negotiationconfig) gets removed - Interface translations (DB:
locales_*tables) get removed - Translation
.pofiles get removed - Config translation overrides (e.g.
language/fr/) get removed - Block language visibility conditions get removed
- Node revision metadata langcode ONLY (
node_revision.langcodereset tound)
But a lot of stuff stays around on the site. Even with just core:
- Config in the removed language (not as overrides)
- Node data table rows (original language and translations)
- Node field data and revision translations (
node_field_data,node_field_revision) - Taxonomy terms (original language and translations) and their fields
- Users (original language and translations) and their fields
- Comments (original language and translations) and their fields
- Block content (original language and translations) and their fields
- Media (original language and translations) and their fields
- Menu link content (original language and translations)
- Path aliases
- Files
- Shortcuts
- Content moderation states
- Search index entries
So there is many chances that something else will also go wrong once the language is removed not just the content listing throwing a fatal error.
Proposed resolution
Ideally there would be a comprehensive data cleanup routine that cleans up everything related to the language. After all removing this language will cause problems later when trying to deal with users in that language (eg. sending emails to them in a nonexistent language). But the state the node data is left in is neither cleaned up nor recoverable as a small part of the node data is overwritten while most of the data is left as-is.
So the proposed resolution for core is to let modules react to the intent of removing a language and respond with data on why the user should not do it. Compile all of this into a big list and present it to the user. Unfortunately some cases would require user input to resolve such as if the language is used as the original language of a content entity if we were to update it to unknown language, then it could not have translations anymore in Drupal's data model, so all the translations also become invalid.
It is not clear if a comprehensive cleanup feature is even possible to build in Drupal core, at least it would be quite a heavy feature. However if we are to allow removing languages with numerous data stored related to them, we need to not leave the site in a half-broken state.
Remaining tasks
1. Build a hook that lets modules react to the intent of removing language, so we can gather data on what each of them use it for. This is similar to what we do when we warn the user at module removal.
2. Decide if we want to allow the user to remove the language when the list still contains items.
User interface changes
TBD.
API changes
TBD.
Data model changes
TBD.
Release notes snippet
TBD.
| Comment | File | Size | Author |
|---|---|---|---|
| #80 | Screenshot 2023-06-15 at 1.46.58 PM.png | 67.56 KB | smustgrave |
| #79 | drupal.ddev_.site_8037_index.php_route=_sql&db=db&table=node_field_data&pos=0.png | 833.68 KB | vasike |
| #79 | drupal.ddev_.site_fr_admin_content (1).png | 288.71 KB | vasike |
| #79 | drupal.ddev_.site_fr_admin_content.png | 281.59 KB | vasike |
| #71 | 2851029-nr-bot.txt | 144 bytes | needs-review-queue-bot |
Issue fork drupal-2851029
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
marassa commentedComment #3
cilefen commentedComment #4
saurabh.tripathi.cs commentedComment #5
saurabh.tripathi.cs commentedHI,
Switched on 8.3.x and checked the file : /core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
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
Comment #6
saurabh.tripathi.cs commentedI have added an extra condition for default translation check in the patch.
Comment #7
saurabh.tripathi.cs commentedComment #8
pixeliteI'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:
Comment #9
pixeliteComment #10
marassa commentedWell, I described what happened to my development site - after that I just restored from the backup and moved on. It could be that some combination of factors was at play (not sure how I could possibly investigate that now) or probably it was "accidentally" fixed somewhere between 8.2.6 and 8.4.x-dev. In any case, if you are not able to reproduce it with current version in a clean environment then it's probably a non-issue - sorry for wasting your time. I guess I should have tried to reproduce it back then on a clean install - will do that next time.
Comment #11
berdirNot 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.
Comment #12
marassa commentedI agree. Imagine you have translations of the same entity into two languages and then remove both languages. Will you wind up with two 'und' translations of the same entity? Not good.
Comment #13
xjmThanks @pixelite for helping triage this issue. Adding issue credit for the triage.
Comment #14
xjmLet'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.)
Comment #15
roderik(Just triaging...)
Comment from Gábor Hojtsy in duplicate #2540876-16: WSOD with listing content after removing language:
Comment #18
awm commentedI 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.
Comment #19
awm commentedComment #20
sonvir249 commentedHi,
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.
Comment #22
awm commentedat 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.
Comment #23
hchonovThe 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:
Comment #24
hchonovComment #25
hchonovThis 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.
Comment #26
awm commented@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 ?
Comment #27
andypostComment #28
shane birley commentedI 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...
This isn't complete. What should be added?
Comment #29
sealionking commentedthe same problem after deleted languages
after adding the deleted languages back, the content admin view came back.
Comment #30
hchonovI've tried creating an entity with a non-existent language and then retrieve that language after entity save.
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:
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.Comment #31
maximpodorov commentedThe patch is based on the code from comment #23.
Comment #32
hchonovI've thought of one more use case - we might have Entity Reference fields
This should become a count query by simply adding "->count()". We would also need a test :).
Comment #33
maximpodorov commentedWhat about Entity Reference fields? Why do you think they add more necessary checks?
What is more efficient, count() or range(0, 1)?
Comment #36
hchonovComment #37
ndobromirov commentedThis 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...
Comment #38
plach+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 (seeConfigImportValidateEventSubscriberBase), 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.
Comment #39
maximpodorov commentedAs suggested, range(0, 1) and accessCheck(FALSE) have been added. Existing tests will fail without adapting to the form changes.
Comment #40
andypost@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
It should use `#type => list' inspead of plain string
Comment #43
sam-elayyoub commentedHere's what I use
Comment #44
sam-elayyoub commentedComment #45
sam-elayyoub commentedComment #46
hchonov@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.
Comment #48
sathish.redcrackle commentedI have modified the failed tests by deleting the languages through backend and attached the patch.
Comment #49
swatichouhan012 commented@sathish.redcrackle Please change status to Needs Review after patch apply.
Comment #50
swatichouhan012 commentedComment #51
astoker88 commentedJust ran into this issue myself, a simple cron run fixed the issue for anyone else this may help.
Comment #53
pasan.gamage commented@astoker88 Running cron didn't work for me.
Comment #54
johanvdr commentedPatches 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.
Comment #55
joseph.olstadComment #56
shaalThere'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.
Comment #57
quietone commentedUpdated IS
Comment #58
quietone commentedThis is blocking #1870454: [PP-1] The "delete" operation for the added languages is scary/confusing.
Comment #59
ankithashettyRerolled patch in #48, thanks!
Comment #60
karishmaamin commentedFixed custom code failure at #59. Please review
Comment #61
joachim commentedLooks good, just a few nitpicks:
I would use getGroup() == 'content' instead of checking instanceof.
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.
Nitpick: line break after the [ and each translation variable on its own line.
Comment #62
marcvangendJust 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
instanceofinstantly 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, soif ($entity_type instanceof ContentEntityTypeInterface ...)Comment #63
joachim commentedgetGroup() 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.
Comment #64
marcvangendOK, 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. (MaybegetGroup() == '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.
Comment #65
knvc commentedThe 8.9 version patch doesn't seem to fix the issue on Drupal 8.9.14.
Comment #70
asirjacques commentedHello,
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.
Comment #71
needs-review-queue-bot commentedThe 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.
Comment #75
vasikenew MR available - continuing the patches efforts
- Create helper method
languageUsedByContent($langcode)forConfigurableLanguageentity classUsed 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
LanguageDeleteFormif 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.phpTo tests what happens if the content still has language in use.
Questions:
Are there other tests needed?
Is "change record" required for this?
Comment #76
smustgrave commentedChange record I think so.
Question bunch of tests had to be updated with
Is that because it broke? Could this cause a regression for existing sites if so.
Comment #77
vasikeThis 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
LanguageConfigurationTesthttps://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
Comment #78
smustgrave commentedThink it could be as simple as just mentioning that you can't delete a language now. Maybe a screenshot of the error
Comment #79
vasikeUnfortunately 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
Comment #80
smustgrave commentedFollowing 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.
Comment #81
vasikeupdated the summary ... i hope we're close ...
Comment #82
smustgrave commentedLets get this in committers eyes
Good job!
Comment #83
catchI 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?
Comment #84
smustgrave commented@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.
Comment #85
vasike@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
Comment #86
catch@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:
And node_field_data should look something like this:
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??
Comment #87
vasike@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
Comment #88
catchThe MR does this:
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.
Comment #89
vasikeadded https://www.drupal.org/project/drupal/issues/2815779 as related issue
Comment #90
redzeufWe could also consider to give the option to the webmaster to delete all the entity translations for this language.
Here is a module who delete translation for entities https://www.drupal.org/project/delete_entity_translations
Comment #92
gábor hojtsyI arrived here via configuration management today. One can very easily remove a language that has a bunch of active configuration using it. (langcode of the config is the language). This is nowhere checked or prevented or reconciled. Configuration in unknown language is also quite bad :D
That said I went to check what happens when a language gets removed because it gives a clue what would need to be checked before we allow to remove it.
1. Block visibility settings get removed:
2. Locale deletes interface translation strings, .po files and config translation OVERRIDES. But leaves config in the deleted language alone.
3. Nodes remove revision language whatever that means:
This clearRevisionsLanguage() AFAIS does what @catch was arguing is missing, which is updating ALL the past revisions of nodes in this language to not specified:
So there are a LOT of problems with this. First there are numerous other content entities in core with language. User entities, media, block content, taxonomy terms, etc. None of that is updated or checked! Second, only the base revision table is updated. Not any of the field tables or data tables, so you have a partial data loss for nodes while invalid data for all the other content entities. Then there is the problem that if you had nodes originally in certain languages that had translations, now you have nodes that don't have language and yet still have translations which is an invalid situation in Drupal.
Then there is that config overrides are removed but base config in the language is not even considered.
So yeah it looks like removing a language would leave your site in a very bad state and you can't just add the language back, the damage has already been done. Your node revision language information is already gone, all of your config translations have all been gone. The only clean way back is restoring a backup.
So I think we should prevent this from happening. I don't know if it is possible at all to provide a clean way to remove all the language stuff, after all you can have language dependent things at so many places. Maybe we need to link to a contributed project that helps dive deep into that. I think it would be odd to have that module in core, but it sounds like a pretty complicated tool to build for sure.
But I think the lack of that tool at our disposal should not prevent us from preventing the user from breaking their site. At minimum we need an extended confirmation screen where we tell them they have 20 users in this language and 256 configuration items and 6753 content translations. Are they sure they want to loose some of that stuff and break some of them? :)
Raising to critical as this feature seems to leave the site in an unusable state with content entity data partially updated and broken.
Comment #93
gábor hojtsyShould update issue summary base don current findings. :) Will look at that tomorrow.
Comment #94
gábor hojtsyThe WSOD can still be reproduced with Umami as explained in the issue summary (I just did), so that does not need updating but there are more problems that need to be added. Updated the issue summary.