Problem/Motivation

Follow-up from #3610122: Field data for multiple cardinality fields are not populated on entity load

Quoting @godotislate from that issue:

When language is uninstalled, the en language config entity is deleted, which causes NodeStorage::clearRevisionsLanguage() to change the langcode to und for all entries in the node_revision table that match the deleted langcode (en). From there, after loading the entity again, the field data for the multiple cardinality nodes are not populated.

[...]

So, one thing I think we can do is prevent NodeStorage::clearRevisionsLanguage() from setting the langcode to und in node_revision for all rows that are in the default langcode. The best place for this logic is probably in NodeEntityHooks::configurableLanguageDelete(), so that it looks like this:

   #[Hook('configurable_language_delete')]
   public function configurableLanguageDelete(ConfigurableLanguageInterface $language): void {
-    // On nodes with this language, unset the language.
-    \Drupal::entityTypeManager()->getStorage('node')->clearRevisionsLanguage($language);
+    if ($language->getId() !== \Drupal::languageManager()->getDefaultLanguage()->getId()) {
+      // On nodes with this language, unset the language.
+      \Drupal::entityTypeManager()->getStorage('node')->clearRevisionsLanguage($language);
+    }
   }

This won't change anything though for deletions of other configured languages, nor will it address anything for existing node_revision rows that were already changed to und.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3620626

Command icon 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:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Title: Data integrity issues with deleting languages as node revisions » Data integrity issues with deleting languages and node revisions

godotislate made their first commit to this issue’s fork.

godotislate opened merge request !16972

godotislate’s picture

godotislate
he/him
commented
Status: Active » Needs review

MR is ready: https://git.drupalcode.org/project/drupal/-/merge_requests/16972
Test only failure: https://git.drupalcode.org/project/drupal/-/jobs/11960729

oily’s picture

oily commented

I have read through the code in MR!16972. Made one comment. EntityManager and LanguageManager are readonly in other hook classes, but not always.

oily’s picture

oily commented

I was wondering why there were a number of test assertions in the test but only 1 is failing in test-only. But I see that the other assertions cover a gap in existing tests and differentiate between nodes in default language of 'en' and those that are not. Not sure if a manual test and screenshots is useful..

godotislate’s picture

godotislate
he/him
commented

I don't see any comments on the MR, but as for readonly, I've been more reluctant to add it or ask for it to be added, because sometimes it turns out later the class needs to be serialized, and DependencySerializationTrait can not repopulate readonly properties in __wakeup(), at least in <= PHP 8.4 (kinda unclear about PHP 8.5). The fix for that is to use __unserialize() (and __serialize() instead of __sleep()), but that's pending review in #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize().

Granted, it's generally unusual for services to be serialized, but I thought there was a recent example of a Hook class that was, because it had a form ajax callback or something. I don't see it in the codebase, though, so probably mis-remembering.

Anyway, tl;dr, added readonly.

differentiate between nodes in default language of 'en' and those that are not.

Yes, because we are changing how nodes in the default language are handled here, while leaving it the same for those that aren't. The default language can only be deleted when the language module is being uninstalled, so this issue really only covers a very specific case.

oily’s picture

oily commented

Re: #8

I don't see any comments on the MR

I needed to click the 'review' button. Just did that and resolved the thread after checking the new pipeline and the 2 additions of 'read-only' to the code. The 2 warnings in the pipeline are showing in other issues so unrelated i think. I don't see any case for manual testing of the fix. I dont see anything more to be done on this.

Will try RTBTC.

oily’s picture

oily commented
Status: Needs review » Reviewed & tested by the community