Problem/Motivation

OverviewTerms doesn't respect the current content language when building the form and term urls and names are generated based on the terms default language.

Proposed resolution

The entity manager is passed in the constructor, so we just have to keep a reference of it and when working with the terms first get the term in the language determined by the current context by executing:
$term = $this->entityManager->getTranslationFromContext($term);.

The EntityManager service is deprecated and will be removed before Drupal 9.0.0 and the function getTranslationFromContext is provided by the EntityRepository service, but the constructor of OverviewTerms now has as a parameter the EntityManager service and to add another constructor parameter will be API break. To allow for the patch to get in 8.0.x or 8.1.x we still use the EntityManager service, which is already there instead of altering the constructor.

Remaining tasks

Write tests.

User interface changes

Terms and their urls are now pointing not to the default term language, but to the translation language determined by the current context.

API changes

none

Data model changes

none

CommentFileSizeAuthor
#28 interdiff-25-28.txt604 bytesjofitz
#28 2671058-28.patch3.05 KBjofitz
#25 interdiff-22-25.txt602 bytesjofitz
#25 2671058-25.patch3.06 KBjofitz
#22 interdiff19_22.txt1.62 KBMunavijayalakshmi
#22 2671058-22.patch3.05 KBMunavijayalakshmi
#19 2671058-19.patch2.96 KBtameeshb
#17 2671058-14.patch2.95 KBtameeshb
#15 2671058-12.patch2.97 KBtstoeckler
#13 2671058-12--tests-only.patch1.66 KBtstoeckler
#12 interdiff-2671058-6-12.txt1.52 KBpguillard
#12 2671058-12.patch2.97 KBpguillard
#6 diff-2-6.txt153 byteshchonov
#6 2671058-6.patch1.31 KBhchonov
#2 2671058-2.patch1.31 KBhchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Issue tags: +Quickfix
FileSize
2671058-2.patch1.31 KB
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Active » Needs review
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Issue summary: View changes
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Issue summary: View changes
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
FileSize
2671058-6.patch1.31 KB
diff-2-6.txt153 bytes

Rerolling the patch as it could not be applied anymore because of a comment change made in #2471689: Entity API documentation should consistently refer to handlers rather than controllers.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

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

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Needs work
Issue tags: +Needs tests

We should have some tests for this.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pguillard’s picture

pguillard CreditAttribution: pguillard commented
Status: Needs work » Needs review
FileSize
2671058-12.patch2.97 KB
interdiff-2671058-6-12.txt1.52 KB

Here is a patch with a test

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Issue tags: -Needs tests
FileSize
2671058-12--tests-only.patch1.66 KB

The test looks great to me. Uploading a tests-only patch, to prove that it is correct.

Status: Needs review » Needs work

The last submitted patch, 13: 2671058-12--tests-only.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Reviewed & tested by the community
FileSize
2671058-12.patch2.97 KB

Perfect! Thanks again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2671058-12.patch, failed testing.

tameeshb’s picture

tameeshb CreditAttribution: tameeshb at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2671058-14.patch2.95 KB

array() -> []

Status: Needs review » Needs work

The last submitted patch, 17: 2671058-14.patch, failed testing.

tameeshb’s picture

tameeshb CreditAttribution: tameeshb at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2671058-19.patch2.96 KB

Patch rerolled

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/taxonomy/tests/src/Functional/TermLanguageTest.php
@@ -107,4 +107,33 @@ function testDefaultTermLanguage() {
+  function testTermTranslatedOnOverviewPage() {

    Just seeing this now, should have seen this earlier:

    1. The method should have a public scope (i.e. put "publich function" instead of "function"

    2. The method should have a docblock. Something like:

    Tests that translated terms are displayed correctly on the term overview.

  2. +++ b/core/modules/taxonomy/tests/src/Functional/TermLanguageTest.php
@@ -107,4 +107,33 @@ function testDefaultTermLanguage() {
+    $edit = array(
...
+    $edit = array(
...
+    $term->addTranslation('bb', array(

    This should now use the short array syntax.

Munavijayalakshmi’s picture

Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commented
Assigned: Unassigned » Munavijayalakshmi
Munavijayalakshmi’s picture

Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commented
Assigned: Munavijayalakshmi » Unassigned
FileSize
2671058-22.patch3.05 KB
interdiff19_22.txt1.62 KB
Munavijayalakshmi’s picture

Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commented
Status: Needs work » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Needs work 
+++ b/core/modules/taxonomy/tests/src/Functional/TermLanguageTest.php
@@ -107,4 +107,36 @@ public function testDefaultTermLanguage() {
+    $this->assertPattern('|<a[^>]*>' . $translated_title . '</a>|', 'The term language is correct');
+  }
 }

Ahh sorry, missed one more coding standards error:

There should be an empty newline before the last closing brace (the one for the class)

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2671058-25.patch3.06 KB
interdiff-22-25.txt602 bytes

Added newline.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Needs work 
+++ b/core/modules/taxonomy/tests/src/Functional/TermLanguageTest.php
@@ -107,4 +107,37 @@ public function testDefaultTermLanguage() {
+  ¶

The whitespace should be removed :).

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented 
+++ b/core/modules/taxonomy/tests/src/Functional/TermLanguageTest.php
@@ -107,4 +107,37 @@ public function testDefaultTermLanguage() {
+  ¶

Sorry, to be so annoying but the newline now contains trailing whitespace, can you please remove that?

Sorry, I promise I will RTBC the next patch. ;-) And yes, we really need better tools so that some of this stuff is automated, but we're getting there, at least we now have coding style checks on Drupal.org...

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2671058-28.patch3.05 KB
interdiff-25-28.txt604 bytes

I blame phpStorm! That is a sign I should go home for the night! White space removed.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community

Well, you should go home content, then. RTBC! Thanks a lot

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d370ee7 to 8.4.x and 2008561 to 8.3.x. Thanks!

Nice find and nice test coverage.

  • alexpott committed d370ee7 on 8.4.x 
    Issue #2671058 by Jo Fitzgerald, hchonov, tstoeckler, tameeshb,...

  • alexpott committed 2008561 on 8.3.x 
    Issue #2671058 by Jo Fitzgerald, hchonov, tstoeckler, tameeshb,...

  • xjm committed 252ff76 on 8.4.x 
    Revert "Issue #2671058 by Jo Fitzgerald, hchonov, tstoeckler, tameeshb,...

  • alexpott committed a46ef96 on 8.4.x 
    Revert "Revert "Issue #2671058 by Jo Fitzgerald, hchonov, tstoeckler,...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Sorry, the revert here was an accident reverting another issue committed around the same time. It's un-reverted now (i.e. still fixed).

Status: Fixed » Closed (fixed)

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