Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Beta phase evaluation
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
Comment | File | Size | Author |
---|---|---|---|
#27 | breadcrumb-term-translation-1799820-27.patch | 24.9 KB | jhedstrom |
#4 | 1799820-breadcrumb_translation.jpg | 158.43 KB | floydm |
#16 | breadcrumb-term-translation-1799820-16-TEST-ONLY-WILL-FAIL.patch | 22.59 KB | jhedstrom |
Comments
Comment #1
tim.plunkettCan you provide steps to reproduce?
Comment #2
cesareaugusto CreditAttribution: cesareaugusto commentedI 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?
Comment #3
jhedstromIs this still an issue in 8?
Comment #4
floydm CreditAttribution: floydm commentedIt 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.
Comment #5
jhedstromComment #6
jhedstromHere'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, whilehu/node/1
shows translation. I'm digging into that now.Comment #8
jhedstromThis 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).
Comment #9
jhedstromAnd here is the fix. I'm not sure if this is the best way to get the translations or not.
Comment #12
jhedstromSlight logic error in the test (wrong front page path). Attached again are just the failing test, and then the fix with the test.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedLooks proper to me.
Comment #15
dawehner... can we please inject these services?
Don't we want to use $entity_manager->getTranslationFromContext ? We need to take care about fallback candidates potentially ...
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.
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.
Comment #16
jhedstromThanks for the feedback. I think traits make much more sense, as does the injection of the entity manager.
Comment #18
jhedstromComment #19
jhedstromComment #20
andypostLooks good to go except:
I think there's no reason to load term before while, ID of term is the same
Comment #21
jhedstromGood catch.
Comment #26
dawehnerDid you thought about simply storing taxonomy storage as variable? This makes it a bit easier to debug in case you have to.
I really like that we add more and more test traits now. This will help us to get better test coverage in the future.
Comment #27
jhedstromThis patch adds a term storage variable as suggested in #26.
Comment #28
dawehnerI would have prefered just a local variable but whatever.
Comment #29
dawehnerComment #30
alexpottI 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.
Made the above fix on commit.