Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because translations aren't used
Disruption Not disruptive, just internal changes ...

Problem/Motivation

Breadcrumb doesn't get localized when displaying parent terms. They are not translated according to the current user language.

Proposed resolution

Fix the breadcrumb builder in the taxonomy module.

Remaining tasks

Review the fix.

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Version: 7.15 » 8.x-dev
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs backport to D7

Can you provide steps to reproduce?

cesareaugusto’s picture

I got breadcrumb set for taxonomy terms. They display HOME > FATHER > CHILD. Main language is Italian. All my terms are translated into English and Spanish too. When I display pages in English or Spanish, the FATHER and CHILD terms in the breadcrumbs are always in Italian (the main language). Taxonomy pages are displayed through Panels.

Do you need any more information?

jhedstrom’s picture

Issue summary: View changes

Is this still an issue in 8?

floydm’s picture

It appears to still be an issue in D8.

I just set up tags with [ 1 => Father, 2 => Son, 3 => Cat ] each nested under the previous and translated. See the attached image for the results.

jhedstrom’s picture

Status: Postponed (maintainer needs more info) » Active
jhedstrom’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
14.47 KB

Here's a test that shows this is an error.

There is one problem with the test, in that hu/taxonomy/term/3 doesn't show the translated version, while hu/node/1 shows translation. I'm digging into that now.

Status: Needs review » Needs work

The last submitted patch, 6: breadcrumb-term-translation-1799820-06-WILL-FAIL.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
13.91 KB

This updates the test to only fail on the breadcrumb, as expected.

The existing term translation test wasn't properly marking terms as translatable (which didn't impact that test).

jhedstrom’s picture

FileSize
1.31 KB
15.21 KB

And here is the fix. I'm not sure if this is the best way to get the translations or not.

The last submitted patch, 8: breadcrumb-term-translation-1799820-08-WILL-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: breadcrumb-term-translation-1799820-09.patch, failed testing.

jhedstrom’s picture

