Create a Language field widget and the related formatter

Note that there is no specific concept of field instances for base fields. There are only base fields and they may be altered or defined by-bundle.

Also, that seems to be a pretty massive change on it's own, not sure it's a good idea to make it a part of this conversion?

I'm not sure if you'd call it a dup, but I opened this issue to consider adding at least something to the entity form by default to handle langcode: #2270633: Create a default language element on content entity forms in case they don't provide a selector

It would really be better to have this sorted out before beta.

@plach I started to work on it. any problem?

You are welcome :)

I've created LanguageWidget that extends WidgetBase. But I see the widget shows the the configured languages and two other options:

<select id="edit-field-language-0" name="field_language[0]" class="form-select"><option value="en" selected="selected">English</option><option value="es">Spanish</option><option value="und">- Not specified -</option><option value="zxx">- Not applicable -</option></select>

I have a few question:

1. Before create the widget, the language selector were rendered just configuring the type to be translatable in the content translation overview UI. Now, the user also have to add the widget language to the content type. What it's the expected behavior?

2. I think The problem commented in #9 it's produced by ConfigurableLanguageManager::getLanguages() that returns two unexpected values. I'm reviewing it.

I forgot the set the weight :S

1. +++ b/core/modules/language/src/Plugin/Field/FieldWidget/LanguageWidget.php
   @@ -0,0 +1,44 @@
   + *     "list_string"
   

   For testing, I suppose it was useful to allow to bind this widget to list fields, but in fact it should only be allowed for fields of type language. That is consistent with what we have done for other special-purposes fields and their widgets. It is just a little bit too confusing to for end users to have this available as an option.

2. +++ b/core/modules/language/src/Plugin/Field/FieldWidget/LanguageWidget.php
   @@ -0,0 +1,44 @@
   +    $language_configuration = \Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('node', $items->getEntity()->getType()));
   

   getType() is a node specific method. The generic method that should be called here is bundle().

