Problem/Motivation

I've noticed that ContentEntityBase::getTranslatedField is called recursively for non translatable fields and also in __sleep the field values of non translatable fields will be stored twice and the function getValue will be called for all the translations for which a field has been initialized. Also if you iterate over $values you will get unneeded iterations over the same values. In the case where the an referenced entity of a non translatable field will be returned by FieldItemList::getValue this would cause $values to contain the referenced entity not only once but one time for the default langcode and additionally so many times as the number of entity translations, which in case of serializing the parent would cause a bigger serialized object.

Proposed resolution

Remove the recursive call of ContentEntityBase::getTranslatedField and store directly non translatable fields only under the default langcode by keeping track which field are translatable and which not.

Remaining tasks

Reroll
Tests (maybe)
Review.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#26 2828133-26.patch6.91 KBAbhisheksingh27
#5 interdiff-4-5.txt984 byteshchonov
#5 2828133-5.patch4.7 KBhchonov
#4 interdiff-2-4.txt773 byteshchonov
#4 2828133-4.patch4.59 KBhchonov
#2 2828133-2.patch4.02 KBhchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
FileSize
2828133-2.patch4.02 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2828133-2.patch, failed testing.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Needs review
FileSize
2828133-4.patch4.59 KB
interdiff-2-4.txt773 bytes

So the problem that has been hidden until now is that if we have already initialized a field but afterwards have removed an entity translation and we try to get the field through ContentEntityBase::get we will get the field but if we try getting the field through ContentEntityBase::getTranslatedField there will be an exception for accessing a field of a removed translation ... This is what happens now when deleting an entity translation for the user entity where the name field is not translatable and not any more accessible with the changes in ContentEntityBase::getTranslatedField from ContentEntityBase::get by the following code :

  public function get($field_name) {
    if (!isset($this->fields[$field_name][$this->activeLangcode])) {
      return $this->getTranslatedField($field_name, $this->activeLangcode);
    }
    return $this->fields[$field_name][$this->activeLangcode];
  }

because non translatable fields will not be kept anymore under the langcode for which they have been requested.

But this however deserves its own issue and for now I am only making changes in ContentEntityBase::get to replicate the current behavior.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
FileSize
2828133-5.patch4.7 KB
interdiff-4-5.txt984 bytes

A little bit faster check for non translatable field in ContentEntityBase::get is also possible.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Issue summary: View changes

The last submitted patch, 4: 2828133-4.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer commented

Thanks for working on this @hchonov. I found it it's quite complex fix. It would be nice to have a test to cover this change (i.e. just test to fail and with this patch to pass). Also if we are looking at perf angle, would be nice to have xprofile.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -457,6 +464,9 @@ public function hasField($field_name) {
+      if (isset($this->fields[$field_name][LanguageInterface::LANGCODE_DEFAULT]) && ((isset($this->fieldsTranslatability[$field_name]) && !$this->fieldsTranslatability[$field_name]) || (($definition = $this->getFieldDefinition($field_name)) && !$definition->isTranslatable()))) {

Looks like a complicated check. can we split this as separate helper? also a comment to say what are we checking here.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

Hmm this is just code refactoring and there is no test that will fail. The code as it is now is correct but it is using unnecessary recursion which means one more function call put on the call stack and avoiding it just simply implies a better code flow. I do not think that we need any profiling.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Issue tags: +DevDaysSeville
maacl’s picture

maacl CreditAttribution: maacl commented

@hchonov is there an issue for the case you described in #4? I'm having exactly this, I cannot delete a translation of a node because of the exception beeing thrown. ("The entity object refers to a removed translation (en) and cannot be manipulated")

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

@maacl , I am not aware of such an issue. I think the problem here was caused by some of my changes in the patch.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Updated the IS remaining tasks field
Reading the comments, specifically #4, this seems like it could be a bug and that we will need a test case showing the issue.

Abhisheksingh27’s picture

Abhisheksingh27 CreditAttribution: Abhisheksingh27 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
FileSize
2828133-26.patch6.91 KB

Adding reroll for 10.1.x

Status: Needs review » Needs work

The last submitted patch, 26: 2828133-26.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.