Cloned entity will point to the same field objects if the clone was created after an entity translation has been initialized

Looks like a very strange bug, but a straightforward solution. The code is already doint the same for translations:

      // Ensure the translations array is actually cloned by overwriting the
      // original reference with one pointing to a copy of the array.
      $this->clearTranslationCache();
      $translations = $this->translations;
      $this->translations = &$translations;

Committed 5e942cf and pushed to 8.1.x and 8.2.x. Thanks!

After this commit the tests for Paragraphs got broken. https://www.drupal.org/pift-ci-job/345975
Is there any change we should make after this commit?

Testing a bit found the steps to reproduce.
1. Enable translation for article
2. Create an English article
3. Translate it to German and save
4. Edit the English translation, first change the language to french and add an image
5. Node supposes to be in French but remains in English
I think it can be something related with ajax, because in the step 4 we add the image first and then change the language to french, the change is saved.

Should we open a followup issue or revert this commit?

@yongt9412: sounds like a pre-extisting bug that only existed due to this bug obscuring it to me?!

It might or might not have been a pre-existing problem, but it did *not* happen like that before this commit.

Given that that is IMHO a more severe problem than what was fixed here (I can't see any immediate user-facing bugs here, if there are, they should possibly be listed), I'm wondering if reverting and making sure we have test coverage for that scenario isn't the better approach.

That bug does seem to be worse than what we've fixed. It needs investigation and resolution.

I added some tests. Let's see what test bot thinks.

The node is created in english, then translated to french, then the english translation is edited and the language is changed to german. As a result, german and french has to exist, not english.

Not impossible, possible and passing on HEAD :)

Yay, thanks @hchonov, you are again amazing :)

Marking this as postponed on #2757003: Entity builder ContentEntityForm::updateFormLangcode changes the form language code during ajax calls. As far as I can tell, the latest patch here contains the patch from the other issue, also, so...

Ok, here we go. This is not completely right yet either, but I think this is on a better way. Both ParagraphsTranslationTest and the new test here are passing with this.

Note that the current behavior in the UI is like this:
1. For a entity without translations, we allow setting language to all values.
2. For the original entity language with translations, we allow to set to all languages that do not exist yet.
3. Existing translations can't have their language changed.

I think 2 vs. 3 is a bit strange, they should possibly work in the same way but they don't.

The basic change that I'm making is to fix updateFormLangcode() instead of removing it (based on how I understand it should work). I'm always updating the langcode and I'm also updating the default langcode if that changed. The logic there is a bit strange, I guess I could just unconditionally set it based on untranslated.

This means that changing the language doesn't break isDefaultFormLangcode(), the current behavior there why it was so complicated for us in paragraphs to make it work.

What's weird is that something is wrong with the entity that we build and the entity that is available in hook_form_alter(). Because validate builds a new entity, uses that for validation but doesn't update $this->entity. That means a form_alter that is then called during form rebuild is actually working with the old object. That means content translation still incorrectly changes the title when you switch the language. That seems like an issue that is should be addressed in its own issue. I believe we could actually fix the fact that we build the entity twice (once in validate, once in submit).

The reason why removing the form_state langcode update worked is because of the bug above, when it no longer changed, then it was the same as the old wrong language on the entity, so content_translation actually didn't see a change and considered it to be still the original untranslated language.

What I don't get is the difference between form language and entity language. @plach said in IRC that in 7.x, form language must always be the entity language. The same is now true for 8.x and I don't understand why it should be any different. Of course, then the question is why the form langcode concept exists in the first place. There is one case in \Drupal\content_translation\ContentTranslationHandler::entityFormAlter() where the two are compared, but IMHO, as the comment says, that should simply do a isDefaultFormLangcode() like it does for the language field access.

I didn't run any other content_translation tests, lets see what happens.

The interdiff won't be very useful as it is against the previous patch that removed the updateFormLangcode() method.

I'v confirmed that just doing $this->entity = $entity in ContentEntityForm::validateForm() fixes the page title. I'll experiment with that in #2799637: Document that #entity_builders and overrides of EntityForm::copyFormValuesToEntity() must be idempotent.

This patch is actually not yet changing that and I disagreee with that behavior being expected. We are not rebuilding the original form, we are always rebuilding the form based on whatever changes we made.

You're welcome to provide an example as to why rebuilding with the changed entity is wrong and what would fail, so far you only said it is wrong, not why.

The problem is not that we change the langcode, it's that we make inconsistent changes that then confuse content_translation and the helper methods we have on content forms.

