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

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

Files: 
CommentFileSizeAuthor
#55 breadcrumb-title-create-article-bartik.png49.6 KBpenyaskito
#55 breadcrumb-title-create-article-seven.png37.84 KBpenyaskito
#53 breadcrumb_screenshot.jpg241.62 KBpakmanlh
#53 interdiff-48-53.txt1.37 KBpakmanlh
#53 remove_breadcrumb_from-2306407-53.patch20.04 KBpakmanlh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,960 pass(es). View
#48 Screen Shot 2015-01-16 at 1.03.03 AM.png53.18 KBlauriii
#48 Screen Shot 2015-01-16 at 1.03.07 AM.png57.88 KBlauriii
#48 interdiff.txt215 byteslauriii
#48 remove_breadcrumb_from-2306407-48.patch19.64 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,885 pass(es), 12 fail(s), and 0 exception(s). View
#41 head.png56.53 KBWim Leers
#10 bartik-new.png100.77 KBmortendk
#9 drupal-remove_breadcrumb-2306407-9.patch5.77 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,818 pass(es). View
#7 drupal-remove_breadcrumb-2306407-7.patch4.28 KBypogue
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,175 pass(es), 6 fail(s), and 0 exception(s). View

Comments

dcrocks’s picture

The 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.

xjm’s picture

Issue tags: +beta target, +rc deadline

Tagging per discussion with @Crell.

stephen-cox’s picture

Issue https://www.drupal.org/node/1256994 has been postponed pending the decision on this.

donquixote’s picture

Just saying, in some themes it can be useful to have the breadcrumb as a fixed part of the page.tpl.php page 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?

mortendk’s picture

Issue tags: +Twig

Nope 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)

ypogue’s picture

I am working on this.

ypogue’s picture

Status: Active » Needs review
FileSize
4.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,175 pass(es), 6 fail(s), and 0 exception(s). View

Created a patch

Status: Needs review » Needs work

The last submitted patch, 7: drupal-remove_breadcrumb-2306407-7.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,818 pass(es). View

Rerolled & I guess I fixed tests

mortendk’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
100.77 KB
104.28 KB
95.71 KB

testet it & it works
added to screenshots of bartik, seven & classy

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/bartik/templates/page.html.twig
@@ -130,7 +128,9 @@
+    {% if page.breadcrumb %}
+      {{ page.breadcrumb }}
+    {% endif %}

+++ b/core/themes/seven/templates/page.html.twig
@@ -77,7 +74,9 @@
+    {% if page.breadcrumb %}
+      {{ page.breadcrumb }}
+    {% endif %}

If there's no wrapper, there's no need for an if.

mortendk’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,809 pass(es). View
4.68 KB

good 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

lauriii’s picture

FileSize
5.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,834 pass(es). View

dunno whats happening with the patch.. Its 0 bytes.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
Parent issue: » #507488: Convert page elements (local tasks, actions) into blocks

Great, 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 :)

Cottser’s picture

Issue summary: View changes

I think this a task, not a bug.

Cottser’s picture

Category: Bug report » Task
davidhernandez’s picture

@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?

Cottser’s picture

Issue tags: +Needs change record

Whether 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.

pounard’s picture

I agree, there is enough regions already, maybe this should just be placed into an well-known region.

mortendk’s picture

Issue tags: +Bikeshed

in 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

  <div id="main-wrapper" class="clearfix"><div id="main" class="clearfix">
    {{ breadcrumb }}

    <main id="content" class="column" role="main"><section class="section">
      {% if page.highlighted %}<div id="highlighted">{{ page.highlighted }}</div>{% endif %}
...
   {{ page.content }}

