View titles in breadcrumbs are being passed through the TranslationManager service as English strings.
The problem is, view titles aren't even necessarily in English, resulting in incorrectly langcoded strings in the user interface translation set. Even worse, if a view's original language isn't English, its title will be impossible to translate to English in the breadcrumb (because it will be incorrectly langcoded as an English string).
Breadcrumbs should use the view's translated title if available, and not use the TranslationManager service at all in case views are translatable, seeing that views are part of the configuration, not the user interface.
How to reproduce:
1) Clean install Drupal 8.1
2) Enable the Content Translation, Configuration Translation and Language modules, enable one language other than English, enable the translation of views, (menu links) and the basic page content type.
3) Create a node/page based view ('Test view', path: 'test'), translate said view (e.g. 'Testansicht').
4) Create a test page ('Test page', path: 'test/page1'), translate it (e.g. 'Testpage', path: 'test/page1' [or anything matching 'test/*']).
The breadcrumb for the translated page will be something like: 'Startseite > Test view > Testpage' instead of 'Startsite >Testansicht > Testpage' as it properly should be.
Comment | File | Size | Author |
---|---|---|---|
#177 | breadcrumb-bug.png | 8.33 KB | katherined |
#162 | Screenshot 2021-05-20 at 23.55.04.png | 104.42 KB | alexpott |
#156 | 2716019-156.patch | 13.87 KB | Suresh Prabhu Parkala |
#155 | 2716019-155.patch | 13.79 KB | Skymen |
#153 | interdiff_147_153.txt | 446 bytes | anmolgoyal74 |
Comments
Comment #2
dbyers55 CreditAttribution: dbyers55 commentedTriaging
Comment #3
dbyers55 CreditAttribution: dbyers55 commentedissue still present in 8.2.x
steps to reproduce accurate
Comment #4
xjmThanks @dbyers55 for confirming the issue! Updating issue credit.
Comment #5
nickolas.wilson CreditAttribution: nickolas.wilson commentedHaving the same problem with Drupal 8.1.
It appears that it is getting the title from the original view and not checking for the translated title. I've been running into similar issues with content links doing the same and only getting the original node url and not checking for the translated one. And with D8 being so different from previous versions, I'm not even sure where to start looking for where the bug could be makes it very frustrating.
Comment #6
nickolas.wilson CreditAttribution: nickolas.wilson commentedOk I think I've found the problem.
It seems that Views Breadcrumb Titles are not using the new "Translate View" tab to look up the translated string. Instead it is still searching for it in the User interface translation section (admin/config/regional/translate).
I also came across that when creating a new view that it will be stored automatically as your site's default language (In my case default is Japanese and 2nd language is English). Which confused the hell out of me as I thought it would be done as the interface language, so was only adding translations from English to Japanese, not the other way round. Making me have the Japanese word under the English column in User interface translation section (admin/config/regional/translate) and then having the English word under the Japanese column. To solve this I ended up temporary setting English as the default language to create the view and now my view breadcrumbs are displaying correctly.
To make this less confusing, I think that it would be much better that views breadcrumbs get it's title from the new "Translate View" tab section and not the old "User interface translation" section. And secondly, adding a language dropdown when creating the view so we don't get confused to which language it is in.
Anyways, hope that helps you other guys having the same problems.
Comment #7
xjmThanks @nickolas.wilson for looking into this and documenting that workaround. It definitely is not intended for that workaround to be required!
@alexpott, @catch, @webchick, @effulgentsia, and I discussed this issue and agreed it is major per https://www.drupal.org/core/issue-priority#major-bug. It interferes with normal site visitors' use of the site (by presenting breadcrumbs in the wrong language), which makes it major even with a workaround being available.
A next step for the issue will be to create a failing test demonstrating the bug.
Comment #8
xjmComment #9
xjmActive since there is not yet a patch.
Comment #11
cllamas CreditAttribution: cllamas as a volunteer commentedunasigning myself from this issue.
Comment #12
cllamas CreditAttribution: cllamas as a volunteer and at Dennis commentedComment #13
cllamas CreditAttribution: cllamas as a volunteer and at Dennis commentedComment #14
HeyJo CreditAttribution: HeyJo commentedI'm taking a look at this on DrupalCon Dublin 2016.
Comment #15
HeyJo CreditAttribution: HeyJo commentedWell, I am stuck, but wanna share what I tried in order to track down the issue.
The Breadcrumb builder is receiving the the wrong
translatedMarkup
already:So I tried to drill down where it is coming from. I noticed that in
\Drupal\Core\StringTranslation\TranslationManager::getStringTranslation
, none of thesortedTranslators
returns anything via$translator->getStringTranslation()
.The breadcrumb's title string is being passed through these two Translators:
-
Drupal\Core\StringTranslation\Translator\CustomStrings
-
Drupal\locale\LocaleTranslation
This is where I got stuck with my limited D8 knowledge.
What I don't know is...
- ... should there be another Translator service in that list for it to work?
- ... are the strings I translated in Views UI supposed to be picked up by on of the Translators above?
I hope that helps anyone to track it down further.
Comment #16
benelori CreditAttribution: benelori commentedI had a look at this issue, and it looks like the PathBasedBreadcrumbBuilder is using TitleResolver->getTitle() to add the desired string to the breadcrumb.
I uploaded a quick patch to fix this, however I am not sure if this is the right way to go.
The main idea is that we need to load the title from the configurations, because the Route that is generated doesn't contain the correct title in the default values.
My fix loads the view and thus the correct title, but maybe we need to register a _title_callback for the Route, or perhaps the problem is solvable by going deeper and by generating correct default values for the Route object.
Comment #17
benelori CreditAttribution: benelori commentedComment #18
dawehnerMaybe one "solution" would be to be able to specify
_title_langcode
or so. On the other hand you are right, maybe we have to use a title callback and at least load the translated version of the view title.Comment #19
benelori CreditAttribution: benelori commentedIs there a scenario that would require _title_langcode or a situation where we are not taking the language from the current context?
Comment #20
_Archy_ CreditAttribution: _Archy_ as a volunteer and commentedHi all! As I saw this issue was marked 'needs work' I followed up on @benelori's solution.
Investigated more on how the routes are set for the views. Drupal\views\Plugin\views\display\PathPluginBase::getRoute is generating the dynamic route for a given view and display. So the title gets set to the default language and will be converted to a TranslatableMarkup. But the problem is that the translated title is stored as configuration translation, so the two never get connected and the title can be found in the user interface translation untranslated.
So to solve the issue in a better way I've implemented the suggested
_title_callback
. I've placed the callback method in the Router\ViewsPageController, but I am not quite sure if this is the right place for this.I guess this issue could also be solved by automatically creating the user interface translation when the configuration translation for the view is added, but that would mean duplicate data, or maybe creating some kind of bridge between the two types of translation. It might also be solvable by making the TranslatableMarkup to also use config translation in some way, but that would complicate things a lot.
Comment #21
_Archy_ CreditAttribution: _Archy_ as a volunteer and commentedComment #23
_Archy_ CreditAttribution: _Archy_ as a volunteer and commentedPut back the default
_title
that I should've not removed.Comment #24
_Archy_ CreditAttribution: _Archy_ as a volunteer and commentedComment #25
_Archy_ CreditAttribution: _Archy_ as a volunteer and commentedReordered displaying of files.
Comment #26
benelori CreditAttribution: benelori commentedHi,
Can you please write a test for this new method that you added to the controller?
Comment #27
dawehnerYou know for issues like this its always better to start with the test first and let the implementation be done later. This makes the implementation side flexible and not be tight to one, and really just one solution.
In general I'm a bit worried about the potential performance problems.
IMHO we don't have to document that. It is the same for every title callback.
Let's document that its a string parameter.
Comment #28
_Archy_ CreditAttribution: _Archy_ at PitechPlus commentedCreated a test for this issue. I couldn't find (I believe there is not) a suitable class to add the test to so I created one. As I notice there is very few tests for config translations.
Adding a patch with only the test so it can fail and the patch with the test, updated according to @dawehner.
@dawehner about the performance: node also uses title callback to get it's title. We could make my patch better if we only added the title callback if the website is multilingual. But for this we need the language manager in the PathPluginBase class which is extended and constructed by other classes, so it requires some changes:
PS: Just learned that I shouldn't name an interdiff with diff extension :D
Comment #31
_Archy_ CreditAttribution: _Archy_ at PitechPlus commentedSet file ordering and status.
Comment #32
jonathanshawRe #16
I wonder if this is related to #2835018: The content type name in the backend breadcrumb should be Human name and #2513570: Changing name (label) of content type is not reflected in breadcrumb link text where we learn that admin breadcrumbs are wrongly displaying machine names not titles.
Comment #33
tstoecklerLet's remove the title completely. It's simply incorrect and can lead to translation errors if modules check that directly (instead of using the
TitleResolver
).Seems that was put back in #23 to avoid the test failures in #20 but that just means we need to fix the tests. Seems it's only unit tests that are affected, so that's should be doable.
Comment #34
tstoecklerComment #35
tstoecklerMarked #2844506: View route titles are double translated as duplicate.
Comment #37
_Archy_ CreditAttribution: _Archy_ at PitechPlus commentedTried to rewrite the unit tests that fail with the _title taken out, but it seems real hard, as I am using Views::load which requires entity manager .. so would mean many mock objects. Maybe I am think about this wrong. Should we be testing the functionality of the ViewPageController::getTitle in the PathPluginBaseTest or just get a mock for it that returns the right title?
Comment #38
rodrigoaguileraJust a rebase of #28 for 8.3.x and the patch without the "_title" to see how many tests fail now (#20 was 7 months ago)
Comment #40
rodrigoaguileraAs a side effect this bug is polluting the database by calling TitleResolver::getTitle and passing through t() non-english strings.
I had a look at the unit tests that are failing and I feel we should not mock the Views page controller or anything since that is not what we are testing here.
I changed the unit test so the check for the callback 'Drupal\views\Routing\ViewPageController::getTitle' to be set just like the
testAlterRouteWithAlterCallback test in that same file.
There is no unit test for the page controller
http://cgit.drupalcode.org/drupal/tree/core/modules/views/tests/src/Unit...
and I don't think there should be one as the the one that this patch creates good coverage.
Comment #42
tim-dielsI had some similar issue with views display titles. The views display title is not translated in the HTML head section. I can confirm that this patch solves this issue also. Good work!
Comment #43
NiklanFaced absolutely the same problem on multilingual site. Patch from #40 fix it! Thank you guys!
Comment #44
dawehnerGiven that these might have performance implications could be maybe check somehow whether the view will have a dynamic title somehow? In case it doesn't have one, could we unset the
_title_callback
?Comment #46
5n00py CreditAttribution: 5n00py commentedComment #47
5n00py CreditAttribution: 5n00py commented\Drupal\views\Views\ViewTestBase is deprecated
Use \Drupal\Tests\views\Functional\ViewTestBase.
assertEqual also deprecated, use assertEquals.
And don't forget swap first tho arguments.
Looks like you forgot to add
use Drupal\Tests\views\Functional\ViewTestBase;
to Use sectionComment #48
dawehnerI think we should still resolve #44
Comment #49
TommyChrisComment #50
5n00py CreditAttribution: 5n00py commentedAs we know, drupal routing system always wait English string in _title.
View can be non-English in many cases:
site have only one, non-english language.
site have multiple languages enabled.
Also t() function designed to translate interface strings(static text with placeholders, etc.)
Since view title is config value, it shouldn't be translated via t().
In this logic we shouldn't use _title at all.
As #44, I think this need some investigation, profiling. I do some debug around this and I see that calls to
Drupal\views\Routing\ViewPageController::getTitle
always use cache.Comment #51
5n00py CreditAttribution: 5n00py commentedAnother problem is title overriding by contextual filters.
View should be executed to get actual title.
Comment #53
ion.macaria CreditAttribution: ion.macaria as a volunteer and commentedPatch #40 works fine!
Comment #54
babis.p CreditAttribution: babis.p as a volunteer commented#40 worked for me as well (cache rebuild was needed for the patch to take effect).
Comment #55
ordermind CreditAttribution: ordermind commented#40 works for me too, after cache rebuild.
Comment #56
RenrhafConfirming #40 works after a cache rebuild too.
Comment #57
StepanISK CreditAttribution: StepanISK commentedPatch #40 works for me
Comment #58
dawehner=
Comment #59
BarisW CreditAttribution: BarisW at LimoenGroen for KRO-NCRV commentedThanks for the patch. #40works great!
Comment #60
mpp CreditAttribution: mpp at AmeXio for District09 commentedThe patch in #40 still has one issue: Drupal\views\ViewExecutable::getTitle returns a string but strings are escaped in twig so the views title appears escaped in the breadcrumb (e.g. "News & Events").
Comment #61
mpp CreditAttribution: mpp at AmeXio for District09 commentedAdded a patch that outputs the config string as a (safe) Markup object.
Comment #62
mpp CreditAttribution: mpp at AmeXio for District09 commentedAdded a patch to filter the title to strip dangerous tags:
Comment #63
mpp CreditAttribution: mpp at AmeXio for District09 commentedComment #64
Fernly CreditAttribution: Fernly at Dropsolid for District09 commentedTested and approved.
Thanks for the patch mpp.
Comment #65
alexpottThe Xss::filter() is super important and therefore we need some test coverage to prove this is implemented. Could have easily been forgotten. Really nice to see this has been considered though @mpp++
Comment #66
ramonma1989Patch #40 works for meComment #67
DamienMcKennaFixes for the coding standards issues alexpott identified in #65.
Comment #68
GrimreaperHello,
I have tested the patch from comment 67 on a Drupal core 8.7.5, It fixes the bug.
Thanks!
Comment #69
Greg BoggsIt seems Easy Breadcrumb might be affected by this. Does anyone have any advice for me on?
https://www.drupal.org/project/easy_breadcrumb/issues/3072117
Comment #70
manuga CreditAttribution: manuga at Diputació de Barcelona commentedHi,
I tested the patch from comment #67 on a Drupal core 8.7.6, It fixes the bug.
Thanks!
Comment #71
ramonma1989Patch on #67 works for me on a Drupal core 8.7.3.
Thanks!
Comment #73
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
I have reviewed the patch #31 and it worked as designed.
I had enabled the translation-related modules and add one more language(French) than English And enable translation for page content type and created a translated view for page content type.
And created a node for page content type and given the path of page same as view.
On page view, the name of the view is being translated in the breadcrumb.
Below is an output screenshot in the English language.
Below is an output screenshot in the french language.
Thanks,
Ren
Comment #74
hudriPatch #67 worked for me using D8.7.10
In my case I was using a german view and didn't get the english title when using
{{ drupal_title() }}
from Twig tweak module (which is also using\Drupal::service('title_resolver')->getTitle()
)Comment #75
alexpottThis doesn't return FALSE as far as I can see. Also doesn't return a string. It returns a Markup object. Also let's use \Drupal\views\Render\ViewsRenderPipelineMarkup as that's View's special markup object for it's funky rendering.
Tests need a $defaultTheme property as of 8.8.x
Putting services as protected properties on a test class has prove problematic in the past. Let's not do that.
Tests that the view route title is translated.
Can be
\Drupal::service('router.builder')->rebuild()
Should use $this-assertEquals() and the expected value should go first.
Comment #76
mvonfrie CreditAttribution: mvonfrie as a volunteer and commentedPatch #67 worked for me using D8.8.1
In my case I have a translated view an am using the
[current-page:title]
token via the metatag module to render the HTML title tag.Comment #77
Krzysztof DomańskiAddressed #47 and #75.
Comment #78
idebr CreditAttribution: idebr at iO commentedTechnically this just wraps \Drupal\Component\Render\MarkupTrait::create() which returns
However, since #75.1 specifically requested this to be documented as a
MarkupInterface
I suggest we leave this up to the maintainer if this should be updated.Patch works as expected. Test coverage tests the title value, but the issue summary mentions the breadcrumb title. Perhaps the test coverage should be extended to cover this element?
Comment #79
LendudeMassively redid the test coverage because it wasn't testing what we were hoping, removing the call to
$request->setLocale($languageManager->getLanguage('ro')->getId());
would still cause the test to be green, so the language wasn't doing anything. Adding a new test-only patch because its pretty different then the original test.Addressed #78. Since the parent function can return a string (when an empty string is passed), the child function should match that.
I'm removing the 'needs profiling' tag, we have coverage for caching and if just using a title callback causes massive performance hits, then I feel that would be a problem with title callbacks in general and not just for views and should probably be addressed outside views.
Comment #80
LendudeQuick clean up
Comment #82
idebr CreditAttribution: idebr at iO commentedThe test coverage is looking much better now, thanks! Setting back to RTBC.
Comment #84
xjmLooks like the latest patch is not apply to 9.1.x -- we need a reroll here. Thanks!
Comment #85
rodrigoaguileraI am interested in this getting fixed so here is a reroll for 9.1.x. It was just changing some assertSame(FALSE, $var) to asserTrue or assertFalse.
Comment #87
rodrigoaguileraFix the test
Comment #88
jofitz CreditAttribution: jofitz at jofitz commentedRemoved "Needs reroll" tag
Comment #89
Krzysztof DomańskiThanks @rodrigoaguilera! Setting back to RTBC.
Comment #90
chegor CreditAttribution: chegor as a volunteer commentedLooks ok!
Comment #91
mdupontMade a separate patch for Drupal 8.9, since patch at #87 doesn't apply because of #3082415: Replace assert(Not)Same/Identical() on booleans with assert(Not)True/False() in PHPUnit tests which is only in D9.
This is the same patch than the one at #87, only with different context lines. Because of that, Interdiff tool did not work between the 2 patches.
Comment #92
alexpottWe need to fix the 9.x patch to not use assertLink / assertNoLink
Remaining self deprecation notices (4)
2x: AssertLegacyTrait::assertLink() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->linkExists() instead. See https://www.drupal.org/node/3129738
2x in ViewTranslationTest::testViewTitleTranslation from Drupal\Tests\views\Functional
2x: AssertLegacyTrait::assertNoLink() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->linkNotExists() instead. See https://www.drupal.org/node/3129738
2x in ViewTranslationTest::testViewTitleTranslation from Drupal\Tests\views\Functional
Comment #93
mdupontFixed patch for 9.x. Applied @alexpott suggestions.
Comment #94
mdupontNote: as per #2787051-51: Views contextual filter's title override is invisible to TitleResolver, this patch does not address the issue of overridden titles, such as with Contextual filters. Would it be worth including a fix for these as well in this patch? I have a new patch ready with these fixes.
Comment #95
mdupontThis patch is the one at #93 and additionally includes fixes for overridden Views titles, from #2787051: Views contextual filter's title override is invisible to TitleResolver.
I think it would be worth adding a few test cases for overridden titles and translations (with Contextual filters), but I'm lacking time to dive into it right now.
Comment #96
mdupontDrupal 8 version of the patch at #95
Comment #97
Ahmad Abbad CreditAttribution: Ahmad Abbad as a volunteer and commentedPatch #96 worked for me
Comment #98
mdupontComment #99
joseph.olstadSorry I need this patch for D8.8.8, here it is rerolled from 8.9.x patch 96
Not sure why this is marked as needs tests, the patch already has new tests.
*EDIT* editted twice, YES THIS PATCH WORKS FOR US! thanks so much!
#2665980: [current-page:title] doesn't integrate well with views
Comment #100
Skymen CreditAttribution: Skymen commented#95 Works for me. Thanks.
Comment #101
joseph.olstadHas tests, great fix! Using 99 on 8.8.10 but other reports of patch 95 working also for D9.1.x.
Needed this patch for a view page that has a translated menu link title, with this patch the correct breadcrumb title shows up in all of our languages.
Thanks!
Comment #102
joseph.olstadThis is an important fix because without this fix the incorrect title can get cached in the breadcrumb for all languages.
Please commit this fix, it's actually a significant improvement and very tough to debug, would be best to just have this fix immediately in the next release of core. Save so many people the time of debugging this and searching for a solution or trying to implement one. Trust me on this one. Has tests, please it's been 4 years lets just get this in, it is a win-win-win.
Comment #103
LendudeStuff got added in #95 that never got test coverage and never really got a full explanation why that was pulled into scope. If we have a separate issue to address this, why are we pulling it in here?
I would suggest we roll this back to #93 (and RTBC that?) and not scope-creep on this. If there is a valid reason to add to the scope here, we need to explain why and add coverage for it .
Comment #104
joseph.olstadComment #105
joseph.olstad@Lendude, this simplified version of the patch should do the trick, we should move forward with it and then open a new issue for the other unrelated bits that were adressed in the larger patch from 96.
For the sake of expediting progress (let's please not wait another 4 years) and also simplicity I agree with your plan to move forward and then open new issues for whatever was missed that is included in patch 96.
PostgreSQL 12.1 fails can be ignored, has nothing to do with this patch, HEAD does not pass testing on postgresql 12.1
Comment #106
larowlanLet's give @Lendude another chance to review
Comment #107
Skymen CreditAttribution: Skymen commented#104 Works well on Drupal 9.0.7. RTBC+1.
Comment #108
Lendude#104 seems to have lost all the new test coverage that was still present in #93, can we add that back?
Comment #109
joseph.olstadHere is the correct reroll of patch 93
Comment #111
joseph.olstadComment #112
Simon Georges CreditAttribution: Simon Georges at Makina Corpus commented#109 works well on my 9.0.x install.
Comment #113
Lendude#109 seems to contain the right code now, but we do need to get them green :)
Comment #114
joseph.olstadComment #115
joseph.olstadMinor change for the D9.0 and D9.1 patches (see interdiff)
public to protected modules property
D9.1 and D9.0 patch is the same.
New 8.9.x patch also
Comment #116
joseph.olstadComment #118
joseph.olstadLet's focus on 9.1.x and 9.0.x
Comment #119
joseph.olstadok @lendude patch 115 has the fix, there was a recent change to 9.1.x that requires that the $modules property be declared as protected, please look at the interdiff, it's pretty simple, I think this should be moved back to RTBC
lets get this into 9.1.x and 9.0.x soon
I accidentally queued php 8.0, please ignore that.
thanks
Comment #120
joseph.olstadOk , D8.9.x issue was related to the interdiff which should only be included for D9
I reversed the interdiff part of the 8.9.x patch
this should pass testing with 8.9.x
Comment #121
LendudeLet's make the fix patch the most recent patch on the issue. So this is just a re-up of the 9.1 patch in #115, no changes.
For TitleOverrides we have #2787051: Views contextual filter's title override is invisible to TitleResolver as a follow up.
Everything seems to be back in place, back to RTBC.
Comment #123
Krzysztof DomańskiRandom test failure. Back to RTBC.
Comment #124
joseph.olstad@Lendude, is there anything left to do here other than to commit and push the patch (and create followup issues for leftovers)?
Comment #126
LendudeUnrelated fail, back to RTBC
Comment #128
Javier_Rey CreditAttribution: Javier_Rey commentedWhen I apply this patch the described problem is solved, but I lose the breadcrumb when creating/editing content
Comment #129
catchLooks like #128 needs to be addressed - we should add test coverage if that's reproducible.
Comment #130
joseph.olstadconfirmed, looking into it now.
Comment #131
joseph.olstadcatch, iiRealXz, I confirm that the patch causes the described regression illustrated in #128
Need to not only fix the patch but also add core test coverage for this illustrated breadcrumb which should be 'Node'
expected breadcrumb in illustration from #128 is :
Home » Node » Add content
with that said, we still need to also fix what is discribed in the issue title (without causing a regression like this).
Comment #132
LendudeI tried reproducing #128 too, but couldn't on a vanilla Umami install. Tried with Seven and Claro themes. I just get the breadcrumb but without the double » shown in the screenshot.
Tried to add easy_breadcrumb cause I could see that triggering this, but no problems with that either
Seven:
Easy breadcrumb:
Am I missing a step to reproduce this?
Comment #133
joseph.olstadYou didn't miss a step, you did reproduce it
it should be Home » Node » Add Content » Create Basic Page
NOT Home » Add Content » Create Basic Page
the regression is that we're missing Node » from the breadcrumb when using the patch
Comment #134
LendudeI don't see a reference to Node with or without the patch. With or without the patch, the breadcrumbs look the same as in the screenshots I posted.
Like I said this is in a vanilla Umami install, might be different on another profile, haven't looked at that.
In the screenshot in #128 I see
» »
which does indicate something is there but not loaded correctly, I don't see that in my setup eitherComment #135
paulocsI also could not reproduce as #128 pointed.
Maybe @iiRealXz can tell us the steps do reproduce.
I'm using the standard install.
Comment #136
joseph.olstadusing the claro theme?
Comment #137
paulocsNo, I'm using seven theme.
Comment #138
paulocsMy breadcrumb looks normal. I don't think Node » is missing in my case. @lendude already attached a foto from Seven. I'm attaching one from Claro. Let's see if @iiRealXz will say something about it.
Comment #139
joseph.olstadmake sure you rebuild cache between patching and unpatching, I think this will reveal it.
Comment #140
Javier_Rey CreditAttribution: Javier_Rey commentedYes, in my portal I only have another patch applied to the core (see below link), but this has happened to me in 3 websites.
Steps to reproduce this error:
Details of my website:
Patch: Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page
Comment #141
viappidu CreditAttribution: viappidu as a volunteer commentedI second @paulocs "node" is not part of my installation either and I don't think it has ever been.
Using 9.1.x-dev + seven. NOT using @iiRealXz patch.
Comment #142
joseph.olstadWe're using menu_breadcrumb, this is probably the main difference. If I back off the patch it comes back after a cache rebuild.
So this patch behaves differently before and after patching for those using menu_breadcrumb
**EDITTED**
Comment #143
zvonimirrPatch #121 works for me with Metatag enabled
Comment #144
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedUpdated the class description.
Comment #145
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedFixed the typo.
Comment #147
adityasingh CreditAttribution: adityasingh as a volunteer and at Material for Drupal India Association commentedFixed depreciation error. Please review the patch.
Comment #148
mpp CreditAttribution: mpp at AmeXio for District09 commented#147 is good for us.
Comment #149
paulocsMoving to RTBC per comment #148.
Comment #150
Skymen CreditAttribution: Skymen commented#147 Works on Drupal 9.1.3. RTBC+1
Comment #151
Ahmad Abbad CreditAttribution: Ahmad Abbad as a volunteer and commented#147 working for me
Thank you
Comment #152
alexpottThis patch needs a post update function to trigger a route rebuild. I.e. it need to do something like:
Given this is merely triggering a rebuild I don't think an update path is necessary.
It would be good to work out what is going on with menu_breadcrumb and if it is the cause of #128 - especially as it is a well used Drupal 8 module.
Comment #153
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAdded post update function.
Comment #154
Skymen CreditAttribution: Skymen at EPAM Systems commentedThe patch does not apply to Drupal 9.1.6 and 9.1.7.
Comment #155
Skymen CreditAttribution: Skymen at EPAM Systems commentedReroll for 9.1.7+ from #153
Comment #156
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled for 9.2.x.
Comment #158
drupalfan2 CreditAttribution: drupalfan2 as a volunteer commentedI am using the metatag module.
Is this the reason why view titles (only the browser title) are not translated?
Is this the right patch to solve the problem?
When will it be commited?
Comment #159
abhaypai CreditAttribution: abhaypai as a volunteer commentedI tried using patch #156 with drupal 9.1.x and it works fine.
Comment #160
DamienMcKennaComment #161
alexpottWe still need to work out what is going wrong with menu_breadcrumb and why this affects it. See #128.
I've also updated the issue credit.
Comment #162
alexpottI've tried to reproduce #128 with menu_breadcrumb installed. Here are my efforts to reproduce this with the patch applied:
1. I create a view and added to the tools menu and I called the menu item node.
2. I created a menu item for node/add/page in the tools menu...
Here's a screenshot of my efforts:
Unless @joseph.olstad or @iiRealXz give us concrete steps on how to reproduce the issue with menu_breadcrumb I think we should commit this patch because it is obviously fixing a problem for a lot of users. I will commit this patch on 25th May unless there is more feedback as to why we should not
Comment #163
Greg BoggsPerhaps the issue in #128 is caused by invalid paths? Easy Breadcrumb has a feature to print breadcrumbs when paths are invalid, menu breadcrumb doesn't, I don't think?
If the test site has no valid path at /node, then it probably won't have a linked breadcrumb to node because that path is a 404. I don't know why this patch would interact with that behavior in menu breadcrumb because this patch adds a title callback for views which is needed so that getTitle(); can return the title of the view.
We've got some placeholder code in Easy breadcrumb to get views titles until this patch is ready, but if there are issues in #128 with Menu Breadcrumb, perhaps we should follow up in the Menu Breadcrumb module (Volunteering myself to work on it if I can reproduce the issue).
Comment #164
alexpottI've opened #3215110: Add automated test coverage on the menu_breadcrumb module.There's no automated test coverage so there's no tests we can run either way to prove it works or is broken by this patch.
Comment #166
larowlanI reviewed this patch and couldn't fault it.
The issue with menu_breadcrumbs shouldn't hold up with issue as mentioned above.
Could the difference be that folks have the frontpage view enabled vs not? As that is what serves the route at /node
Typically that is the one of the first things people delete when building a site.
Given May 25th has passed and this is a 5 year old major bug, I'm going to go ahead and commit it.
Committed 93d251d and pushed to 9.3.x. Thanks!
I'll discuss with other committers whether this is eligible for backport. The presence of the update hook makes it possibly disruptive.
Comment #168
larowlanDiscussed this with @catch who felt it was better to backport this during the RC than a minor.
So backported to 9.2.x
Thanks all
Comment #169
larowlanAdding issue credit for @catch for the release-manager review of backporting
Comment #170
chr.fritschBecause our Thunder distribution tests are failing since this was committed, I investigated a bit and I have some steps to reproduce:
So yes, the default front page is the crucial part.
Comment #171
larowlan@chr.fritsch if you stop at step 5 above, what does the Node breadcrumb link to?
Comment #172
chr.fritsch@larowlan
Home links to /
Node links to /node
Comment #173
larowlanIs there anything at /node if the view is disabled?
Comment #174
larowlanDoesn't seem to be anything in node.routing.yml
What I'm hinting at is that we fixed another bug by accident, a broken node breadcrumb without the view
Comment #175
chr.fritschIf the view is disabled, we get a 404 page. So that all makes no sense :)
Because the link is broken, I think it's fine to leave it out. So in 9.2.0, if the view is enabled, we get 'Home >'. If the view is disabled it's just 'Home'.
Comment #176
larowlanI wonder if there is no title set on the front page view, if so that's the issue - can we get a follow up for that?
Comment #177
katherinedI can reproduce this with 9.2.x-dev. Using a fresh install, I:
1. created a Basic Page with the title "Home" and path /home
2. Set the default front page to /home
3. Visited node/add/article
This produces:
Comment #178
larowlan@katherined can you edit the frontpage view and add a title and see if that resolves it?
If so, I think we need a new bug here, and the bug is 'The frontpage view assumes it will be the homepage and hence doesn't have a title'
Comment #179
katherined@larowlan Yes. That does resolve it. I created a new issue at #3220437: [PP-1] Empty breadcrumb at node/add and node/add/{content_type} when frontpage view is enabled.
Comment #180
larowlanThanks, glad we got to the bottom of that
Comment #181
abhaypai CreditAttribution: abhaypai as a volunteer commentedComment #182
larowlanPlease leave the status as fixed, the bots will come along and mark it Closed (Fixed) in due course
Comment #184
xjmComment #185
xjm