Make (content) translation language available as a field definition

Make the langcode field definition translatable and add a default_langcode boolean definition allowing to determine the default language.

Could we have a default_langcode field which just stores the langcode instead? Or would that make things more difficult?

$entity->default_langcode->value == 'en' would be nice to have imo - also it's a good replacement for the current behaviour of $entity->langcode->value then.

@fago: We need the default flag in the data table, so that we can query on only rows which are the default language. Storing the default language doesn't give you that directly, you'd have to compare that with the actual language.

It would also be a storage change, this is AFAIK not.

Moreover I'd avoid having two language codes on the entity object, it might make things more confusing than they need to be.

I see the reasoning, ok to me.

This way the only way to get the default language is via the API ($entity->getUntranslated()->language()).

So, should be the boolean be then "translated" yes|no?

/**
* Returns the translatable object referring to the original language.
*
* @return $this
* The translation object referring to the original language.
*/
public function getUntranslated();

Introducing another terminology with "default langcode" here seems unfortunate, but keeps 1:1 with the storage. Unfortunate, but probably the best do now. Maybe we can clarify the relationhip to the "original default language" here.

Huh what?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
+  public function isDefaultTranslation() {
+    return $this->activeLangcode == Language::LANGCODE_DEFAULT;
+  }

Um. I am really confused here. When you first create an entity, assuming you have the i18n modules all turned on and configured, you can set its language to anything, and that becomes the original/base/default language for that entity. It need not match the site's default language. So I don't think this logic is correct at all.

But maybe I'm misunderstanding what this method is about. Can we change the terminology here? There are so many things called "default language". I suggest adopting this terminology, to avoid confusion:

- Default site language - a property of the site (the setting for which language is the site default) This is set in locale.module at admin/config/regional/language

- Default language for an entity - a property of the entity bundle (the setting for the default language of this entity/bundle when you create new content). This is set in content_translation.module at admin/config/regional/content-language, and need not match the site default language.

- Original language (or base language?) - a property of an entity (the original language this particular entity was written in). This is set on the node/add form when you first create an entity, and need not match the site defeault or entity default language. Note: When you go to the node/N/trnanslations page, this language is already shown on the page there as "Original language".

- Translation language - a property of an entity translation, the language you translated it into. Cannot match the original language of the entity; may be equal either to the default site or default entity language, or not.

- Original? - Boolean property of an entity translation: TRUE if this is the original translation of the entity (the translation in the original language). I think there are some queries that need this.

So I just did a test (without any patches). My default languages are all set to English. I created an Article and set its language to Spanish (so the original language of this node is Spanish). In the database, node_field_data.default_langcode is 1, and when I translate this node into English, the value on that translation is 0.

On another node I created in English and translated into Spanish, node_field_data.default_langcode is 1 for the English translation and 0 for the Spanish one.

So that database field already has the meaning of "original?" not "Matches the default site language" or "matches the default entity language for this bundle".

I like default_translation - how big of a change would that be exactly ?

Also, re: "Should we hard-code the default_langcode field definition (and the related entity key) in the Entity Manager"

I think freeing developers from having to define fields that are required to implement the entity system business logic makes some sense too.

That seems to makes sense to me ? If the entity system relies on the presence of a default_langcode field, and on the actual content of its definition (i.e a specific entity type has no use case for defining it a bit differently, and can only break stuff by doing so), then it feels right to avoid entity type authors the burden of copy/pasting it over and over again ?