Slight logic error in the test (wrong front page path). Attached again are just the failing test, and then the fix with the test.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks proper to me.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -35,8 +36,10 @@ public function build(RouteMatchInterface $route_match) {
    +    $language = \Drupal::service('language_manager')->getCurrentLanguage(LanguageInterface::TYPE_CONTENT);
    +    while ($parents = \Drupal::entityManager()->getStorage('taxonomy_term')->loadParents($term->id())) {
    

    ... can we please inject these services?

  2. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -35,8 +36,10 @@ public function build(RouteMatchInterface $route_match) {
    +      $term = $term->getTranslation($language->getId());
    

    Don't we want to use $entity_manager->getTranslationFromContext ? We need to take care about fallback candidates potentially ...

  3. +++ b/core/modules/taxonomy/src/Tests/TaxonomyTranslationTestBase.php
    @@ -0,0 +1,111 @@
    +
    +  /**
    +   * Enables translations where it needed.
    +   */
    +  protected function enableTranslation() {
    +    // Enable translation for the current entity type and ensure the change is
    +    // picked up.
    +    \Drupal::service('content_translation.manager')->setEnabled('node', 'article', TRUE);
    +    \Drupal::service('content_translation.manager')->setEnabled('taxonomy_term', $this->vocabulary->id(), TRUE);
    +    drupal_static_reset();
    +    \Drupal::entityManager()->clearCachedDefinitions();
    +    \Drupal::service('router.builder')->rebuild();
    +  }
    +
    +  /**
    +   * Adds term reference field for the article content type.
    +   */
    +  protected function setUpTermReferenceField() {
    +    entity_create('field_storage_config', array(
    +      'field_name' => $this->termFieldName,
    +      'entity_type' => 'node',
    +      'type' => 'taxonomy_term_reference',
    +      'cardinality' => FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED,
    +      'translatable' => FALSE,
    +      'settings' => array(
    +        'allowed_values' => array(
    +          array(
    +            'vocabulary' => $this->vocabulary->id(),
    +            'parent' => 0,
    +          ),
    +        ),
    +      ),
    +    ))->save();
    +
    +    $field = entity_create('field_config', array(
    +      'field_name' => $this->termFieldName,
    +      'bundle' => 'article',
    +      'entity_type' => 'node',
    +    ));
    +    $field->save();
    +    entity_get_form_display('node', 'article', 'default')
    +      ->setComponent($this->termFieldName, array(
    +        'type' => 'taxonomy_autocomplete',
    +      ))
    +      ->save();
    +    entity_get_display('node', 'article', 'default')
    +      ->setComponent($this->termFieldName, array(
    +        'type' => 'taxonomy_term_reference_link',
    +      ))
    +      ->save();
    +  }
    

    For stuff like that I would recommend to not write a base class but a trait, so its much easier to be reused in other places.

  4. +++ b/core/modules/taxonomy/src/Tests/TermTranslationBreadcrumbTest.php
    @@ -0,0 +1,211 @@
    +   */
    +  protected function assertBreadcrumbParts($trail) {
    +    // Compare paths with actual breadcrumb.
    +    $parts = $this->getBreadcrumbParts();
    +
    +    $pass = TRUE;
    +    // There may be more than one breadcrumb on the page. If $trail is empty
    +    // this test would go into an infinite loop, so we need to check that too.
    +    while ($trail && !empty($parts)) {
    +      foreach ($trail as $path => $title) {
    +        $url = _url($path);
    +        $part = array_shift($parts);
    +        $pass = ($pass && $part['href'] === $url && $part['text'] === String::checkPlain($title));
    +      }
    +    }
    +    // No parts must be left, or an expected "Home" will always pass.
    +    $pass = ($pass && empty($parts));
    +    $this->assertTrue($pass, format_string('Breadcrumb %parts found on @path.', array(
    +      '%parts' => implode(' » ', $trail),
    +      '@path' => $this->getUrl(),
    +    )));
    +  }
    

    So yeah ... we want to extract that MenuTestBase assertions into a trait and reuse it here and there ... just c&p crap is not the solution.

jhedstrom’s picture

Thanks for the feedback. I think traits make much more sense, as does the injection of the entity manager.

jhedstrom’s picture

jhedstrom’s picture

andypost’s picture

Looks good to go except:

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -35,8 +50,10 @@ public function build(RouteMatchInterface $route_match) {
-    while ($parents = taxonomy_term_load_parents($term->id())) {
+    $term = $this->entityManager->getTranslationFromContext($term);
+    while ($parents = $this->entityManager->getStorage('taxonomy_term')->loadParents($term->id())) {
       $term = array_shift($parents);
+      $term = $this->entityManager->getTranslationFromContext($term);

I think there's no reason to load term before while, ID of term is the same

jhedstrom’s picture

FileSize
749 bytes
24.73 KB

Good catch.

Status: Needs review » Needs work

The last submitted patch, 21: breadcrumb-term-translation-1799820-21.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: breadcrumb-term-translation-1799820-21.patch, failed testing.

Status: Needs work » Needs review
dawehner’s picture

  1. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -35,8 +50,9 @@ public function build(RouteMatchInterface $route_match) {
    +    while ($parents = $this->entityManager->getStorage('taxonomy_term')->loadParents($term->id())) {
    

    Did you thought about simply storing taxonomy storage as variable? This makes it a bit easier to debug in case you have to.

  2. +++ b/core/modules/taxonomy/src/Tests/TaxonomyTranslationTestTrait.php
    @@ -0,0 +1,111 @@
    +trait TaxonomyTranslationTestTrait {
    

    I really like that we add more and more test traits now. This will help us to get better test coverage in the future.

jhedstrom’s picture

FileSize
1.35 KB
24.9 KB

This patch adds a term storage variable as suggested in #26.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I would have prefered just a local variable but whatever.

dawehner’s picture

Issue summary: View changes
alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I like having more test traits too! We should open an issue to get them documented better.

Committed 9917b07 and pushed to 8.0.x. Thanks! And thanks for adding the beta evaluation for to the issue summary.

diff --git a/core/modules/taxonomy/src/TermBreadcrumbBuilder.php b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
index 65f41fe..7c4cc89 100644
--- a/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -34,7 +34,10 @@ class TermBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   protected $termStorage;
 
   /**
-   * Setup the entity manager.
+   * Constructs the TermBreadcrumbBuilder.
+   *
+   * @param \Drupal\Core\Entity\EntityManagerInterface $entityManager
+   *   The entity manager.
    */
   public function __construct(EntityManagerInterface $entityManager) {
     $this->entityManager = $entityManager;

Made the above fix on commit.

  • alexpott committed 9917b07 on 8.0.x
    Issue #1799820 by jhedstrom, Floydm: Breadcrumb doesn't get localized...

  • alexpott committed 9917b07 on 8.1.x
    Issue #1799820 by jhedstrom, Floydm: Breadcrumb doesn't get localized...

  • alexpott committed 9917b07 on 8.3.x
    Issue #1799820 by jhedstrom, Floydm: Breadcrumb doesn't get localized...

  • alexpott committed 9917b07 on 8.3.x
    Issue #1799820 by jhedstrom, Floydm: Breadcrumb doesn't get localized...

  • alexpott committed 9917b07 on 8.4.x
    Issue #1799820 by jhedstrom, Floydm: Breadcrumb doesn't get localized...

  • alexpott committed 9917b07 on 8.4.x
    Issue #1799820 by jhedstrom, Floydm: Breadcrumb doesn't get localized...