In #2076445-69: Make sure language codes for original field values always match entity language regardless of field translatability @yched said:

I'm not sure I see the value in keeping the internal runtime storage keyed by LANGCODE_DEFAULT (in $this->translations, $this->fields[$field_name], $this->values[$field_name]), rather than the actual langcode. It complicates the code and logic in a lot of places in ContentEntityBase ?

To which @fago replied:

I think we should start arranging data according to our translation model, i.e. entity - language - fields. I agree the keying by default language stuff seems deprecated since the object now deals with all translations, instead we could start doing shortcuts as $this->current_fields =& $this->fields[$this->activeLangcode];. But that all does not have to happen in this issue as it's internal restructuring, so let's do as it's easier?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

We also discussed putting hardcoded logic in FieldItemList::getLangcode(), removing the need for setLangcode().

I'd favor doing that in this patch too (easier to review "final, simplified code" than an intermediary step containing remnants of complex logic that we know is useless anyway; + at this point in time, yay on less followups mentally track)
But that probably depends on the actual code impact.

yched’s picture

Status: Postponed » Active

#2076445: Make sure language codes for original field values always match entity language regardless of field translatability is RTBC - maybe we can unpostpone this ?
(patches would need to include that other patch until it gets committed, but at least there's a solid ground)

yched’s picture

FWIW, I still think we should do this. The internal code supporting ContentEntity translation is more complex / confusing that it needs to be - and we don't really need more complexity here :-)

Berdir’s picture

I don't disagree :)

The entity cache makes this less of a problem, but it would still improve the performance of loading entities from the storage after cache clears/updates/inserts.

chx’s picture

As berdir asked me to comment: I am neutral. Just let me know what changes and I will adapt.

Berdir’s picture

Status: Active » Needs review
FileSize
13.17 KB

Ok, let's try this. Entity tests are passing.

yched’s picture

Oh sweet, thanks a ton for kicking this @Berdir.

Internally keying by language first definitely makes more sense in D8 :-)
Any chance we can get rid of the weirdness of "the internal key for the default langcode is LANGCODE_DEFAULT ('x-default') rather than the actual langcode" ? IIRC that would further simplify ContentEntityBase by removing needless massaging and special casing.

Status: Needs review » Needs work

The last submitted patch, 7: refactor-internal-storage-2142885-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

I'm not sure, one problem is that the default langcode will also be keyed by that very langcode :) That's kind of recursive ;)

It would essentially mean that we trade the $translations argument, against a $default_langcode argument to make that possible?

yched’s picture

@Berdir : you mean the boolean property pointed by the $this->defaultLangcodeKey key ? In fact, I don't clearly see where it's actually used, and why exactly it would be an issue that we don't have direct access to it - but looking back in the code trying to figure it out brings lots of places where removing that LANGCODE_DEFAULT special casing would simplify things :-)

yched’s picture

Also, detail :

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -162,17 +162,19 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
+    $translations = array_keys($values);

We could simply inline this as foreach (array_keys($values) as $langcode) { lower in the __construct() ?

Berdir’s picture

Assigned: Unassigned » plach

In fact, I don't clearly see where it's actually used

That's kind of my point :)

We don't have to use it, because we know that the default language is keyed by LANGCODE_DEFAULT.

If you get fields with 'en', 'fr' and 'de' in there, to find out which one is the default language, you have to loop over the values of that field until you find the one that says 1.

And for new entities, where do you put the values? You might not even have a language code until the langcode field is initialized, so you'd need a temporary value, or hardcode the default value implementation of the language field (we might actually have that somewhere already, not sure).

But maybe it's easier than I think, let's see what @plach thinks.

yched’s picture

If you get fields with 'en', 'fr' and 'de' in there, to find out which one is the default language, you have to loop over the values of that field until you find the one that says 1.

Sure, but :
- it doesn't seem like having to loop on $values until you find the one where $values[$some_langcode]['default_langcode'] == TRUE would be a real perf hit ?
- I'm not even sure which part of the code actually needs that info anyway ?

As you point, I guess new entities being created, and the fact that their langcode is, well, part of the values keyed by langcode, might be a bit more problematic, true :-/. It would be a bit sad that this has to spill over the code and runtime structures in ContentEntityBase, cause the impact on code complexity is real...

plach’s picture

Assigned: plach » Unassigned

In fact, I don't clearly see where it's actually used

It's used in the ::onChange() method to prevent the related field value from being changed, since it could be initialized via field setters and not via constructor values. The isset() check is used to detect this scenario.

If you get fields with 'en', 'fr' and 'de' in there, to find out which one is the default language, you have to loop over the values of that field until you find the one that says 1.

I don't think this is a big deal unless you have 50 translations, however...

It would be a bit sad that this has to spill over the code and runtime structures in ContentEntityBase, cause the impact on code complexity is real...

... honestly I don't see how using an actual language would make things simpler. You'd basically trade LanguageInterface::LANGCODE_DEFAULT with $this->defaultLangcode more or less as-is. And additionally you'd have to detect which is the default language by looping on $values and rekey everything when the default language changes. Also, this would break when constructing the entity with empty values and initializing field values via setters, as we might not have the default_langcode flag available to determine the default language. I suspect trying to implement this approach would make things more complex.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -162,17 +162,19 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
+        if (is_array($this->values[LanguageInterface::LANGCODE_DEFAULT][$field_name]) && isset($this->values[LanguageInterface::LANGCODE_DEFAULT][$field_name])) {

The second test looks redundant. Also we could skip one nesting level by and-ing the first one and the parent one.

plach’s picture

Instead, @fago's suggestion about using internal references (see the OP) could be leveraged to point to $this->values[LanguageInterface::LANGCODE_DEFAULT] and avoid referring to LanguageInterface::LANGCODE_DEFAULT in most places.

yched’s picture

OK, too bad about LANGCODE_DEFAULT.

Having $this->current_fields as a ref to $this->fields[$this->activeLangcode] sounds handy, but then wouldn't we want the same for $this->current_values / $this->fields[$this->activeLangcode] ?

Status: Needs review » Needs work

The last submitted patch, 7: refactor-internal-storage-2142885-1.patch, failed testing.

Berdir’s picture

Issue tags: +Novice, +Needs reroll

Tagging for reroll.

siva_epari’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
13.33 KB

Patch rerolled.

Status: Needs review » Needs work

The last submitted patch, 21: refactor-2142885-21.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

geek-merlin’s picture

Title: Refactor ContentEntityBase internal field storage » Simplify ContentEntityBase internal field storage by removing special treatment for LANGCODE_DEFAULT

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should 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.

Version: 9.5.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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.