Problem/Motivation

When doing an entity query on a revisionable and translatable entity type, which has a field that is translatable but not revisionable the data table and the revision data table need to be queried. Currently the join to query both of these tables is done on the entity ID, which can potentially mean the record for the wrong language.

Proposed resolution

Update the join in the query to use both the entity id and the langcode.

Remaining tasks

Review.

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#7 test-only.patch2.35 KBtimmillwood
#7 interdiff.txt2.9 KBtimmillwood
#7 non_revisionable-2728403-7.patch3.28 KBtimmillwood
#3 non_revisionable-2728403-3.patch2.25 KBtimmillwood
#3 interdiff.txt1.65 KBtimmillwood
#2 non_revisionable-2728403-2.patch683 bytestimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Active » Needs review
FileSize
non_revisionable-2728403-2.patch683 bytes

This patch updates the non-revisionable field in \Drupal\entity_test\Entity\EntityTestRev::baseFieldDefinitions to be translatable, and therefore should cause this issue to happen. Otherwise I'll need to update the patch to add a test to cause this issue to happen.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
FileSize
interdiff.txt1.65 KB
non_revisionable-2728403-3.patch2.25 KB

Here's a test for this

Status: Needs review » Needs work

The last submitted patch, 3: non_revisionable-2728403-3.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Parent issue: » #2725433: WI: Phase A: Use the revision API in more places

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
non_revisionable-2728403-7.patch3.28 KB
interdiff.txt2.9 KB
test-only.patch2.35 KB

When joining the revision and data tables we were only joining on id and not langcode.

Status: Needs review » Needs work

The last submitted patch, 7: test-only.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
Status: Needs work » Needs review

Perfect!

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Issue tags: +Needs issue summary update

I think that we need a better explanation of what actually happens. I think that the fields are being saved correctly but not loaded again correctly due to the missing join condition for the language code key.
Beside that +1.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
Issue summary: View changes

Making the issue summary a little more understandable.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
Issue tags: -Needs issue summary update
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityNonRevisionableTranslatableFieldTest.php
@@ -0,0 +1,47 @@
+

Not needed empty line between the both assertions, which could be removed on committing this.

Otherwise everything else looks fine.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Component: language system » entity system

  • catch committed 50ab536 on 8.3.x 
    Issue #2728403 by timmillwood: Non-revisionable translatable fields on...

  • catch committed 8088a24 on 8.2.x 
    Issue #2728403 by timmillwood: Non-revisionable translatable fields on...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +D8MI, +language-content

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.