Just a review from someone without a lot of knowledge about the entity system.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -144,6 +151,7 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   +    $this->defaultLangcodeKey = $this->getEntityType()->getKey('default_langcode');
   

   Its odd that this has to be configurable, but well, i guess this is how we do it everywhere.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -245,6 +253,13 @@ public function isDefaultRevision($new_value = NULL) {
   +    return $this->activeLangcode == Language::LANGCODE_DEFAULT;
   

   Nitpick: Afaik we use more === these days.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -245,6 +253,13 @@ public function isDefaultRevision($new_value = NULL) {
   @@ -543,14 +558,38 @@ public function onChange($name) {
   
   @@ -543,14 +558,38 @@ public function onChange($name) {
          }
   

   Should we update the documentation of FieldableEntityInterface::onChange to reflect the thrown exceptions? Not sure about our way of doing here.

4. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -450,15 +462,21 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +    // Make sure translatable entity types define the "langcode" field properly.
   +    if ($entity_type->isTranslatable() && (!isset($keys['langcode']) || !isset($base_field_definitions[$keys['langcode']]) || !$base_field_definitions[$keys['langcode']]->isTranslatable())) {
   +      throw new \LogicException(String::format('The @entity_type entity type cannot be translatable as it does not define a proper "langcode" field.', array('@entity_type' => $entity_type->getLabel())));
   +    }
   
   +++ b/core/modules/node/src/Entity/Node.php
   @@ -353,6 +353,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        $fields['langcode'] = BaseFieldDefinition::create('language')
          ->setLabel(t('Language'))
          ->setDescription(t('The node language code.'))
   +      ->setTranslatable(TRUE)
          ->setRevisionable(TRUE)
          ->setDisplayOptions('view', array(
            'type' => 'hidden',
   

   ... can't we just split up the exception to throw an exception for the last case which tells better what is going on?

5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -673,12 +665,14 @@ protected function attachPropertyData(array &$entities) {
   +        // Find revisioned fields that are not entity keys. Exclude the langcode
   +        // key as the base table holds only the default language.
   +        $base_fields = array_diff($table_mapping->getFieldNames($this->baseTable), array($this->langcodeKey));
   

   Just curious, should that kind of logic on the longrun be moved to the table mapping?

6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -163,7 +164,6 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
          $storage_definition->getSchema() != $original->getSchema() ||
   ...
   -      $storage_definition->isTranslatable() != $original->isTranslatable() ||
   

   Just curious, why can we stop checking isTranslatable()?

7. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -146,17 +153,48 @@ protected function setUp() {
   +      ->will($this->returnValueMap($map));
   ...
   +      ->will($this->returnValue(array()));
   ...
   +      ->will($this->returnValue(array()));
   ...
   +      ->will($this->returnValueMap($map));
   

   Small tip in case you want to write less in the future. You can use ->willReturnMap($map) / ->willReturnValue()

I took a careful, long look at this patch, with plach in IRC helping me understand what is all doing. I have a few questions/comments -- but other than the questions below, I think the patch is nearly ready and the new tests look adequate to test the new functionality.

a) As one point, even after the previous conversation about the 'default_langcode' field, I *still* got confused about it being Boolean while reviewing. ;) But I understand that changing it now would be disruptive, so will not complain again. :) [This is not something to fix/answer, just a comment]

b) I love all the - lines in the patch without corresponding + lines -- all those if/else things for handling the langcode field that are now GONE. Super! The one exception being in dawehner's review in #20, item 5. I'm curious about that too? And there's another new one in content_translation.admin.inc but I think that cannot be avoided. [not something to fix/answer]

c) Regarding dawehner's review in #20, item 3 - yes we should add @throws. Probably on the interface, but if it can't be for some reason, then instead of using @inheritdoc you should copy the interface docs over to the class method and add @throws to the end.

d) I'll leave plach to answer the other points in dawehner's review.

e) Nitpick, in SqlContentEntityStorageSchema::onFieldStorageDefinitionDelete()

+        throw new FieldStorageDefinitionUpdateForbiddenException('Unable to delete a field with data that can\'t be purged.');

use "" instead of '' quotes to avoid \' here

f) I especially like that you tested the "use your own key" functionality for the default_langcode entity key. But I saw one possible hole in the testing: You removed a bunch of lines in SqlContentEntityStorageTest that verified that default_langcode was part of the "extra fields", and it shouldn't be any more, so that is good. But I didn't see that default_langcode was added to the test anywhere to verify it was part of the base schema? Maybe that doesn't need to be done, not sure?