3. +++ b/core/modules/language/src/Plugin/Field/FieldWidget/LanguageWidget.php
   @@ -0,0 +1,44 @@
   +    $element = array(
   

   So the way that widgets work is that they provide a form element, for each property that the corresponding field item declares in propertyDefinitions(). In our case the field item is \Drupal\Core\Field\Plugin\Field\FieldType\LanguageItem and the corresponding property from LanguageItem::propertyDefinitions() is value. Therefore the form element should be in a 'value' sub-key.

   Simply changing $element = array( to $element['value'] = array( should suffice.

   I expect that to solve a bunch of test failures as well.

4. +++ b/core/modules/node/src/Entity/Node.php
   @@ -349,7 +349,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +        'weight' => -5,
   

   As the title also has a weight of -5, I think we should use a different weight here. I will try this patch out now to see what a good weight would be.

Ahh, I accidentally lost my long comment, so can't be bothered to write that again, so here's the short version:

Re #10:
1. Once we fix the things mentioned above and here, that issue will be resolved
2. That's how it is in 8.0.x as well, no problem there

Additionally:
1. LanguageItem should declare "default_widget" in its annotation
2. Weight should be 0
3. We need to remove the langcode stuff from node_entity_extra_field_info()
4. Not sure, but perhaps the class should be renamed to LanguageSelectWidget instead of just LanguageWidget

Thanks a lot @fran seva for working on this. I think we are really close on this and it already works beautifully.

@tstoeckler thanks for review. I'm working on it right now :)

@tstoeckler Here is the patch.

+++ b/core/modules/language/src/Plugin/Field/FieldWidget/LanguageSelectWidget.php
@@ -0,0 +1,44 @@
+ * Plugin implementation of the 'lancode' widget.

small typo: "lancode"

@stBorchert thanks for review :)

+++ b/core/modules/language/src/Plugin/Field/FieldWidget/LanguageSelectWidget.php
@@ -0,0 +1,44 @@
+    $language_configuration = \Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('node', $items->getEntity()->bundle()));

Module invoke is not necessary here because the widget is provided by the Language module.

+++ b/core/modules/node/src/Entity/Node.php
@@ -347,9 +347,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDisplayOptions('form', array(
+        'type' => 'language_select',
+        'weight' => 0,
+      ))
+      ->setDisplayConfigurable('form', TRUE);
 

We should alter field definitions (for all entity types/bundles we support) from the Language module, otherwise this will blow up when the language module is disabled (because the widget plugin will not be found).

This implements the approach discussed with @yched, @tstockler and Gabor.

tstoeckler directed me to this via #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?.

If the new wording is better, that seems fine to change at least up until we support an upgrade path (possibly afterwards since it is a static config file so can't affect contrib configuration entities). Tagging D8 upgrade path.

I'm still working on this...

@plach I'm working in hook_entity_field_access() implementation for language field. Are you working in the same?

@fran seva:

I was working on test failures: there were quite some things to fix in the content translation handler. I avoided implementing the Field Access API as we don't want to prevent people from saving an entity programmatically, we just want to hide the widget in some particular contexts.

There are still failures, if you wish to keep working on this, please assign it to you :)

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
@@ -29,10 +30,14 @@ class LanguageSelectWidget extends WidgetBase {
+    $entity = $items->getEntity();
+    if ($entity instanceof TranslatableInterface) {
+      $entity = $entity->getUntranslated();
+    }
     $element['value'] = array(
       '#title' => $this->t('Language'),
       '#type' => 'language_select',
-      '#default_value' => $items->getEntity()->language()->getId(),
+      '#default_value' => $entity->language()->getId(),

Now that think about this, it makes no sense: we should just use $items[$delta]->getId()

@plach: yeah, so the topic came up for discussion again last sunday at the post-conf sprint :-) Basically, we (tstoeckler, Gabor, effulgentsia and myself) had the following reasoning:

- This "do not show language selector" option in c_t UI looks like a direct descendant of the D7 contrib feature. Back then there was no REST support or entity validation, so it naturally was formulated in terms of "forms" / "show in forms".

- It really seems that the underlying feature ("allow posting content in different languages") is broader than just forms, and defines a behavior that shoud also be applied to REST submits. If I check that "Do not show" checkbox, it doesn't seem right that some stuff will be forbidden in forms but still be possible with REST ?

So maybe it's still unclear to us what this 'language_show" is really about and what it should do (and in that case I'd suggest making things clear with Gabor on IRC first ?) but:

1) Either it's not just about forms, but more generally about what you can do when submitting entities (form *and* REST), and then it is definitely a question of field access API rather than #access in the widget.

2) Or it really is about just forms, and then just adding an #access flag does not require swapping an alternate LanguageWidget implementation (non-intuitive, and only one module gets a chance to do that) :
- hook_field_widget_form_alter() lets you alter the render array produced by a widget
- for the special case of forcing a field to hidden, this is what hook_entity_form_display_alter() is there for ($display->removeComponent($field_name)
- also: doesn't that setting in c_t UI just duplicate the ability to "hide" fields in Field UI's "form display" pages ?

People in this issue are also likely to be interested in #2350843: LanguageItem field type's value can be any string, should be a valid language code., BTW : if that issue over there gets done, then "a widget for the Language field" would just be the options_select widget ?

Attach rerroll.
[edit]
The test has the same error that the previous patch

@GaborHojtsy @plach this patch is not completed but I need to be sure that what I'm doing is what is expected (I think it is but I want to be sure).

The more important changes in the patch are:

1. class ConfigurableLanguageSelectWidget. has been deleted. As I see, this class just set the correct value to #access and the idea is delegate it to language_entity_fields_access.

2. I have added the hook_entity_fields_access in language.module and for langcode widget check the language_show. This hook needs to return an AccessResult object, in other case will through an error. For that reason, when all widget pass for that hook, when the widget is not langcode I return AccessResult::neutral(); and in the other case, if language_show is true, AccessResult::allowed(); else, AccessResult::forbidden();.

I think the code can be improve using AccessResult::allowedIf or AccessResult::forbiddenIf (I'm on it)

I'm fine with the current approach, thanks :)

@plach thanks for review it :)
I'll continue working on it

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
   @@ -0,0 +1,46 @@
   +    if ($entity instanceof TranslatableInterface) {
   +      $entity = $entity->getUntranslated();
   +    }
   ...
   +      '#default_value' => $entity->language()->getId(),
   

   Interesting.

   Does it make sense to do anything at all if the entity does not implement TranslatableInterface?

2. +++ b/core/modules/aggregator/src/Tests/FeedLanguageTest.php
   @@ -43,6 +43,8 @@ protected function setUp() {
   +    language_save_default_configuration('aggregator_feed', 'aggregator_feed', array('langcode' => 'site_default', 'language_show' => TRUE));
   

   Yay, for consolidation! The fact that this is needed shows that feeds are currently implementing the pattern incorrectly.

3. +++ b/core/modules/language/language.module
   @@ -171,6 +178,26 @@ function language_process_language_select($element) {
   +function language_field_widget_info_alter(array &$info) {
   +  $info['language_select']['class'] = '\Drupal\Core\Field\Plugin\Field\FieldWidget\LanguageSelectWidget';
   +
   +
   +}
   

   This should no longer be necessary.

4. +++ b/core/modules/language/language.module
   @@ -171,6 +178,26 @@ function language_process_language_select($element) {
   +    if ($definition instanceof BaseFieldDefinition && $definition->getType() == 'language' && ($options = $definition->getDisplayOptions('form')) && !empty($options['type'])) {
   

   I think both the instanceof BaseFieldDefinition and the !empty($options['type'] can be dropped.

   1. $fields should always be an array of field definitions

   2. It is valid to leave out 'type' to let the default apply.

5. +++ b/core/modules/language/language.module
   @@ -520,3 +547,15 @@ function language_field_info_alter(&$info) {
   +function language_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
   

   Needs a docblock.

6. +++ b/core/modules/language/language.module
   @@ -520,3 +547,15 @@ function language_field_info_alter(&$info) {
   +    if($language_configuration['language_show']) {
   +      return AccessResult::allowed();
   +    } else {
   +      return AccessResult::forbidden();
   +    }
   +  }
   +  return AccessResult::neutral();
   

   I think we should drop the AccessResult::forbidden(). If you turn off Language module, then the access result will also be AccessResult::neutral() (unless I'm missing something). So it would be weird for that behavior to be different if the module is turned on but the configuration is off.

7. +++ b/core/modules/language/language.module
   @@ -520,3 +547,15 @@ function language_field_info_alter(&$info) {
   \ No newline at end of file
   

   NoooO!!!!!!!!!! ;-)

8. +++ b/core/modules/node/node.module
   @@ -385,26 +385,7 @@ function node_entity_extra_field_info() {
   -        $extra['node'][$bundle->type]['form']['langcode'] = array(
   ...
   -    $extra['node'][$bundle->type]['display']['langcode'] = array(
   

   So awesome that we can drop this

9. +++ b/core/modules/node/node.module
   @@ -385,26 +385,7 @@ function node_entity_extra_field_info() {
   +    ¶
   

   Trailing whitespace.

Needs reroll...

Here is a new patch.

1. @tstoeckler, in #30 @plach comments that we should use $items[$delta]->getId(). I tried but $items[$delta] is a DefaultLanguageItem object and has no getId() method. The method that returns language code is DefaultLanguageItem::getDefaultLangcode($entity). I'm not sure what is better, use $entity->language()->getId() or DefaultLanguageItem::getDefaultLangcode($entity)

2. mmmm yep?, I think I need learn more about what pattern is supposed to be implemented :S

3. This was added to use the ConfigurableLanguageSelect, and I change it in #37 to LanguageSelect, so I think is not needed to make languageSelect be used.

4. I've checked the hook definition and $fields should be an array with field definition. I have dropped. About, point 2 have dropped.

5. Done. Just added implements hook_entity_field_access

6. Finally, I've used AccessResult::forbidenIf();
@tstoeckler what do you think about use forbidenIf()?, basically move the if into the method.

function language_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
  if($field_definition->getName() == 'langcode') {
    $language_configuration = language_get_default_configuration($field_definition->getTargetEntityTypeId(), $items->getEntity()->bundle());
    return AccessResult::forbiddenIf(!$language_configuration['language_show']);
  }
  return AccessResult::neutral();
}

7. newline added :p

8. :)

9. Done

Also, I have made the following manual tests

Previous steps before tests language widgets:

-Add a language (spanish)
- Go to Content translation overview and check
- comment
- content
- custom block
- custom menu link
- shortcut link
- taxonomy link
- user
- click on translatable checkbox for each one.
- Click on save button

Test Comment ==> OK
- Check language widget appears in comment form
- Go to in content translation overview and uncheck the checkbox "Show language selector on create and edit pages" in Comment
section and save the form.
- Reload the comment form and check the language widget is not showed.

Test Content ==> OK
- Check language widget appears in node form
- Go to in content translation overview and uncheck the checkbox "Show language selector on create and edit pages" in Content
section and save the form.
- Reload the node add/edit form and check the language widget is not showed.

Test Custom block ==> OK
- Check language widget appears in custom block creation form
- Go to in content translation overview and uncheck the checkbox "Show language selector on create and edit pages" in Custom block
section and save the form.
- Reload the Custom block form and check the language widget is not showed.

Test Custom menu link ==> NOT OK
- Check language widget appears in Add menu form
- Go to in content translation overview and uncheck the checkbox "Show language selector on create and edit pages" in Custom menu
link section and save the form.
- Reload the Add menu form and check the language widget is not showed.

Test Shorcut link ==> OK
- Check language widget appears in Add link form
- Go to in content translation overview and uncheck the checkbox "Show language selector on create and edit pages" in Shorcut link
section and save the form.
- Reload the Add link form and check the language widget is not showed.

Test Taxonomy link ==> I'm not sure if this is what is should be tested but in all cases the language selector is not hidden.
+Vocabulary form
- Check language widget appears in Add vocabulary form
- uncheck in content translation overview the checkbox "Show language selector on create and edit pages" in Taxonomy link section
and save the form.
- Reload the Add vocabulary form and check the language widget is not showed.
+ Term form
- Check language widget appears in Add term form
- uncheck in content translation overview the checkbox "Show language selector on create and edit pages" in Taxonomy link section
and save the form.
- Reload the Add term form and check the language widget is not showed.

Test User ==> I'm not sure if this is what is should be tested but the language selector is not hidden.
- Check language widget appears in User edit profile
- uncheck in content translation overview the checkbox "Show language selector on create and edit pages" in User section
and save the form.
- Reload the User profile edit form and check the language widget is not showed.

+++ b/core/modules/language/language.module
@@ -548,14 +539,14 @@ function language_field_info_alter(&$info) {
+    return AccessResult::forbiddenIf(!$language_configuration['language_show']);

I would suggest to use allowIf() with the reverse condition instead.

Again, I am thinking of two situations which should behave identically IMO:
1. Language module is non installed. In this case language_entity_field_access() is obviously not called. No result is semantically equivalent to AccessResult::neutral().

2. Language module is installed but the language_show configuration is disabled for a specific bundle. In this case we are returning AccessResult::forbidden() (due to the usage of forbiddenId()).

I think these two situations should be have similar, therefore I think we should return AccessResult::neutral() if language_show is FALSE. That can be achieved by using allowedIf().

Will maybe look into the test fails later today.

Awesome work doing all those manual tests!!!!

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -147,9 +147,13 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +    $langcode_key = $this->entityType->getKey('langcode') ?: 'langcode';
   +    $language_widget = NULL;
   +    if (isset($form[$langcode_key]['widget'][0]['value']['#type'])) {
   +      $language_widget = &$form[$langcode_key]['widget'][0]['value'];
   +    }
   +    if (!$is_translation && $language_widget && $language_widget['#type'] == 'language_select' && $has_translations) {
   

   Looks like the logic here could be streammlined a bit.
   Since $language_widget is necessrily NULL unless the 1st "if" is TRUE, second "if" could be nested within the first one.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -147,9 +147,13 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   -    $language_widget = isset($form['langcode']) && $form['langcode']['#type'] == 'language_select';
   -    if ($language_widget && $has_translations) {
   -      $form['langcode']['#options'] = array();
   +    $langcode_key = $this->entityType->getKey('langcode') ?: 'langcode';
   +    $language_widget = NULL;
   +    if (isset($form[$langcode_key]['widget'][0]['value']['#type'])) {
   +      $language_widget = &$form[$langcode_key]['widget'][0]['value'];
   +    }
   +    if (!$is_translation && $language_widget && $language_widget['#type'] == 'language_select' && $has_translations) {
   +      $language_widget['#options'] = array();
          foreach (language_list(LanguageInterface::STATE_CONFIGURABLE) as $language) {
            if (empty($translations[$language->getId()]) || $language->getId() == $entity_langcode) {
              $form['langcode']['#options'][$language->getId()] = $language->getName();
   

   More generally, this piece of code looks a bit weird - this empties the #options from $form[$langcode_key]['widget'][0]['value'], and adds #options to $form['langcode'] ?

   - I understand that $form[$langcode_key]['widget'][0]['value'] is the new "language widget", but what is $form['langcode'] now ?
   - The "old" code only manipulated $form['langcode']['#options'] (it emptied it, and then added new options), which kind of hints that the new code should be entirely about $form[$langcode_key]['widget'][0]['value'], and that $form['langcode'] is just a remnant that shouldn't be there anymore ?
   - If the above is true, then directly manipulating $form[$langcode_key]['widget'][0]['value'] like is being done here is a bit shaky. hook_field_widget_form_alter() exists for these kind of things.

1. +++ b/core/modules/language/language.module
   @@ -171,6 +178,17 @@ function language_process_language_select($element) {
   +    if ($definition->getType() == 'language' && ($options = $definition->getDisplayOptions('form'))) {
   

   nitpick : the $options var is not needed (and a comment describing what is being done could be nice ;-)

2. +++ b/core/modules/language/language.module
   @@ -520,3 +538,15 @@ function language_field_info_alter(&$info) {
   +  if($field_definition->getName() == 'langcode') {
   

   missing space after "if"

3. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -139,7 +139,7 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
      protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
        // Only users with the administer nodes permission can edit administrative
        // fields.
   -    $administrative_fields = array('uid', 'status', 'created', 'promote', 'sticky');
   +    $administrative_fields = array('uid', 'status', 'created', 'promote', 'sticky', 'langcode');
   

   why is that change needed ?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
   @@ -0,0 +1,46 @@
   +    $entity = $items->getEntity();
   +    if ($entity instanceof TranslatableInterface) {
   +      $entity = $entity->getUntranslated();
   +    }
   +    $element['value'] = array(
   +      '#title' => $this->t('Language'),
   +      '#type' => 'language_select',
   +      '#default_value' => $entity->language()->getId(),
   +      '#languages' => LanguageInterface::STATE_ALL,
   +    );
   

   If $entity is not a TranslatableInterface, then you cannot call its ->language() method ?

   More generally, a "language" field being present on a non-translatable entity type feels like a mistake. We can as well return nothing in that case ?

   public function formElement(...) {
     $entity = $items->getEntity();
     if ($entity instanceof TranslatableInterface) {
       ...
       $element['value'] = array(...);
       return $element;
     }
   }
   

   ?

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
@@ -0,0 +1,46 @@
+    $entity = $items->getEntity();
+    if ($entity instanceof TranslatableInterface) {
+      $entity = $entity->getUntranslated();
+    }
+    $element['value'] = array(
+      '#title' => $this->t('Language'),
+      '#type' => 'language_select',
+      '#default_value' => $entity->language()->getId(),
+      '#languages' => LanguageInterface::STATE_ALL,
+    );
+    return $element;

This really cannot work: this is a generic widget so we should not be making assumptions on the field it refers to. For instance we might have a node describing a stele and a configurable language field storing the language in which the stele is written. This node might be translated in many languages, so the entity language would have nothing to do with the stele language. In this case the current language widget would break badly. Sorry if my previous suggestion was wrong, I think $items[$delta]->language->getId() or $items[$delta]->value should work.

By the way, this makes me think that it would be nice to have a widget setting allowing to hide the special languages and leaving only actual languages in place.

By the way, this makes me think that it would be nice to have a widget setting allowing to hide the special languages and leaving only actual languages in place.

@plach: your idea sounds very close to #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc), I would suggest not pursuing that here.

Ok, definitely not required here :)

Hi!!
a bit late but It's being a crazy week :S

@tstoeckler I see your point of view and I totally agree :) but I tried to use allowif and if the language_show flag is false return AccessResult::neutral() and as the widget has access, the widget will never be hide. For that reason I think we have to return a AccessResult::fobiden() to hide the widget. In other hand, I'm going to try to set the access to false but not in the widget ('#access' => FALSE) because that way avoid the hook and will never be show. I'm on it :)

@yched thanks for review. I have made the following changes (see attached)

46.1 Sure. I didn't see it. I have change the if, and now is nested.

46.2 I'm on it, but I need more time :p

47.1 Done. The var is not being use.

47.2 Done.

47.3 I think the initial idea was check perms in checkFieldAccess and just let users with admin perms to edit the node get the widget.
But now I think has no sense because we are using hook_entity_field_access and language_show flag to show/hide it. @tstoeckler?

47.4 in that point, after read the @plach comment (#48) I see we don't need the $entity to get the langcode value, so I have removed the if($entity instanceof TranslatableInterface) and use $item[$delta] to get the langcode.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
   @@ -0,0 +1,43 @@
   +    $element['value'] = array(
   +      '#title' => $this->t('Language'),
   

   This doesn't look right, the #title (and other important properties like #required) come in pre-populated in the $element passed as a parameter.

   Typically widgets do something like :

   $element += array(
     '#type' => ...
     '#default_value' => ... $items[$delta] ...
     ...
   );
   return array('value' => $element);
   

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -147,19 +147,23 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +    if (isset($form[$langcode_key]['widget'][0]['value']['#type'])) {
   +      $language_widget = &$form[$langcode_key]['widget'][0]['value'];
   +      if (!$is_translation && $language_widget && $language_widget['#type'] == 'language_select' && $has_translations) {
   

   No need to test for $langcode_widget in the second if :-)

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -147,19 +147,23 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +        $language_widget['#options'] = array();
   +        foreach (language_list(LanguageInterface::STATE_CONFIGURABLE) as $language) {
   +          if (empty($translations[$language->getId()]) || $language->getId() == $entity_langcode) {
   +            $form['langcode']['#options'][$language->getId()] = $language->getName();
   +          }
            }
   

   Looks like this code just cannot work as long as there are still some stuff about $form['langcode']. What you want is $language_widget['#options'].

   AAMOF, this code would be a bit clearer if it did:

   $options = array();
   foreach (...) {
     if (...) {
       $options[$language_id] = $language_name;
     }
   }
   $language_widget['#options'] = $options.
   

   Makes it clear that this piece of code is about building the list of #options for the widget.

A bit late again :S

@yched thanks for your review. The point 3 has been great. I didn't check this case in my manual tests.
I have apply all the changes but I still have to work on #46.2

Thanks for keeping the ball rolling!

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -147,19 +147,25 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +        foreach (language_list(LanguageInterface::STATE_CONFIGURABLE) as $language) {
   

   Let's retrieve the language list from the language manager and remove the procedural wrapper while we are at this :)

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -147,19 +147,25 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +//            $form['langcode']['#options'][$language->getId()] = $language->getName();
   

   We can remove this, I guess (sorry for the mess :))

3. +++ b/core/modules/language/language.module
   @@ -13,6 +15,11 @@
   +
   

   Surplus blank line.

4. +++ b/core/modules/language/language.module
   @@ -171,6 +178,17 @@ function language_process_language_select($element) {
   +  foreach ($fields as $definition) {
   +    if ($definition->getType() == 'language' && ($options = $definition->getDisplayOptions('form'))) {
   +      $definition->setDisplayConfigurable('form', TRUE);
   +    }
   +  }
   +}
   

   Can we add a comment explaining what's happening here? It took me a moment, even if I knew it ;)
   Btw, $options is not used.

5. +++ b/core/modules/language/language.module
   @@ -520,3 +538,15 @@ function language_field_info_alter(&$info) {
   +  if ($field_definition->getName() == 'langcode') {
   

   We should use the 'langcode' entity key here, the field name might not be 'langcode' (see the content translation handler hunk)

Although this is still NW, let's see what the bot thinks about it...

Gave ContentTranslationHandler::entityFormAlter() a closer look, and rearranged things a bit.
(this took care of @plach's #55 .2, but I didn't adress the other remarks)

With that in place, I take back what I said earlier about moving this to hook_field_widget_form_alter(), this logic is better fitted where it currently is.

Added a note that this only works if the widget used structured the same way as LanguageSelectWidget, though.

Also, removed a stale "use" statement in LanguageSelectWidget.

Will still need work for fails, though.

Also, not striclty tied to this issue here, but opened #2364863: Perform all c_t $form alterations in CTH::entityFormAlter()

Attached patch...pending:

1. Use getKey() to get the language field name.
Using getKey('langcode') I don't get the name. Checking all the keys with getKeys() I see that langcode is not a key.
$items->getEntity()->getEntityType()->getKey('langcode') --> return false

2. Use hook_field_widget_form_alter.

3. check #46.2

4. check test error and fix it.

@yched:

With that in place, I take back what I said earlier about moving this to hook_field_widget_form_alter(), this logic is better fitted where it currently is.
Added a note that this only works if the widget used structured the same way as LanguageSelectWidget, though.

I'm honestly not sure about that: hook_field_widget_form_alter() looked like the right place to alter the widget as there we can be sure of the array structure. Maybe we could do that and add some code in the validation handler to check that the selected language code is valid? That way if the widget has been changed and thus is not altered, we can still prevent wrong language codes from being picked up...

I'm working in test errors.

@plach I've been working in tests and I haven't finished. I have posted a patch because I need feedback (I'm not sure about some behaviors supposed in tests).

Current status:

*Fixed tests: (green, almos in my local)

- Drupal\node\Tests\NodeFieldMultilingualTest
- Drupal\taxonomy\Tests\TermLanguageTest
- Drupal\system\Tests\Entity\EntityTranslationFormTest

*Doubts:

-Drupal\node\Tests\NodeTypeInitalLanguageTest :
Fails in display tests because the new language widget does not allow display configuration,
so it will never be show in the content as well will not be configurable. If this is true (I think it is) we should remove all assertions related with language widget display, isn't it?

The following tests have a common error, and it's related to retranslate flag and "outdated" status:
- Drupal\block_content\Tests\BlockContentTranslationUITest
- Drupal\comment\Tests\CommentTranslationUITest
- Drupal\content_translation\Tests\ContentTestTranslationUITest
- Drupal\menu_link_content\Tests\MenuLinkContentUITest
- Drupal\shortcut\Tests\ShortcutTranslationUITest
- Drupal\taxonomy\Tests\TermTranslationUITest

I have made changes in the test implementing an approach for the behavior I think have drupal currently (I have read the code where the flag and status are calculated). Basically, what I have seen in the tests is that always we are checking the retranslate flag and as I have seen, this just should be done when the entity for that language has no source language and its status is outdate. In other hand, I have seen that the test supposed by default as checked the checkbox for outdate status, but that is only true when the added_language is not equal to original language. This can be seen in [1]

[1] ContentTranslationUITest.php::doTestOutdatedStatus()

The patch is not completed but I post it just to get feedback

re @plach #63 / hook_field_widget_form_alter() :

Yeah, what I meant is that I'm not married to the idea of hook_field_widget_form_alter() anymore. Having all the logic in one place (ContentTranslationHandler::entityFormAlter()) also makes sense IMO, since:
- moving that to a separate place will need to duplicate some of the logic (the $is_translation and $has_translations variables)
- the best we can do is code stuff for a specific widget structure ($widget[0]['value']['#options']) anyway...

Remove patch #65 and post a patch which should fix the following tests:

• Drupal\locale\Tests\ LocaleContentTest
• Drupal\menu_ui\Tests\MenuLanguageTest
• Drupal\node\Tests\NodeFieldMultilingualTest
• Drupal\taxonomy\Tests\TermLanguageTest
• Drupal\system\Tests\Entity\EntityTranslationFormTest

I'm still working in the other tests and I think the problem is in the language widget added to:

• Block: Error in test Drupal\block_content\Tests\BlockContentTranslationUITest
• Comment: Error in test Drupal\comment\Tests\CommentTranslationUITest
• Content Translation: Error in test Drupal\content_translation\Tests\ContentTestTranslationUITest
• Menu Link: Error in test Drupal\menu_link_content\Tests\MenuLinkContentUITest
• ShortCut: Error in test Drupal\shortcut\Tests\ShortcutTranslationUITest
• Term: Error in test Drupal\taxonomy\Tests\TermTranslationUITest

All of them have the same error in ContentTranslationUITest, but I don't see what I'm missing. @plach, @yched in #43 I made a manual test and review of language widget and I think the widget is not added in all forms, could someone confirm where should be added?

Sorry for being vacant, I will have a look to this tomorrow.

This should fix the translation UI tests.

Ok, this should be better, although it's a bit dirty. We should probably obtain the langcode value from the widget, if available.

I should know I should stop coding after 3AM :)

I created #2375845: Ensure entity language field name can be defined by the entity provider, which is related. It would be good to have that one fixed before this, so we can clean-up some code, but not strictly required.

@fran seva:

Are you still working on this? Do you still need feedback on anything?

@plach yes I'm still working on it. I think I need feedback from the manual test i did in #43. I saw langcode selectors that were not widget. Should all of them be language widget?

I did a quick manual test:

• Menu link seems to work correctly now.
• Taxonomy terms too.
• Taxonomy vocabulary is a config entity so it's out of luck. We should try to provide a generic language configuration widget in the Language module for any bundle config entity, but that's out of scope here.
• Users are not meant to have an entity language selector, so the configuration widget in the Language settings page should not be displayed IMO. @Gabor what do you think?

Actually the langcode entity key issue was already existing.

@plach: I always thought the user language selector configurability works with the preferred language field for users as well (the only one displayed and assumed to be equal to all three language preferences on users by default). Practically I would assume if I don't have language selection enabled on users, they could not set a preferred language either and vice versa.

After review the test errors I see that there is a problem with language selector when we render the creation/edit form. The problem is tha the widget is not being rendered due to the default configuration for bundle aggregator_feed has by default language_show = false and when hook_entity_field_access is called we check AccessResult::forbiddenIf(!$language_configuration['language_show']), so if language_show if false we will return static::forbidden(), so the widget will not be showed.

At this point a return to #45 where @tstoeckler recommend use AccessResult::allowedIf that will return static::neutral() if language_show is false, and the widget will be showed. That's true, but if we have set language_show = false, don't we should return AccessResult::forbiden()?

In other words, I think the code should be similar to:

if ($field_definition->getName() == 'langcode') {
    $language_configuration = language_get_default_configuration($field_definition->getTargetEntityTypeId(), $items->getEntity()->bundle());

    if(!$language_configuration['language_show']) {
     return AccessResult::forbidden();
    }else {
     return AccessResult::allowed();
    }
  }
  // if we check one field distinct to langcode, we delegate in other hook_entity_field_access implementations
  return AccessResult::neutral();

But this required one more thing to allow agrregator_feed bundle show the language widget, and it's allow to set to true language_show (I've been looking for it but I don't see it, so I supposGae it's not possible set it to true)

@plach What do you think?

@plach I'm thinking that by default bundle aggregator_feed could set language_show to true, and if language module is activated will return AccessResult::allowed() and if not, will return AccessResult::neutral().

@plach Attach a patch that fix Drupal\comment\Tests\CommentLanguageTest and remove code from test core/modules/node/src/Tests/NodeTypeInitialLanguageTest.php because AFAIK language widget will not have display options so the tests to check it will not be needed, Is that correct?

@franSeva:

I had a look to the latest patch: actually I'd prefer not to drop the ability of displaying the Language field value, so this adds a Language formatter and restores the previous test coverage. Interdiff is from #77.

About feed language, the attached patch includes a quick fix, since your proposal did not make the widget come back :) Ultimately I think we should allow non translatable entity types having language support to be configured in the content language settings, but that's a separate task. Here you can find some experimental code (plus screenshot) I tried tonight: https://gist.github.com/plach79/c1425c28d671ec828a6c.

The screenshot:

This should fix one test failure, no idea of what's wrong with the last one...

Aside from that, we should be mostly ready, I think.

I tried the attached patch @alexpott provided me to figure out what's going on. Attached you can find three examples of the differences that are making the test fail. It seems to me for some reason additions performed by the Language module are not properly uninstalled. I am wondering whether they should be part of the third party settings instead...

Only checked quickly, but my guess is that on uninstall, onDependencyRemoval() in EntityDisplayBase isn't able to remove the langcode key from the display, simply because it's not really a dependency I guess.

One option is to cleanup entitydisplays when language module is uninstalled maybe in hook_modules_preuninstall() ?
modules/field/src/Entity/FieldConfig.php contains code to remove fields from entity displays as well when a field is removed.

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -156,9 +156,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDisplayOptions('view', array(
+        'type' => 'hidden',
+        'weight' => 2,
+      ))
+      ->setDisplayOptions('form', array(

Nitpick - weight is ignored for hidden fields (happens in a couple of places)

@plach : can you provide some context on what the diffs showed in #93 represent exactly ? That's a bit obscure :-)

@yched If I'm reading the test right (ConfigImportAllTest) - it's a the point where modules are being uninstalled. The entity displays should be cleaned up and the langcode key information should be gone, but it's not currently. Most likeley because there's no mechanism todo this removal operation. language_entity_base_field_info_alter() makes the language types configurable and setDisplayOptions() is called in various entity types now.

I'll try a quick fix locally, see if I can come up with something.

Hmkay - what is "left" and what is "right" in #93 ?

See https://www.drupal.org/files/issues/test.patch.txt

Still not too explicit :-)

What I get is :

- "left" is the current live config, i.e after all modules were unsinstalled,
- "right" is the config we're trying to import back, i.e the config before the modules were uninstalled
[edit: wrong, corrected below]

- "right" is the config that was saved when all modules were still installed
- "left" is the config after all modules were manually installed and the saved above config was re-imported

Is that correct ?

This looks related to the fact that when a Display is saved to config, EntityDisplayBase::toArray() removes the info about fields whose display is not configurable (entries about fields with non-configurable display are only added to the Display at runtime, and not actually saved in config - to ensure that "not configurable" means "don't store or load anything from config about it").

So the presence or absence of entries for 'langocde' in the config that gets saved depends on whether language.module is enabled when the save occurs, because language_entity_base_field_info_alter() changes "is the display of 'langcode' field configurable or not".

What I think happens is something like:
- The files on the right (state of the config with all modules enabled) contain no entry for 'langcode', because the Displays were created before language was enabled, and not re-saved after that.
- Then we uninstall all modules. This triggers a series of EDB::onDependencyRemoval() / EDB::save(). If the last one happens when language.module is not uninstalled yet, then the config records for the Displays contain entries for 'langcode'. But I don't think this causes the issue here, because those records are overwritten during the config re-import that comes next.
- Then we re-import the previous config. This starts by re-enabling modules, including language.module. Then the config entries for the displays are imported, this resaves the Displays, thus including entries for 'langcode' since language.module is enabled.
- Thus the files on the left (active config after we have done all the above) contain entries for 'langcode'.

No clear vision of a fix for now, though :-/

From my #99

What I get is :
- "left" is the current live config, i.e after all modules were unsinstalled,
- "right" is the config we're trying to import back, i.e the config before the modules were uninstalled
Is that correct ?

No, that was not correct :-)
Adjusted it, and adjusted the reasoning in #100 accordingly

In short :
- The files on the right represent the result of "create the displays, then enable language.module"
- But by nature, config sync discards the original sequence of events, and treats module enabling first, and then saves the config entities.
As we knew for long, this can mean tricky race conditions in some edge cases - this is one of them :-)

So I think that the issue here is rather that the initial config state (language.module enabled, but Displays created before that) is kind of inconsistent (Display records don't contain the entries for 'langcode' they would get if they were re-saved at this point), and thus cannot be re-obtained after subsequent config manipulations.

language_entity_base_field_info_alter() changing the ->setDisplayConfigurable() means that existing Displays should be re-adjusted somehow.

Generally speaking, since the content of an Entity*Display config record currently depends on FieldDefinitions (EntityDisplay::toArray() adjusts the content that gets saved depending on $field_definition->isDisplayConfigurable()), then it should be re-evaluated and re-saved when the FieldDefinitions change. I don't see how we could detect that, though.

So maybe for now, it should be language_install() / language_uninstall() that re-saves all existing Entity*Displays, given what it does in language_entity_base_field_info_alter() ?

So maybe for now, it should be language_install() / language_uninstall() that re-saves all existing Entity*Displays, given what it does in language_entity_base_field_info_alter() ?

Yeah I was thinking the same after I checked again what was going on. Fun racing condtions :)

Anyway, this patch is green.

(note, I have a test issue at #2384189: Test issue for 2230637

Nice; We should probably add comments and crosslink language_install() & language_entity_base_field_info_alter() ?

I think so - needs work also for nitpicks in #94 - have to run now though.

Also, we should do the same in language_uninstall()

Also, we should do the same in language_uninstall()

The code is now in language_modules_installed() which is also called for language_modules_uninstall(), so should be fine.

Thanks a lot @swentel and @yched for the very helpful insights and code :)

This should address #94 and #105 and thus should be ready to go IMO. I'd RTBC it but I worked on it way too much to be allowed to.

Qualified one @todo with the related issue.

+++ b/core/modules/language/language.module
@@ -428,6 +430,10 @@ function language_modules_installed($modules) {
+    // system, we to clean up the related values manually.

Nitpick: either remove 'to' or add 'have'

Other than that, this is ready I think.

Fixed, thanks!

RTBC anyone? ;)

Thanks for the review :)

A few replies:

2: That's a 'form' component, the others are 'view' components.
3: That's a temporary workaround, we should probably provide a more complete fix elsewhere, see #88.

Just a reroll for now

This one should be better

Sorry @Wim, fran is going to post a new patch addressing your review ASAP :)

@Wim @plach I'm working on it. I have 8/10 completed.

@wim @plach I'm having problems in my environment meanwhile I fix it I post what I got.
1. pending
2. done
3. done
4. done
5. done
6. done
7. EntityViewDisplay::loadMultiple() --> I'm on it.
8. Done
9. nothing to do
10. done

How does that todo address the #access thing?

That allows to remove it because at that point we can configure the value that determines the #access key value from the UI. We need to create the issue and qualify the @todo with the related URL though (also missing trailing point).

@Wim here is the patch with all changes.
About 1. just say that the widget will always post an array so I remove the if.

@Wim it seems that $langcode not always is an array. Tests fail in CollapsedDrupal\user\Tests\UserTranslationUITest.
After revert the change in ContentEntityForm::updateFormLangcode the test should pass.

Sorry, the last change is not correct: we need to fix tests or forms instead. We must deal with a predictable data structure.

Probably it was me introducing that change, but since only user translation tests are failing it would be worth to fix them. I can do it myself if you prefer.

Also:

+++ b/core/modules/language/language.module
@@ -376,7 +376,7 @@ function language_modules_installed($modules) {
       /** @var $display EntityDisplayInterface */

The type hint below is still not correct: we need the full namespace in it.

@plach I could work on it in a few hours :)

@plach that's what I'm going to do:

1. remove is_array from ContentEntityForm::updateFormLangcode
2. change user edit form to send the language as an array and not as a string
3. Fix the type hint

is ok?

Nevermind, I spoke with @yched about this, I have a better solution than what I coded earlier. I will post it later.

This exploits entity builders to clean-up the generic handling of form language and user language synchronization.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -51,6 +51,7 @@ public static function create(ContainerInterface $container) {
   +    $form['#entity_builders']['update_form_langcode'] = [$this, 'updateFormLangcode'];
   

   Is the recommended practice to use $form['#entity_builder'], or to call updateFormLangcode() from within buildEntity() ?

   Might be worthing checking with @fago / @Berdir, but I would have thought that, since you're writing code in the EntityForm class itself, you might as well do the latter.

   Adding stuff in $form['#entity_builder'] is more for 3rd party code (contrib modules...) that are not the "owner" of the EntityForm class ?

2. [edit : that's exactly @Wim's point 1. in the previous comment :-)]

   +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -76,6 +76,8 @@ protected function init(FormStateInterface $form_state) {
   +    // Initialize default form langcode status.
   +    $this->isDefaultFormLangcode($form_state);
   

   It's weird that the method is named like a simple "getter for a boolean property" (which is how it was used so far), but is now in fact used for an internal side effect it happens to have (initialize something in $form_state) rather than for the value it returns ?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -100,7 +102,11 @@ public function getFormLangcode(FormStateInterface $form_state) {
   +      $value = $this->getFormLangcode($form_state) == $this->entity->getUntranslated()->language()->getId();
   +      $form_state->set('default_form_langcode', $value);
   

   Nitpick : $value is a bit generic for a boolean, $is_default_form_langcode (matching the method name) would make it clearer what this "value" is.

Thanks, this should address #137 and #138.

Yay, #139 is way cleaner IMO - thanks !

All pending concerns have been addressed, this looks ready to me :-)

Thanks a lot @plach & @fran seva, awesome patch !

General error: 2006 MySQL server has gone away

Added beta-phase evaluation.

Draft CR at https://www.drupal.org/node/2393007

Just a question after reading the patch in #2227381: Apply formatters and widgets to User base fields 'name' and 'email' : this patch here doesn't use the widget for the language fields in User ?
Do we need a followup for that ?

Nope, the plan is not having the language widget exposed to users by default. We are syncing the entity language with the preferred language, which has its own custom widget.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -138,13 +164,20 @@ public function setFormDisplay(EntityFormDisplayInterface $form_display, FormSta
      /**
       * Updates the form language to reflect any change to the entity language.
       *
   +   * @param string $entity_type_id
   +   *   The entity type identifier.
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity updated with the submitted values.
   +   * @param array $form
   +   *   The complete form array.
       * @param \Drupal\Core\Form\FormStateInterface $form_state
       *   The current state of the form.
       */
   -  protected function updateFormLangcode(FormStateInterface $form_state) {
   +  public function updateFormLangcode($entity_type_id, EntityInterface $entity, array $form, FormStateInterface $form_state) {
   

   I think the documentation should reflect that fact that this is a #entity_builder callback. Otherwise people will wonder why it is not on the interface - like I did :)

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -12,6 +12,7 @@
   +use Drupal\Core\Language\LanguageManager;
   

   Not used.

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -138,19 +138,19 @@ protected function doTestOutdatedStatus() {
        $default_langcode = $this->langcodes[0];
   

   The changes here remove all the usages - unnecessary code.

4. +++ b/core/modules/language/language.module
   @@ -5,12 +5,16 @@
   +use Drupal\Core\Entity\Display\EntityDisplayInterface;
   

   Not used.

5. +++ b/core/modules/shortcut/src/ShortcutForm.php
   @@ -69,13 +69,6 @@ public function form(array $form, FormStateInterface $form_state) {
   -      '#languages' => LanguageInterface::STATE_ALL,
   

   No longer need to use LanguageInterface in ShortcutForm.php

6. +++ b/core/modules/user/src/AccountForm.php
   @@ -10,6 +10,7 @@
   +use Drupal\Core\Entity\EntityTypeInterface;
   

   Unused.

7. This change looks extremely sensible - much easier to make custom entities have to do less for multilingual-ness.

This should address #152.

Changes are fairly minor, back to RTBC.

Committed 8ae055f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

Yay, thanks!

Just published the CR.

Amazing, thanks!

Yay, congrats @plach & @fran seva !

@fran seva++

Created a follow-up for #88: #2397729: Content language settings cannot be configured for non-translatable types on the one page configuration form.

Yay!!! I learned a lot with that issue :)

Any ideas why language field formatter is completely ignored? #2637808: LanguageFormatter is not used for nodes because it's still hard coded in NodeViewBuilder