Config forms already work like this, only content entity forms are different (and IMHO broken).

Opened #2813073: Don't update $this->entity in EntityForm::afterBuild() for the #after_build thing. The short answer is that we have to live with this inconsistency for now.

Will look at #2811841: Add test coverage ensuring user input is mapped on the correct form elements when elements are reordered now...

First stab at expanding the issue summary, will continue later.

We also agreed at Dublin that this is a major bug, because of the potential impact on data integrity.

Ok, slowly getting back to this after #2811841: Add test coverage ensuring user input is mapped on the correct form elements when elements are reordered is in. Yay, one small step forward and many things learned :)

Looking at #41 now. My approach has two test fails. The first one about the wrong title is something that I already pointed out in #36:

What I don't get is the difference between form language and entity language. @plach said in IRC that in 7.x, form language must always be the entity language. The same is now true for 8.x and I don't understand why it should be any different. Of course, then the question is why the form langcode concept exists in the first place. There is one case in \Drupal\content_translation\ContentTranslationHandler::entityFormAlter() where the two are compared, but IMHO, as the comment says, that should simply do a isDefaultFormLangcode() like it does for the language field access.

So doing exactly that makes that test pass, also with my approach. What we need for that to work is consistency. In your approach, it is consistently the old original langcode, in my case, it is consistently the new langcode, both cases result in it correctly detecting a translation vs editing the original.

Attaching a patch with that fix to demonstrate that. *This is not a final patch.*

The second is more interesting. It fees a bit like that is specifically written to fail with my approach. Which might be OK if that is an actual valid use case, but I'm not convinced about that yet. Lets look a bit at that test:

1. It adds a custom field that is only visible when the language is either EN or FR.
2. On editing an existing entity in the default translation (EN), we change the language to german.***
3. Then we save.
4. And now we ensure that the value, which is only supposed be there for EN/FR was saved when saving the entity as a german translation?

Yes, that fails with my code, but a) It would be possible to write code that wouldn't fail (for example currently, by getting the language of the entity, not the form language) and b) That logically doesn't make sense to me. Yes, when using ajax, that field is still visible but that's a bug in that specific logic and it would have to find a way to update it. For example with #states on the langcode field, or trying to find a way to attach another ajax command to the response. It also seems more like a a test case for #2757003: Entity builder ContentEntityForm::updateFormLangcode changes the form language code during ajax calls than for this issue.

*** As also commented in #36, the fact that you can change the language of the original language but not of a translation seems very strange to me and I think we should consider to remove that feature. Or allow to change the language of a translation too. The current logic just doesn't make too much sense there.

In #41, @hchronov wrote this:

If the form had to be built with the entity built based on the new user submitted values then we would had $this->entity set in the validate function so that the form is intentionally rebuilt with the new generated entity based on the user input, but I am pretty sure it has been intentionally left unmodified.

You might be putting too much trust in the people who wrote and reviewed that code (which includes me, so I'm allowed to say that I think :)). There clearly are obvious bugs with form langcode stuff, whether or not it should exist at all or not. So what exactly is intentional and what isn't really isn't clear.

Here is a suggestion for a compromise to continue moving forward in small steps:

Lets leave it for #2757003: Entity builder ContentEntityForm::updateFormLangcode changes the form language code during ajax calls to decide if we should do that or not. What is clear is that a) right now the call exists and there are contrib modules like paragraphs relying on that (@tstoeckler is working on code that doesn't need that anymore, which is great but it's not quite finished yet). b) Its implementation is broken and doesn't handle repeated builds at all.

My patch keeps the status quo in keeping that call and it fixes bugs in the current implementation which allows us to fix the clone bug here without introducing new regressions in core or risking them in contrib/custom code.

I also never proposed to do "I believe we could actually fix the fact that we build the entity twice (once in validate, once in submit)." in this issue, I was just thinking out loud and #2799637: Document that #entity_builders and overrides of EntityForm::copyFormValuesToEntity() must be idempotent already proved that we can't do that, and we have #2813073: Don't update $this->entity in EntityForm::afterBuild() to investigate that part.

Given that, my suggestion would be to do this issue by fixing updateFormLangcode() and leave out the test coverage from #41 and #49 and move that to the mentioned issue instead. 2675010-49-tests-only.patch AFAIK proves that the things tested by it are failing in HEAD as well and are *not* caused by my suggested (maybe temporary fix) fix for updateFormLangcode.

Thank you for the update!!

I'm not yet sure about the validity of the test, so I will not (yet) comment on it in either direction.

I am in general fine with the plan. It is based on the assumption, though, that the remaining test failure is invalid, which at least in my brain has yet to be determined ;-).