g) We'll need a change record.

I think there's enough in my review and #20 to mark this needs work now?

4: I don't get what you are asking, can you please rephrase?

Well, the exception message is this:

he @entity_type entity type cannot be translatable as it does not define a proper "langcode" field.

but it could just say that the langcode field should always be marked as translatable.

CR looks good -- I made a couple of small edits for better readability (I hope). And I added a section telling developers that they need to make their langcode fields translatable.

Can you verify it is still accurate? https://www.drupal.org/node/2446315

The rest of the changes look good to me. I think @dawehner's idea in #24 is good for the exception message, but other than that I think it's ready to go.

The patch looks good to me as well, just found one tiny thing:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -245,6 +253,13 @@ public function isDefaultRevision($new_value = NULL) {
+    return $this->activeLangcode === Language::LANGCODE_DEFAULT;

$activeLangcode is declared with a default value of LanguageInterface::LANGCODE_DEFAULT, is there any reason we're not using the interface here?

Thank you

+1

So.. I stared at this for at least a half hour and can't quite parse this. The issue summary looks accurate, but assumes a lot more understanding of D8MI than I possess and this time zone is not friendly to Europeans. :) (I did manage to find Berdir, but he also needed to take a closer look at this before he could really walk through it.)

Something that that would help from a reviewer POV is providing a series of steps one can take in the UI to reproduce whatever the situation is we're trying to fix with this patch. It's not clear from the summary. It's also not clear from the summary the big benefits we get from making the langcode field translatable, because at a glance it sounds like a "BOOOOOM brains all over the floor" kind of change. ;)

Also, regarding the change record:

     $fields['langcode'] = BaseFieldDefinition::create('language')
       ->setLabel(t('Language'))
       ->setDescription(t('The comment language code.'))
// The line below is newly required:
       ->setTranslatable(TRUE)
       ->setDisplayOptions('view', array(
         'type' => 'hidden',
       ))

This seems like a very easy thing for developers to forget to do. Is there a chance we could default translatable to TRUE for langcode fields somehow? That would also make this change less disruptive, and also reduce fragility in contrib.

(In about 16 hours, I will be traveling for basically 2 days, so please don't hold committing this up on me.)

One of the main reasons we need this is the (critical) issue that is postponed on it:
#2384863: Translation language base field handler should use views field handler, provide unified options
Basically, we want everything on an Entity, when used in Views, to be using Field UI to format it, or at least it absolutely needs to be using an entity-aware formatter. This was not possible to do with the Language views-field, because it is not currently a real entity-field. I mean, it's terrible: when you load a particular translation of an entity, there is apparently no way in code to get the translation's language off the object, so unlike all the other information on the entity, you can't easily write an entity-aware formatter for it.

I'll add this to the issue summary... I don't know of other ways this affects the UI or even if there are any. I think it's mostly a developer-oriented change. It makes a LOT of the entity code a LOT cleaner, and cleans out a LOT of special cases for "When someone is trying to get language information out of the danged entity, special case it because we can't read it directly", by simply saying "language is a translatable field".

We do really need to do this...

Updated the issue summary and tried to make the code snippet more clear, used an == instead of != condition, because == is what you'd expect.

Also can't see any mention of @chx/MongoDB here. This will affect him too, we should probably get an OK from him. I *think* it will make his life easier as well but it would be good to have a confirmation.

@webchick: There is nothing broken right now when looking at the UI, because we have no code that would incorrectly rely on wrong assumptions. But the referenced views issue is trying to do exactly that and failed to do so.

Didn't manage to read the patch yet, but the proposed changes look great to me.

Not a full review, just noticed this:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -386,8 +386,17 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
+    catch (DatabaseExceptionWrapper $e) {

See #2443663: PostgreSQL: Fix system\Tests\Entity\EntityDefinitionUpdateTest, I think you should check for the DatabaseException interface instead, as other backends might return slightly different exceptions.

There are two sides of a storage engine, write and read. Read is being tested at #2436209: Test ContentEntityBase constructor called with multilanguage values . If that issue needs to change, that's fine, but that test is almost a 100% copy of what MongoDB could/should do (currently it's mimicking core, less optimal) so as long as it exists and passes I know what to do. As for storage, this is what I do:

    $languages = $entity->getTranslationLanguages();
    foreach ($languages as $langcode => $language) {
      $translation = $entity->getTranslation($langcode);
      foreach ($translation as $field_name => $items) {
        if (!$items->isEmpty()) {
          foreach ($items as $delta => $item) {
            $translated_values[$field_name][$delta] = $item->toArray();

I think that doesn't change, does it?

The changes look good to address all concerns raised.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -450,15 +471,21 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
        foreach ($keys as $key => $field_name) {
   -      if (isset($base_field_definitions[$field_name]) && in_array($key, array('id', 'revision', 'uuid', 'bundle')) && $base_field_definitions[$field_name]->isRevisionable()) {
   +      if (isset($base_field_definitions[$field_name]) && in_array($key, $fixed_keys) && $base_field_definitions[$field_name]->isRevisionable()) {
            throw new \LogicException(String::format('The @field field cannot be revisionable as it is used as @key entity key.', array('@field' => $base_field_definitions[$field_name]->getLabel(), '@key' => $key)));
          }
   -      if (isset($base_field_definitions[$field_name]) && in_array($key, array('id', 'revision', 'uuid', 'bundle', 'langcode')) && $base_field_definitions[$field_name]->isTranslatable()) {
   +      if (isset($base_field_definitions[$field_name]) && in_array($key, $fixed_keys) && $base_field_definitions[$field_name]->isTranslatable()) {
            throw new \LogicException(String::format('The @field field cannot be translatable as it is used as @key entity key.', array('@field' => $base_field_definitions[$field_name]->getLabel(), '@key' => $key)));
          }
        }
   

   This would be easier to read like so - also if the base field definition does not exist for an entity key - is that not a problem?

       // Ensure defined entity keys are there and have proper revisionable and
       // translatable values.
       foreach (array_intersect_key($keys, array_flip(['id', 'revision', 'uuid', 'bundle'])) as $key => $field_name) {
         if (!isset($base_field_definitions[$field_name])) {
           throw new \LogicException(String::format('The @field field definition does not exist and it is used as @key entity key.', array(
             '@field' => $base_field_definitions[$field_name]->getLabel(),
             '@key' => $key
           )));
         }
         if ($base_field_definitions[$field_name]->isRevisionable()) {
           throw new \LogicException(String::format('The @field field cannot be revisionable as it is used as @key entity key.', array(
             '@field' => $base_field_definitions[$field_name]->getLabel(),
             '@key' => $key
           )));
         }
         if ($base_field_definitions[$field_name]->isTranslatable()) {
           throw new \LogicException(String::format('The @field field cannot be translatable as it is used as @key entity key.', array(
             '@field' => $base_field_definitions[$field_name]->getLabel(),
             '@key' => $key
           )));
         }
       }
   

2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -192,6 +192,8 @@ public function getFields($include_computed = TRUE);
   +   *
   +   * @throws \LogicException
   

   Missing the \InvalidArgument. Also we should be detailing the circumstances in which these exceptions are thrown.

Probably you shouldn't RTBC your own patch. ;) Two comments/questions:

a) Nitpick on the @throws docs in FieldableEntityInterface::onChange():

    * @throws \LogicException
+   *   When trying to change:
+   *   - the language of a translation;
+   *   - the value of the flag identifying the default translation object.

We normally want each list item to start with a capital letter and end in .

b) Nitpick in the new EntityManager logic sections:

+          '@key' => $key
+        )));

Last array element should end in , if you use vertical spacing. This occurs three times in the patch.
[EDIT: removed stray text after this]

Looks good. Thanks for the additional review @jhodgdon.

Problem is plach, you took Alex's code as written. ;)

Committed a21abc7 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to issue summary.