Integrity constraint violation when translating body field

I also reproduced this earlier several times, and we have seen people reproducing it in Prague at the sprint. (There may be other duplicate copies of this) While the translation is actually saved, there is no way out of this error, you need to navigate manually away. Therefore uping to critical.

I found out what is causing this: it seems we have some weird issue with field item references, basically we are getting different revision ids per language in the entity object, aw :(

Still looking for the root cause.

Adding more tags.

I think the two issues might be related.

The attched patch should fix the issue.

Since the test posted above more or less replicates what we have now, but just for nodes, and given that the bug lives in ContentEntityBase, I'd like to try to provide test coverage for this in EntityTranslationTest. Couldn't reproduce the issue so far, though.

Just a note: this does not happen if you create from scratch a plain content type with just the body field. I usually run the minimal install so I didn't experience this issue previously.

I'm not yet familiar with the Drupal 8 core, but after doing some late night and morning research on this issue I ended up with the following:
When the new revision get created in the FieldableDatabaseStorageController->saveRevision the new vid is passed only for the active language, which in this particular flow is the german (de).

Update:
1. keep the active language revision id up to date (accidentaly removed in the earlies patch)
2. include pfrenssen's test

@nyirocsaba:

Sorry, but I think the approach you are following is not correct: untranslatable fields are not supposed to have different values per language, AAMOF currently we are creating references so that the field item objects for the various languages are always the same, see ContentEntityBase::getTranslatedField().

The fact that right now we can somehow store different revisions ids per language in the same entity object is completely wrong and will cause huge problems.

What we need to fix is the root cause, which seems to lie in the entity cloning process, although I am not able to reproduce the bug in the entity translation tests. However, the proper fix should be something along the lines of #8.

@Plach: are you not able to repro this issue? I have installed latest dev from scratch with default install (not the minimal) and than you run directly into these issues, once translation get's enabled for article.

I am not able to reproduce the issue in a test case dealing only with the entity object.

(review of @plach's #8)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -904,10 +904,21 @@ public function __clone() {
          foreach ($this->fields as $name => $properties) {
            foreach ($properties as $langcode => $property) {
   

   While we're in here, the variable names seem really misleading here. What is called $properties is an array of (langcode => FieldItemList for the langcode). So this should be:

   foreach ($this->fields as $name => $items_by_langcode) {
     foreach ($items_by_langcode as $langcode => $items) {
   

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -904,10 +904,21 @@ public function __clone() {
   +            $this->fields[$name][$langcode]->setContext($name, $this);
   

   Looks like this is done in both cases, so it could go out of the if / else block ?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -904,10 +904,21 @@ public function __clone() {
   +          // Untranslatable fields have multiple references for the same field
   +          // object keyed by language. To avoid creating different field objects
   +          // we retain just the original value, as references will be recreated
   +          // later as needed.
   +          elseif ($langcode == Language::LANGCODE_DEFAULT) {
   +            $this->fields[$name] = array($langcode => clone $property);
   +            $this->fields[$name][$langcode]->setContext($name, $this);
   +          }
   

   In the case of untranslatable fields, we only act on $langcode == LANGCODE_DEFAULT because that's the only langcode that's supposed to be present in $this->fields[$name], right ?
   So maybe in the case of untranslatable fields, no need to foreach on langcodes, just do the one needed stuff on LANGCODE_DEFAULT ? Would make the logic easier to follow IMO.

I was not able to reproduce the issue in the Entity Translation tests but I added test coverage anyway. This merges also the test provided by @pfrenssen with the existing node translation tests. We need quite some refactoring to make those pass with the standard profile (including fixing a bug caused by form cache in the entity form controller).

This should also address @yched's review: I opted for using $values instead of $items_by_langcode (which is quite verbose) for consistency with the field caching code.

Re-uploading the test-only patch (should fail tests).

Come on, bot :(

Last try for tonight

Don't know why those did not go for tests... maybe some undocumented file name pattern?!

Finally :)

@yched, @hass, @nyirocsaba, @pfrenssen any feedback on the patch?

20: et-clone_refs_broken-2129039-16-should-fail-tests-only.patch queued for re-testing.

Oh, sorry - the fails for #20 were expected, actual patch is in #16.
Reviewing.

Yup, looks good :-)
(although, not sure why this is filed for content_translation.module while the fix is in ContentEntityBase ?)

RTBC as per #26 - #16 is the patch to commit.

16: et-clone_refs_broken-2129039-16.patch queued for re-testing.

Let's get this in the Monday alpha :)

Still get this when adding a translation:

At URL: node/2/translations/add/de/en

InvalidArgumentException: Invalid translation language (en) specified. in Drupal\Core\Entity\ContentEntityBase->addTranslation() (line 691 of core\lib\Drupal\Core\Entity\ContentEntityBase.php).

I have created an English translation of a German article. I also get Article English article 2 has been updated.

EDIT: And afterwards I see two English articles listed on project home (English UI) where only one should be shown. Also with the same URL alias... it's just shown twice. I guess every translation will add an item. If I switch to German I see two times the German article.

I saw that in Prague but I was not able to reproduce it reliably. Can you post the steps? I suspect it might be a different issue.

And afterwards I see two English articles listed on project home (English UI) where only one should be shown. Also with the same URL alias... it's just shown twice. I guess every translation will add an item. If I switch to German I see two times the German article.

See #2006606: Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations.

It's easy to repro here. Just add a translation to an existing node and save.

UHHH, uncheck Create new revision!

Just playing around to look into the untested files issue. Please ignore.

Created follow up #2137815: InvalidArgumentException: Invalid translation language specified.

Trying to reproduce #2136501: Sometimes patches are not sent for testbot. ... sorry for the noise. :( Still RTBC, and the patch in #16 is still the one to commit.

Bizarre ... can't explain why the bot wasn't picking up the patches, but can't reproduce it either. :(

Back to RTBC and restored the patch from #16 to the top of the table.

Committed and pushed to 8.x. Thanks!

So it seems #41 was committed and not #16: http://drupalcode.org/project/drupal.git/commitdiff/b0ed757

?!

Shoot, I am so sorry! This is why I should not commit patches while on the phone. :P~

Reverted previous; committed and pushed #16 to 8.x. Let's see how it likes that. :)

Thanks all!