and in system:

  </header>

  {{ page.primary_menu }}
  {{ page.secondary_menu }}

  {{ breadcrumb }}

  {{ messages }}

  {{ page.help }}

  <main role="main">
    <a id="main-content" tabindex="-1"></a>{# link is in html.html.twig #}

    <div class="layout-content">
........
      {{ page.content }}
    </div>{# /.layout-content #}


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.

Cottser’s picture

Yep, that makes sense. Let's add the region.

davidhernandez’s picture

Issue tags: -Bikeshed

I 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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,480 pass(es), 56 fail(s), and 0 exception(s). View

Lets see what testbot says

Status: Needs review » Needs work

The last submitted patch, 23: remove_breadcrumb_from-2306407-23.patch, failed testing.

Wim Leers’s picture

Issue tags: -beta target +Performance

This also affects performance; we currently build the breadcrumbs on every page, even when they're not used at all! This patch will solve that.

  1. This should also update 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.
  2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -460,6 +460,7 @@ public function rebuildThemeData() {
    +        'breadcrumb' => 'Breadcrumb',
    

    Should be plural?

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
8.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,503 pass(es), 57 fail(s), and 0 exception(s). View

I 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.

Status: Needs review » Needs work

The last submitted patch, 26: remove_breadcrumb_from-2306407-26.patch, failed testing.

Wim Leers’s picture

One 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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
16.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,685 pass(es), 36 fail(s), and 33 exception(s). View
6.85 KB

Lets see if it fixes some of the tests.. I ran some of them and they seemed to be green!

Status: Needs review » Needs work

The last submitted patch, 29: remove_breadcrumb_from-2306407-28.patch, failed testing.

Wim Leers’s picture

Progress! 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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
22.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/file/src/Tests/FileFieldWidgetTest.php. View
8.15 KB

Lets see if some of the tests get fixed now :P

Status: Needs review » Needs work

The last submitted patch, 32: remove_breadcrumb_from-2306407-31.patch, failed testing.

davidhernandez’s picture

$modules is currently declared with 'comment'. I think you want to add to the array not replace it?

lauriii’s picture

Status: Needs work » Needs review
FileSize
680 bytes
22.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,749 pass(es), 1 fail(s), and 0 exception(s). View

Ahh I missed it because normally its been placed top of setup..

Status: Needs review » Needs work

The last submitted patch, 35: remove_breadcrumb_from-2306407-35.patch, failed testing.

Wim Leers’s picture

+++ b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php
@@ -110,6 +113,7 @@ function testPageCacheTags() {
diff --git a/core/modules/system/src/Tests/Theme/EngineTwigTest.php b/core/modules/system/src/Tests/Theme/EngineTwigTest.php

diff --git a/core/modules/system/src/Tests/Theme/EngineTwigTest.php b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
index acfd1d1..ecd8d12 100644

index acfd1d1..ecd8d12 100644
--- a/core/modules/system/src/Tests/Theme/EngineTwigTest.php

--- a/core/modules/system/src/Tests/Theme/EngineTwigTest.php
+++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php

+++ b/core/modules/system/templates/page.html.twig
@@ -96,7 +94,7 @@
diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php

diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
index f657c4d..c309a76 100644

index f657c4d..c309a76 100644
--- a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php

--- a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
+++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php

+++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
@@ -49,4 +49,13 @@ public function linkGeneratorRender() {
diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module

diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
index b754ffa..4a415cd 100644

index b754ffa..4a415cd 100644
--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module

--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
@@ -33,6 +33,10 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml

diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml
index 695645b..d2abf91 100644

index 695645b..d2abf91 100644
--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml

--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml
+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml

These hunks are from a different patch.

They also contain the single remaining test failure :) Remove those hunks, and this is green!

lauriii’s picture

Status: Needs work » Needs review
FileSize
19.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,758 pass(es). View

Removed!

lauriii’s picture

FileSize
2.59 KB

here's interdiff also

Wim Leers’s picture

Status: Needs review » Needs work

Patch looks great. Great work, great progress :)

Still needed:

  1. CSS fixes in Bartik (see attached screenshots); it looks fine in Seven
  2. The label is 'breadcrumb' (lower case, bad) in the Seven block layout form, but 'Breadcrumb' in Bartik & Classy's block layout forms (good)
  3. CR
Wim Leers’s picture

FileSize
56.53 KB
57.19 KB

And the screenshots I forgot.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,568 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled latest patch

Status: Needs review » Needs work

The last submitted patch, 42: remove_breadcrumb_from-2306407-42.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
19.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,833 pass(es). View

Fixed #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).

penyaskito’s picture

FileSize
29.75 KB

Uploading screenshot with title.

Wim Leers’s picture

Status: Needs review » Needs work

NW for #40.1 and #40.3.

lauriii’s picture

Status: Needs work » Needs review
FileSize
19.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,889 pass(es), 10 fail(s), and 0 exception(s). View

Rerolled the patch

lauriii’s picture

FileSize
19.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,885 pass(es), 12 fail(s), and 0 exception(s). View
215 bytes
57.88 KB
53.18 KB

Should fix #40.1

Before:

After:

The last submitted patch, 47: remove_breadcrumb_from-2306407-47.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 48: remove_breadcrumb_from-2306407-48.patch, failed testing.

Wim Leers’s picture

Did you upload the right screenshots? The "after" in #48 still doesn't look quite right to me.

pakmanlh’s picture

I'll try to figure out what's happenin and fix the styles issue.

pakmanlh’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
20.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,960 pass(es). View
1.37 KB
241.62 KB

I tested the correct visualization (see the screenshot below) and fixed the PageCacheTagsIntegrationTest error.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

Steps taken:

  1. Applied the patch.
  2. Ran a clean Drupal installation.
  3. Verified that the breadcrumb block is there for Bartik, Classy and Seven.
  4. Verified that there are no visual regressions in any theme.
  5. Browsed around and the breadcrumb looks and works just like before.

As far as I can tell, this is RTBC, good work everyone!

penyaskito’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
37.84 KB
49.6 KB

The 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:

penyaskito’s picture

Thinking 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!

Jeff Burnz’s picture

What I do with the label/title is set it inline with the list and make it 1em/bold. Could be an idea for core.

Manuel Garcia’s picture

It really makes no sense to have a title for this block, perhaps we could just set the title not display by default for now?

penyaskito’s picture

It is already hidden by default. Is not *usual*, but I wouldn't say that it is a no sense ;-)

Manuel Garcia’s picture

ow hehe ok -=]

rteijeiro’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Change 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!

mortendk’s picture

Status: Needs review » Reviewed & tested by the community

changenotice looks good - may i suggest that @penyaskito creates the follow up to the breadcrumb block to take on the markup discussion there.

rteijeiro’s picture

Follow-up created: https://www.drupal.org/node/2412951

Let's move this on!

  • catch committed 8c517bf on 8.0.x
    Issue #2306407 by lauriii, penyaskito, mortendk, pakmanlh, Wim Leers,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Removes a performance hit when you don't display breadcrumbs.

Committed/pushed to 8.0.x, thanks!

joachim’s picture

Category: Task » Bug report
Status: Fixed » Active

This 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:

  protected function setUp() {
    parent::setUp();
    $this->drupalPlaceBlock('system_breadcrumb_block');
joachim’s picture

Category: Bug report » Task
Status: Active » Fixed

Status: Fixed » Closed (fixed)

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

ypogue’s picture