I would like to fix the form langcode handling and the content translation stuff in a separate issue, but as far as I can tell that's not actually possible, as that is directly influenced by the bug at hand. :-/ So (again under the above assumption) I think your patch is fine.

I am fine with the rest happening in #2757003: Entity builder ContentEntityForm::updateFormLangcode changes the form language code during ajax calls. I am by now fairly certain that we want to remove usages of and deprecate getFormLangcode() but I guess that's something to be discussed over there.

Ok, removed the failing test method. And restored that method call. I did that because the next thing we do is calling getFormLangcode(), which itself calls initFormLangcodes() again, so it really is pointless. But yeah, not related/required, so removed that changed.

Not sure about keeping the the en_fr_only field in the test, it is still used in testTitleDoesNotChangesOnChangingLanguageWidgetAndTriggeringAjaxCall() but only to check the initial value, which I think isn't too useful but kept it for now.

Thank you!

Today is a good day.

That is a fair point. I still think this should go in the way it currently is, because the behavior change this introduces is less of an API-change than what it otherwise would be. Instead of explaining that in detail in this comment, though, this should be explained in the issue summary and in a change record, so marking needs work for that. Will try to get on that today/tomorrow.

I also just found that Inline Entity Form uses $form_state->get('entity_default_langcode') as well, so we need to check whether we haven't broken anything. It seems that Inline Entity Form suffers from a similar problem as core's Content Translation in that it currently sometimes thinks that the form is a translation form although it actually isn't and that this issue would (not so) magically fix that. But that needs to be verified before this can go in.

So what currently happens is in order not to break Paragraphs we break Inline Entity Form.

That's not true, at least not necessarily. As I've explained it might also be that this actually fixes a bug in Inline Entity Form. But we definitely need to find out which it is before proceeding here.

I am updating the default langcode exactly to make $form_state->get('langcode') != $form_state->get('entity_default_langcode') work as expected. That's exactly what \Drupal\Core\Entity\ContentEntityForm::isDefaultFormLangcode() does and it now works as expected.

We might be changing/affecting IEF but IMHO in a positive way. Changing the language does *not* mean we are translating, it means we are changing the language, we are still editing the default language.

That remove access is the same thing as paragraphs does. You're only allowed to remove if you are not editing a tranlsation. You are still allowed access to delete when you change the language.

The fact that isDefaultFormLangcode() was broken and inconsistent before is why paragraphs did not use that and is exactly the bug that I am fixing in updateFormLangcode().

It seems the latest patch is now failing badly...

Had a look at the fails but didn't figure out the problem yet.

Looks like it saves a new revision, but the $entity->getUntranslated() still has a revision_id. Why I have no idea, it is an untranslatable field, so shouldn't matter. Maybe something weird happen with cloning and the reference to the translation object?

According to git bisect, it is #2669802: Add a content entity form which allows to set revisions that broke this.

Indeed, thanks! This fixes at least one of those test fails. Lets see about the others.

This is the combined patch, had a small conflict on the test.

Updated the issue summary and added a change notice.

I also verified that the Inline Entity Form tests pass with this applied.

-> RTBC!!

I haven't provided any patches here, and this patch has been approved by both @Berdir and @hchonov so I am comfortable pulling the trigger on this.

Thanks to the two of you for all your time on this and your persistence and of course to everyone else involved with this, as well.

Committed 4b65a2b and pushed to 8.3.x. Thanks!

I'm not committing to 8.2.x just yet because of all the earlier issues.

diff --git a/core/modules/content_translation/tests/modules/content_translation_test/content_translation_test.module b/core/modules/content_translation/tests/modules/content_translation_test/content_translation_test.module
index 7093947..d07549d 100644
--- a/core/modules/content_translation/tests/modules/content_translation_test/content_translation_test.module
+++ b/core/modules/content_translation/tests/modules/content_translation_test/content_translation_test.module
@@ -1,5 +1,10 @@
 <?php
 
+/**
+ * @file
+ * Helper module for the Content Translation tests.
+ */
+
 use \Drupal\Core\Form\FormStateInterface;
 
 /**

Fixed missing docblock on commit.

Testbot tried to re-test on 8.3.x. Submitted a test run for 8.2.x to see if it passes there, not 100% sure if we should commit it to the stable branch..

Ok, conflicted with the loadedRevisionId property.

Patch looks good.

There is not going to be another 8.2.x release. This is in 8.3.x and 8.4.x