Entity builder ContentEntityForm::updateFormLangcode changes the form language code during ajax calls

Can we also assert that when save the node its language is updated?

yes, but IMHO is also good and useful to check that the node language has changed when saving, just to be sure.

Reviewing. Interesting because the code says it allows modules to act before and after the form language is updated. But the updateFormLangcode is the one updating it itself. So I guess the intention was to override in extensions this method to do things before/after the default behavior. Did you do any archeology to see why was this introduced in the first place? Is the form langcode not supposed to reflect what language are we editing of the entity? What should happen on preview for example (if you are not going into Ajax, change the language and then preview?).

Concrete patch review:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -52,8 +52,6 @@ public function form(array $form, FormStateInterface $form_state) {
   -    // Allow modules to act before and after form language is updated.
   -    $form['#entity_builders']['update_form_langcode'] = [$this, 'updateFormLangcode'];
   
   @@ -248,32 +246,6 @@ public function setFormDisplay(EntityFormDisplayInterface $form_display, FormSta
   -   */
   -  public function updateFormLangcode($entity_type_id, EntityInterface $entity, array $form, FormStateInterface $form_state) {
   

   I don't think its possible to remove a public method and claim we have backwards compatility :)

   I also think it should be left called from the form entity builder (even if empty in the base implementation) as this is a base class that others expect the same behavior (if they override the updateFormLanguage for example).

2. +++ b/core/modules/system/src/Tests/Form/FormLanguageTest.php
   @@ -0,0 +1,101 @@
   +    ¶
   +    // Check that the form language did not change after the ajax call.
   +    $form_langcode = \Drupal::state()->get('entity_test.form_langcode');
   +    $this->assertEqual($form_langcode, 'en', 'Form language did not change after an ajax callback.');
   +    ¶
   

   Minor: spacing issues on empty lines.

I don't think you addressed the preview question. What happens there? Also for the ajax interaction, I think people change the langcode on the form and then start editing entity references, they expect that entities in that language will show up instead of the prior language. I know that is an "inconsistent" expectation given that form elements that are not AJAX would not yet work like that. But (again) what about preview before/after this patch. Would after preview all elements work with the new language?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -264,14 +264,10 @@ public function setFormDisplay(EntityFormDisplayInterface $form_display, FormSta
-  public function updateFormLangcode($entity_type_id, EntityInterface $entity, array $form, FormStateInterface $form_state) {
-    // Update the form language as it might have changed.
-    if ($this->isDefaultFormLangcode($form_state)) {
-      $langcode = $entity->language()->getId();
-      $form_state->set('langcode', $langcode);
-    }
-  }
+  public function updateFormLangcode($entity_type_id, EntityInterface $entity, array $form, FormStateInterface $form_state) {}

Since the method is named updateFormLangcode() but no updating of the form langcode happens here or elsewhere, should it document why?

Seems to be fine with me based on the history digging and lack of side effect discovery work done above :)

Sent for a retest. Update tests fail with config schema errors, so it completely looks unrelated:

fail: [Browser] Line 248 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
GET http://localhost/checkout/update.php/start?id=2&op=do_nojs returned 0 (0 bytes).

fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key block.block.bartik_account_menu:settings.cache failed with: missing schema

fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key block.block.bartik_breadcrumbs:settings.cache failed with: missing schema

... etc.

Same random fail as detailed in #20.

+++ b/core/modules/system/src/Tests/Form/FormLanguageTest.php
@@ -0,0 +1,102 @@
+
+    // Add an image field.
+    $this->drupalGet('admin/structure/types/manage/article/');
+    $this->drupalGet('admin/structure/types/manage/article/fields');
+    $this->drupalGet('admin/structure/types/manage/article/fields/add-field');
+    $edit = [
+      'new_storage_type' => 'image',
+      'field_name' => 'image_field',
+      'label' => 'image_field',
+    ];
+    $this->drupalPostForm(NULL, $edit, t('Save and continue'));
+    $this->drupalPostForm(NULL, [], t('Save field settings'));
+    $this->drupalPostForm(NULL, [], t('Save settings'));

we have a trait for this, or you could create the field using the API, need you don't need field_ui module, which should make the test faster.

I don't really see an explanation in the issue summary *why* this change is wrong and a bug and has to be changed. We *are* making a behavior change, and just because core doesn't fail doesn't mean that contrib/custom code won't have a problem with it.

I am not saying that the change here is wrong. I'm just saying we need to a better job at explaining why the current behavior is wrong. That might include documenting what the meaning of the langcode in $form_state actually is and why changing it is wrong. Otherwise I'm not sure we can get this into 8.1 as a major bugfix.

That's also visible in the test, which is basically "self-fullfilling". It doesn't expose an actual bug/problem with the current behavior (like, something being saved in the wrong language or so), it just asserts for the new behavior.

I think it would be important to get @Berdir's updated feedback on this since he did not have a chance for that since the update.

Looks like my comment here didn't make it.

No, I don't think that's needed. I didn't ask for those updates for me, at least not the issue summary updates, that's for those that will need to decide about committing this and against which versions. (Bugfix would imply fixing this in 8.1 too, but I'm not sure about that..)

Random fails on 8.1.x, the retest is already running.

fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key block.block.bartik_account_menu:settings.cache failed with: missing schema

fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key block.block.bartik_breadcrumbs:settings.cache failed with: missing schema

fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key block.block.bartik_content:settings.cache failed with: missing schema

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -52,7 +52,10 @@ public function form(array $form, FormStateInterface $form_state) {
+    // Prior to Drupal 8.1.4, this entity builder updated the form langcode.
+    // That has been found to be an issue in #2757003 and therefore the body of

@@ -248,11 +251,12 @@ public function setFormDisplay(EntityFormDisplayInterface $form_display, FormSta
+   * Prior to Drupal 8.1.4, this function used as an entity builder updated the
+   * form langcode. That has been found to be an issue in #2757003 and

@@ -264,14 +268,10 @@ public function setFormDisplay(EntityFormDisplayInterface $form_display, FormSta
+   * @deprecated in Drupal 8.1.4, will be removed before Drupal 9.0.0.

So we've just released 8.1.8 - imho the comments shouldn't reference release apart from the @deprecated one. Also I don't think the issue #2757003 should be referenced - we should just say why updating form the the langcode is wrong. Also I think maybe the correct BC behaviour is to not add the entity builder but leave the method functionally the same. And just document that it is no longer used and has caused problems with AJAX.

Also I'm still not convinced that the proper archaeology has been done to understand why it exists in the first place.

I just heard about this, sorry, I'll try to have a look to it tomorrow...

So, I picked up my whip and did some more archaeology: CEFI::updateFormLangcode() is a far descendant of entity_translation_entity_form_language_update, in fact I suspect it was added as part of the initial port, when it probably made sense, since we had no (Content) Entity Translation API at the time (and so we couldn't rely on ContentEntityInterface::language() for the active language).

This is the function body:

/**
 * Validation handler for the entity language widget.
 */
function entity_translation_entity_form_language_update($element, &$form_state, $form) {
  $handler = entity_translation_entity_form_get_handler($form, $form_state);
  // Ensure the handler form language match the actual one. This is mainly
  // needed when responding to an AJAX request where the languages cannot be set
  // from the usual page callback.
  if (!empty($form_state['entity_translation']['form_langcode'])) {
    $handler->setFormLanguage($form_state['entity_translation']['form_langcode']);
  }
  // When responding to an AJAX request we should ignore any change in the
  // language widget as it may alter the field language expected by the AJAX
  // callback.
  if (empty($form_state['triggering_element']['#ajax'])) {
    $handler->entityFormLanguageWidgetSubmit($form, $form_state);
  }
}

As you can see, it explicitly mentions AJAX requests and the second comment is encouraging, as it describes an issue similar to the one we are addressing here. The first comment makes me think this logic is no longer needed in D8, since we have a different way to initialize the form language.

Additionally, I couldn't find any explicit usage of the form language after form submission both in D8 core and D7 entity_translation and title code bases. However, in D7 form language may still be important after form submission because the core entity_language() function may rely on it:

/**
 * Entity language callback.
 *
 * This callback changes the entity language from the actual one to the active
 * form language. This overriding allows to obtain language dependent form
 * widgets where multilingual values are supported (e.g. field or path alias
 * widgets) even if the code was not originally written with supporting multiple
 * values per language in mind.
 *
 * The main drawback of this approach is that code needing to access the actual
 * language in the entity form build/validation/submit workflow cannot rely on
 * the entity_language() function. On the other hand in these scenarios assuming
 * the presence of Entity translation should be safe, thus being able to rely on
 * the EntityTranslationHandlerInterface::getLanguage() method.
 *
 * @param $entity_type
 *    The the type of the entity.
 * @param $entity
 *    The entity whose language has to be returned.
 *
 * @return
 *   A valid language code.
 */
function entity_translation_language($entity_type, $entity) {
  $handler = entity_translation_get_handler($entity_type, $entity);
  if (empty($handler)) {
    return LANGUAGE_NONE;
  }
  $langcode = $handler->getFormLanguage();
  return !empty($langcode) ? $langcode : $handler->getLanguage();
}

Given all that, I strongly suspect CEFI::updateFormLangcode() is no longer needed. If it weren't for the PHP doc mentioning use cases, I would be pretty sure about that. OTOH, maybe I didn't have any specific use case in mind, I was just thinking one may want to act before or after form language has been updated, and that's all.

To be safe, I'd suggest to keep the function around and working as it currently does and just make sure the initial form language is preserved when rebuilding the form (see the attached draft). I think this should be the least disruptive change and we may want to change the deprecation note to state that the function will be removed in D9 (not before).

Looking at the code, my main remark is about the test itself: we already have EntityTranslationFormTest, so I guess this should be a new method on that class and deal with the test entity and not nodes, for consistency. Also, I agree with Berdir that the test should outline the consequences of this misbehavior, for instance that submitting the form will create a new translation instead of updating the default language (if that's actually the case).

Oops, I forgot the patch draft

Are you sure? I tested that code and it seemed to be working. Well, at least the form was rebuilt with the proper language...

Beside that I am not really sure that it is fine to have the updateFormLangcode function and then somewhere else reseting what the functions has done. If a committer says this is fine then I would introduce a new entity builder which runs exactly after the updateFormLangcode one and resets what the updateFormLangcode has done.

What you are proposing is not the same of what I coded: the form language is reset only when rebuilding the form, but validation and submission handlers will always find the updated form language code if the form is actually being submitted.

Changing to entity system as it is not a bug in the form component.

For sake of full disclosure: @hchonov and I work for the same employer.

I fully agree with @hchonov's position and I disagree with @plach's last comment and patch.

My thoughts on this:
We can never change the default language code of the entity, which means that when you are changing the value of the language field in a form, all other form fields will be updated in the translation in the language that you just selected. Therefore, I can understand the impulse to want to provide a proper translation form (thus, updating the form language code) when rebuilding the form, to properly distinguish in the user interface the fact that - as explained in the previous sentence - you are editing translation values.

However, operations in forms are not atomic. When submitting a form (Ajax or not) you are never just updating a single value. So there will always be inconsistent states as values might have been updated before changing the language code, while changing the language code (i.e. in the same submission) or after changing it. So there is no way that we can communicate to the user what is happening in a reliable and sane way. Our current solution of just changing the language during form validation/submission from underneath your feet is not sufficient as can be easily seen from the issue summary.

So while it is a behavior change, I think it is the only sensible thing to remove the updateFormLangcode() call. Solving this properly would involve much bigger changes, if it is even possible at all. I am thinking that we would need to get rid of the simple select element for the language and provide a dedicated button or something, but even then it is not exactly trivial to define a proper and intuitive behavior for the user. As I said, that's clearly out of scope here.

So it is a bug fix (again see the issue summary), but fixing it requires a behavior change, so I think we should only get this into 8.3.x. Modules that do rely on the new entity language can access that through the form state already, and they can in fact do that in a way that will work with and without this patch. So if we get this in to 8.3.x soon modules will have ~6 months to be fixed to properly fetch the language code.

Would love to get some more thoughts on this by @plach and @Berdir, though.

Edit: I didn't know that Content Translation prevents you from editing translations by just switching the language code (which is great!). The described problem still applies to adding translations, though.

@tstoeckler: @hchonov:

Let me try to clarify my position: as stated in #37, theoretically I agree that we can remove the ::updateFormLangcode() method, I'm just unsure whether we are allowed to do that by the current BC policies. Hence I was trying to find a solution that would not imply a change in the current behavior, even if the current behavior does not make much sense to me.

I'll talk to the committers to figure out a way forward.

Discussed this with @catch, @Berdir, @hchonov in IRC: we agreed to wait for @Berdir to try an alternative fix not implying a behavior change. If there's no way to achieve that we will move on with the current approach, that got @catch's approval, if there's no other way forward.

Btw, during the discussion @catch introduced the new

'piss off berdir as little as possible' bc policy ;)

My remark on the test from #37 still stands, unless @Berdir provides an alternative patch.

In both cases needs work ;)

Just a note, that I was under a wrong impression regarding what actually happens when you change the value of the language selector on the form. So I agree that we should spend more time trying to understand this problem space and am more or less on one page with @Berdir.

#2675010: Cloned entity will point to the same field objects if the clone was created after an entity translation has been initialized now has a patch that works without this change. Not everything is 100% clear over there yet, but I would suggest we either close this as won't fix or move to 9.x if we agree on my approach in the other issue.

So at the meeting we did not agree on a solution, but did agree that it's major, so that's something ;-)

Hopefully won't be long, but let's mark this postponed on #2675010: Cloned entity will point to the same field objects if the clone was created after an entity translation has been initialized.

> tstoeckler commented 11 January 2017 at 15:30
> Hopefully won't be long, but let's mark this postponed on #2675010: Cloned entity will point to the same field objects if the clone was created after an entity translation has been initialized.

Wellllll...

Setting this to active again, but I would need to catch up on the long discussion to figure out if this is still an issue.. apparently it at least wasn't a priority anymore for either of you ;)

***EDIT*** my test environment is too dirty, I have to rebase. Followup later if there's a need.

Ok guys, I have a use case where I need this api function or something similar to it. (updateFormLangcode)

I'm currently helping out with the entity_translation_unified_form module, which is very similar to the multi form display module ( mfd ) , the idea is to render node edit forms with all languages content on one page. ETUF for short (entity_translation_unified_form) module does work in D8 with the exception of image fields precisely I am pretty sure due to ajax calls being made in only the form language. So, what I would need to figure out or maybe an api for, is a way to say to the form element to run in the intended language , keeping in mind there could be 2 or 3 languages, so in this case, the image widget for uploading images (translateable enabled) so that the ajax call is posted in the correct language.

Now, I didn't know about this api until now and I haven't tried it, but ya see my issue description and screenshot and see the contrib module we're working on

all this worked with the D7 version of this module, but having problems getting the D8 version to co-operate fully.

so there's two contrib modules that can do this, but they both have the same problem according to my test environment anyway.
It's for managed files upload, the image upload widget.

Contrib module 1)

https://www.drupal.org/project/mfd

Contrib module 2)

https://www.drupal.org/project/entity_translation_unified_form

see a screenshot for a quick illustration:

multilingual form display (mfd) module screenshot (similar to the entity_translation_unified_form module (ETUF)

out of these two modules, the ETUF approach is probably the simplest , I just have to resolve this ajax issue.

***END EDIT***

This core issue turned out not to make a difference in our case, sorry for the noise. I found a workaround for my use case, although it looks like some sort of a core bug but not what I had originally suspected, not sure where the core bug is in my use case but my contrib fix is a workaround to a core glitch. I fixed the symptom.

Please disregard my previous comment above.

I tested this on Drupal 9.5.x, standard install, with Italian and Spanish instead of French and German, but I was unable to follow the steps in the issue summary at step 5. Step 5 is "Go to edit of the newly english translation." except the last translation created was in Italian. Does this mean to add an english translation of the Italian. I played around with this but wasn't able to reproduce the problem.

I then looked at the patch and there is a test, so I got that to run in Drupal 9.5. and it fails. So, if the test is correct there is something wrong here.

I think the next step is to confirm that this is a problem. I have updated the IS.

Can anyone confirm this is still an issue?

Thanks!

I meant to upload the test patch.

Since there hasn't been a follow up in a year going to close out for now. If still a valid bug though please reopen, maybe updating issue summary with additional steps to trigger.

Thanks all!