Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The new breadcrumb system is designed to go through a block.
However, the page template still has a hard-coded $breadcrumb variable. That is vestigial and hard to remove and defeats the whole purpose of making the breadcrumb a block instead.
Proposed resolution
Remove the $breadcrumb variable from the page template entirely and instead ship a default block that places the breadcrumb block in the same place.
Remaining tasks
- Add a default 'breadcrumb' region that all themes inherit unless it specifies different regions in its .info.yml.
- Actually remove the 'breadcrumb' variable from the page template (template_preprocess_page()).
- Remove the 'breadcrumb' variable from the system page.html.twig template, use the block instead (#1869476: Convert global menus (primary links, secondary links) into blocks may be a helpful reference).
Beta phase evaluation
Issue category | Task because it fully implements the vision from #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service but is not a functional bug. |
---|---|
Issue priority | Normal, it's essentially a follow-up to #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service. Could arguably be major as part of #507488: Convert page elements (local tasks, actions) into blocks. |
Unfrozen changes | Unfrozen because it changes theme system components which are not frozen: templates/default regions. |
Prioritized changes | Part of #507488: Convert page elements (local tasks, actions) into blocks which is a major |
Disruption | Small disruption for contributed themes and sites, the 'breadcrumb' variable in page.html.twig will no longer output breadcrumbs, the breadcrumb block needs to be placed somewhere instead (like the breadcrumb region). |
User interface changes
Ideally none, other than it being possible to remove the breadcrumb properly.
API changes
Remove a vestigial variable, otherwise none.
Comment | File | Size | Author |
---|---|---|---|
#55 | breadcrumb-title-create-article-bartik.png | 49.6 KB | penyaskito |
#55 | breadcrumb-title-create-article-seven.png | 37.84 KB | penyaskito |
#53 | breadcrumb_screenshot.jpg | 241.62 KB | pakmanlh |
#53 | interdiff-48-53.txt | 1.37 KB | pakmanlh |
#53 | remove_breadcrumb_from-2306407-53.patch | 20.04 KB | pakmanlh |
Comments
Comment #1
dcrocks CreditAttribution: dcrocks commentedThe breadcrumb blocks are already defined for bartik and seven but aren't assigned a region. All that needs to be done is remove one line from the page.html.twig files in core and assign a region for the blocks in the block.block.bartik/seven.breadcrumbs.yml files. Apparently that hasn't been decided yet. The same problem exists for the primary and secondary menu blocks, when that issue is finally done. And since stark may have to run without the block modules enabled it may have to stay as is.
Some regions need to be defined to contain these elements that were formerly hard coded and are now blocks. Some have been suggested in other issues but have usually been very specific to the blocks being created(ie. main_menu region). I think they should be a little more generic and be layout related(eg, top_of_content, top_of_header, etc.). I'm sure someone can think of something better.
Comment #2
xjmTagging per discussion with @Crell.
Comment #3
stephen-cox CreditAttribution: stephen-cox commentedIssue https://www.drupal.org/node/1256994 has been postponed pending the decision on this.
Comment #4
donquixote CreditAttribution: donquixote commentedJust saying, in some themes it can be useful to have the breadcrumb as a fixed part of the
page.tpl.phppage template, and not have it floating in a region. Would it be easy enough for a theme developer to re-add it to the page variables?Comment #5
mortendk CreditAttribution: mortendk commentedNope its a PITA to have some elements put inside a template file and other elements put into a config. So from a theme persective, we are totally OK with having everything inside of blocks, cause it make sense - lets not make exceptions just cause we used to do things back in D4.7
Menus & submenus are moved out of the page.html.twig file, so lets fix the same with breadcrumbs (and title, action links etc - but thats for another issue)
Comment #6
ypogue CreditAttribution: ypogue commentedI am working on this.
Comment #7
ypogue CreditAttribution: ypogue commentedCreated a patch
Comment #9
lauriiiRerolled & I guess I fixed tests
Comment #10
mortendk CreditAttribution: mortendk commentedtestet it & it works
added to screenshots of bartik, seven & classy
Comment #11
star-szrIf there's no wrapper, there's no need for an
if
.Comment #12
mortendk CreditAttribution: mortendk commentedgood catch wasnt thinking about that (was looking at some of the old crap we have in the page with if statements on menus etc)
cleaned up & added interdiff
Comment #13
lauriiidunno whats happening with the patch.. Its 0 bytes.
Comment #14
star-szrGreat, one more thing is we should document the page.breadcrumb region in Seven's page.html.twig, Bartik has these docs. But while reviewing this more closely I don't think this is quite ready to fly. It really only implements part of the changes, and only for Seven and Bartik.
I am adding an initial beta evaluation table to the issue summary and updating the issue summary to outline some proposed remaining tasks, namely:
1. Is it possible to update the system page.html.twig to use the breadcrumb block? Seems like we were able to do that for primary and secondary menu. This also means adding a breadcrumb region to the default system list of regions, which I think is in \Drupal\Core\Extension\ThemeHandler::rebuildThemeData().
2. I think we actually want to remove the 'breadcrumb' variable from the page template rather than 'deprecating' it. This would be consistent with how the primary and secondary menu block conversions were handled. That means removing the code from template_preprocess_page(), and comes back to #1. If we can get these two sorted out we should end up with a green patch again :)
Comment #15
star-szrI think this a task, not a bug.
Comment #16
star-szrComment #17
davidhernandez@Cottser, why do we need a region just for the breadcrumbs? Isn't that a bit too specific? RE point 2, yes, completely. We aren't in markup freeze, so we shouldn't need to worry about any backwards compatibility.
Also, we should probably include a change record?
Comment #18
star-szrWhether we include a region or not is bikesheddable. We could maybe stick it in the content region with a negative weight. I'm just basing that on the latest patch here.
Comment #19
pounardI agree, there is enough regions already, maybe this should just be placed into an well-known region.
Comment #20
mortendk CreditAttribution: mortendk commentedin the case of the need for bikesheed and the need for a region:
the "old breadcrumb" is in a complete seperate place, so translating that into being a part of part of page.content wont fly, its not the same place.
post patch: bartik
and in system:
The number of regions we have in a template is not a part of this patch, its only role is to fix the hardcoded elements.
If theres a need for a discussion aboout regions that should be taken in the disucssion for the themes: seven, bartik & classy.
Else post is as a followup issues.
Comment #21
star-szrYep, that makes sense. Let's add the region.
Comment #22
davidhernandezI don't really care so much how the markup in Bartik is/was. It is the way it is just because that's where things fell in line. If we move the breadcrumbs, we can fix Bartik's presentation. But, I'm in anti bikeshed mode, so add the region. It doesn't hurt anything.
Comment #23
lauriiiLets see what testbot says
Comment #25
Wim LeersThis also affects performance; we currently build the breadcrumbs on every page, even when they're not used at all! This patch will solve that.
core/modules/system/templates/page.html.twig
. I also suspect this is the main reason for the 56 fails: that template is used by Classy, and most tests use Classy; hence tests verifying breadcrumbs will fail, because it will simply never show up.Should be plural?
Comment #26
lauriiiI guess it shouldn't be plural if the name of the region is breadcrumb. I think it would be also confusing if the name of the block is breadcrumb and region would be breadcrumb. Thats why I chose to use singular. I fixed point 1.
Comment #28
Wim LeersOne more fail, not one less :(
But I think I know why. All those current tests assume that the breadcrumb trail is just there, automatically. But now, you'll have to place a block to get it there.
I remember encountering similar problems in #1869476: Convert global menus (primary links, secondary links) into blocks. You'll need to do add some
$this->drupalPlaceBlock()
calls.Comment #29
lauriiiLets see if it fixes some of the tests.. I ran some of them and they seemed to be green!
Comment #31
Wim LeersProgress! But many of them now have exceptions. You'll want to add the
block
module to the list of modules that these tests should install.Comment #32
lauriiiLets see if some of the tests get fixed now :P
Comment #34
davidhernandez$modules is currently declared with 'comment'. I think you want to add to the array not replace it?
Comment #35
lauriiiAhh I missed it because normally its been placed top of setup..
Comment #37
Wim LeersThese hunks are from a different patch.
They also contain the single remaining test failure :) Remove those hunks, and this is green!
Comment #38
lauriiiRemoved!
Comment #39
lauriiihere's interdiff also
Comment #40
Wim LeersPatch looks great. Great work, great progress :)
Still needed:
Comment #41
Wim LeersAnd the screenshots I forgot.
Comment #42
lauriiiRerolled latest patch
Comment #44
penyaskitoFixed #40.2, and the failing test.
Still pending the styling in Bartik and the Change Record. When fixing #40.1, we must take into account that the block title could be there (although I guess no-one would use it in this block, it should not look misplaced as default).
Comment #45
penyaskitoUploading screenshot with title.
Comment #46
Wim LeersNW for #40.1 and #40.3.
Comment #47
lauriiiRerolled the patch
Comment #48
lauriiiShould fix #40.1
Before:
After:
Comment #51
Wim LeersDid you upload the right screenshots? The "after" in #48 still doesn't look quite right to me.
Comment #52
pakmanlhI'll try to figure out what's happenin and fix the styles issue.
Comment #53
pakmanlhI tested the correct visualization (see the screenshot below) and fixed the PageCacheTagsIntegrationTest error.
Comment #54
Manuel Garcia CreditAttribution: Manuel Garcia commentedSteps taken:
As far as I can tell, this is RTBC, good work everyone!
Comment #55
penyaskitoThe edge case of displaying the title of the block still doesn't look aligned in bartik, but it is on seven.
Compare screenshoots:
Create article using admin theme Seven:
Create article with Bartik:
Comment #56
penyaskitoThinking again about this... As showing a title for the breadcrumbs is a nice new feature of converting breadcrumbs to a block and was not supported in D7, it shouldn't stop this patch if you don't want to, but please create a follow-up if we don't fix it here.
However, leaving this as Needs work because we still need a change record.
Thanks for working on this!
Comment #57
Jeff Burnz CreditAttribution: Jeff Burnz commentedWhat I do with the label/title is set it inline with the list and make it 1em/bold. Could be an idea for core.
Comment #58
Manuel Garcia CreditAttribution: Manuel Garcia commentedIt really makes no sense to have a title for this block, perhaps we could just set the title not display by default for now?
Comment #59
penyaskitoIt is already hidden by default. Is not *usual*, but I wouldn't say that it is a no sense ;-)
Comment #60
Manuel Garcia CreditAttribution: Manuel Garcia commentedow hehe ok -=]
Comment #61
rteijeiro CreditAttribution: rteijeiro commentedChange record created: https://www.drupal.org/node/2410773
Back to needs review and go for RTBC. I'm looking forward to see this and watch breadcrumbs variable burn in hell!
Comment #62
mortendk CreditAttribution: mortendk commentedchangenotice looks good - may i suggest that @penyaskito creates the follow up to the breadcrumb block to take on the markup discussion there.
Comment #63
rteijeiro CreditAttribution: rteijeiro commentedFollow-up created: https://www.drupal.org/node/2412951
Let's move this on!
Comment #65
catchLooks great. Removes a performance hit when you don't display breadcrumbs.
Committed/pushed to 8.0.x, thanks!
Comment #66
joachim CreditAttribution: joachim commentedThis change causes contrib tests using FieldUiTestTrait to fail.
Two things are needed I think:
- a separate change record to state that tests using FieldUiTestTrait need to be updated to enable the breadcrumb block
- documentation needs to be added to FieldUiTestTrait stating this too.
Eg, CommentNonNodeTest uses the trait, and does this:
Comment #67
joachim CreditAttribution: joachim commentedReclosing this and filing a follow-up: #2417517: removal of breadcrumb from theme causes FieldUiTestTrait to fail
Comment #69
ypogue CreditAttribution: ypogue as a volunteer commented