Breadcrumb should be term name instead of term id on term edit and delete page

Code looks good. Error applying patch due to trailing whitespaces.

Optimized patch.
It make sense to show parents on edit and delete also.

Updated patch

To replicate issue:
- Create a taxonomy term
- Edit or delete term created.
- You can watch tid on breadcrumb.

I applied patch on #18 and it's works.

Greetings from Costa Rica.

Looks like we could add a test for this.

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -48,8 +49,11 @@
+          'entity.taxonomy_term.canonical',
+          'entity.taxonomy_term.edit_form',
+          'entity.taxonomy_term.delete_form',

We already have tests for all these pages we just need some link asserts in those tests.

Here are the tests and test only patch.

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -10,6 +10,7 @@
+use Drupal\Core\Url;

left over :/

The patch works good, however I think the breadcrumb link should point to the term page, not to term edit page, as it does now. Same logic as node breadcrumbs do

Removed +use Drupal\Core\Url from #23
And changed the breadcrumb for edit and delete page to link to the term page as mentionned in #24

The patch works good, however I think the breadcrumb link should point to the term page, not to term edit page, as it does now. Same logic as node breadcrumbs do

It does point to the term page. Have you used the one in #22?

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -61,14 +65,22 @@ public function build(RouteMatchInterface $route_match) {
+      $breadcrumb[] = Link::createFromRoute($term->getName(), $route_match->getRouteName(), array('taxonomy_term' => $term->id()));

Fix is fine as is we just need to fix the test. We need more breadcrumb specific(xpath) assert.

@pec can you please provide the interdiff?

Sure, here it is.

Oh i see now. NW for tests.

Let's submit a clean patch and retest once again. Good work!

Tested against the current HEAD. Works great.

Tested. Looks ok.

Please post a test only patch to show the fails.

Test only patch does not fail

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -48,8 +48,11 @@ public function __construct(EntityManagerInterface $entityManager) {
+          'entity.taxonomy_term.canonical',
+          'entity.taxonomy_term.edit_form',
+          'entity.taxonomy_term.delete_form',
+        )) && $route_match->getParameter('taxonomy_term') instanceof TermInterface;

indent is wrong

Fixed indent issue as mentioned in #36

I think I got the test right this time.

So it fails because testbot doesn't use clean urls.

Lets get the issue summary updated and screenshots added.

Ok am going to work on this issue.....Drupalcon LA Sprint!

Before continuing coding work on this, please update the title to describe the issue and make use of the template to improve the issue summary.

#22

I'm wondering why an integration test was introduced instead of an PHPunit, when a unit test looks like it should cover this without the overhead of an integration test. If all okay, I was going to write a PHPunit test to cover this.

Quick update on the issue, it is a generic issue with Views overriding the Breadcrumb output. To replicate the fact that Views is changing the correct title to an tid, disable Views and the Bookmark title is correct.

Thank you for working on this jmOkay!! I think now we need issue summary update even more than before

I don't think this is good issue for novice anymore after the findings that has been made

Poo. I just noticed this today and thought it looked like there would be a straightforward fix.

If it's views - what do we think the next steps for making progress here would be? Is all the work so far off-topic now?
I guess the *test* is at least still valid.

Download test patch from #38 and install and test it. It did not fail. So, queue for retest.

#38 test patch failed indeed. But it is not failed in the right place. So, I am afraid, we need to do something with the test patch.

Redo the test patch.

1) Add the views module. According to #54
2) Remove the link validation. It can be added back soon the clean URL thing is fixed on the text bots. After all, this ticket is focusing the link text.
3) This patch is focusing on catch the term names error in the taxonomy term pages' breadcrumb.

Put the new test patch on a test. Will work on combining the fixing and testing patch.

Here is the patch, before we find a better fix on views module, maybe use it as a backup.

Would someone please rewrite the issue title to explain what this issue is actually about?

Makes sense?

That is much improved!

New Patch on Latest revision.

Adding tests for this issue

Removed some depricated functions

Couldn't we also fix this by having a proper title callback on (taxonomy/term/1), I guess we don't have one yet as its a view?

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -70,17 +73,23 @@ public function build(RouteMatchInterface $route_match) {
     }
-
     // This breadcrumb builder is based on a route parameter, and hence it
     // depends on the 'route' cache context.
     $breadcrumb->addCacheContexts(['route']);
-
     return $breadcrumb;
   }
-
 }

Whoever makes the next patch, please remove these unrelated changes.

Actually, in the first place, the edit and delete forms need better titles. The edit route could use this, for example:

_title_callback: '\Drupal\Core\Entity\Controller\EntityController::editTitle'

That could be a separate issue. Someone, feel free to open one if none exists.

Verified the patch in #80,breadcrumb on term edit and term delete page now display the term name instead of id.Attached snapshot for reference.

@Truptti Please embed the before and after snapshots in the issue summary.

The above patch works great, looks like RTBC to me

Let's ask committer's

Nice, seems we do not have a core committer here in this thread. I will leave a message at irc for this issue.

Discussed with all the committers and we agreed this is an RC target!

1. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
   @@ -49,8 +49,11 @@ public function __construct(EntityManagerInterface $entityManager) {
   +    return in_array($route_match->getRouteName(), array(
   +        'entity.taxonomy_term.canonical',
   +        'entity.taxonomy_term.edit_form',
   +        'entity.taxonomy_term.delete_form',
   +      )) && $route_match->getParameter('taxonomy_term') instanceof TermInterface;
   
   @@ -70,10 +73,19 @@ public function build(RouteMatchInterface $route_match) {
   +    if (in_array($route_match->getRouteName(), array(
   +          'entity.taxonomy_term.edit_form',
   +          'entity.taxonomy_term.delete_form',
   +        ))) {
   

   The code formatting here looks really odd - normally we indent by 2.

2. +++ b/core/modules/taxonomy/src/Tests/TermTest.php
   @@ -367,8 +367,19 @@ function testTermInterface() {
   +    $this->drupalGet('taxonomy/term/' . $term->id() . '/edit');
   

   Missing test coverage for the delete route.

Latest patch include:
- Fixed indentation;
- Moved the test for checking the breadcrumb on term delete page in the correct place;

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -50,10 +50,10 @@ public function __construct(EntityManagerInterface $entityManager) {
   public function applies(RouteMatchInterface $route_match) {
     return in_array($route_match->getRouteName(), array(
-        'entity.taxonomy_term.canonical',
-        'entity.taxonomy_term.edit_form',
-        'entity.taxonomy_term.delete_form',
-      )) && $route_match->getParameter('taxonomy_term') instanceof TermInterface;
+      'entity.taxonomy_term.canonical',
+      'entity.taxonomy_term.edit_form',
+      'entity.taxonomy_term.delete_form',
+    )) && $route_match->getParameter('taxonomy_term') instanceof TermInterface;
   }

I think the array elements were indented once for being in an array and a second time for being inside a function parameter.

If it were me I would break the in_array() parameters onto separate lines for readability in a case like this. However, the core coding standards seem to leave it to the the coder's discretion.

Patch works for me, looks like RTBC to me.

For some reason I'm not seeing automated tests on #93... re-uploading.

In TermBreadcrumbBuilder::build() we can move

    // @todo This overrides any other possible breadcrumb and is a pure
    //   hard-coded presumption. Make this behavior configurable per
    //   vocabulary or term.
    $parents = $this->termStorage->loadAllParents($term->id());
    // Remove current term being accessed.
    array_shift($parents);

into the else as we only need to load the parents there.

Also lets add a use for \Drupal\Component\Utility\Html so we can make the code more readable... ie. $breadcrumb->addLink(Link::createFromRoute(Html::escape(...

With the patch applied a term with the label Cheese & wine appears as Cheese & wine...

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -70,10 +73,19 @@ public function build(RouteMatchInterface $route_match) {
+      $breadcrumb->addLink(Link::createFromRoute(\Drupal\Component\Utility\Html::escape($term->getName()), 'entity.taxonomy_term.canonical', array('taxonomy_term' => $term->id())));
...
+        $breadcrumb->addLink(Link::createFromRoute(\Drupal\Component\Utility\Html::escape($term->getName()), $route_match->getRouteName(), array('taxonomy_term' => $term->id())));

Actually adding \DrupalComponent\Utility\Html::escape() is not necessary - Twig will auto escape as necessary and this will cause double escaping. Can we ensure that we test the escaping of term name in breadcrumbs too - since making this incorrect change broke nothing.

Latest patch include:
- Removed Html::escape();
- Moved @todo with $parents code into else section.

@Nitesh Pawar - please learn how to make interdiffs - it makes it easy to see what has changed between two patches, see https://drupal.org/documentation/git/interdiff.

We need a test for ensuring that term names are escaped as expected in breadcrumbs.

@alexpott not too sure but i think this is the way to test for escaping the breadcrumbs. Please let me know in case i am missing anything

@ameymudras We need the new patches in addition to the interdiffs please.

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -373,6 +373,7 @@
     $this->assertIdentical((string) $breadcrumbs[0], 'Home', 'First breadcrumb text is Home');
     $this->assertIdentical((string) $breadcrumbs[1], $term->label(), 'Second breadcrumb text is term name on term edit page.');
+    $this->assertEscaped($breadcrumbs[1]);

Some HTML must be put in a breadcrumb to be sure it is escaped later.

It may be too hardcore, but the name for the term may be:
Foo <>&"' bar

created separate Term Breadcrumbs test

Please ignore my previous patch(#106).

Could views not also keep the existing _title_callback when it overrides the existing route?

I can't answer the @dawehner's question but here is the patch which removes unneeded steps which are available in testTermInterface

+++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
@@ -64,17 +67,27 @@ public function build(RouteMatchInterface $route_match) {
+      // @todo This overrides any other possible breadcrumb and is a pure.
+      //   hard-coded presumption. Make this behavior configurable per

Minor thing: There is an added period breaking up this sentence. I would remove the word "pure" because it is unnecessary and could be confusing.

I'm with you @cilefen

Should we change the status to "Reviewed & tested by the community" ?

No, there is an unanswered question in #109.

@dawehner ca you please expand your concerns/question ?

Let's uninstall views. Once done you will see something like this:

The title we currently use is then coming from from the following route definition:

~/w/d8 (8.0.x) $ drush php
Psy Shell v0.5.2 (PHP 5.5.28 — cli) by Justin Hileman
>>> $route = $route_provider->getRouteByName('entity.taxonomy_term.canonical');
=> Symfony\Component\Routing\Route {#2644}
>>> $route->getDefaults();
=> [
     "_controller" => "Drupal\views\Routing\ViewPageController::handle"
     "_title" => ""
     "view_id" => "taxonomy_term"
     "display_id" => "page_1"
     "_view_display_show_admin_links" => true
   ]
>>>

this is the entry which exists when you uninstall views:

~/w/d8 (8.0.x) $ drush php
Psy Shell v0.5.2 (PHP 5.5.28 — cli) by Justin Hileman
>>> $route = $route_provider->getRouteByName('entity.taxonomy_term.canonical');
=> Symfony\Component\Routing\Route {#3454}
>>> $route->getDefaults();
=> [
     "_entity_view" => "taxonomy_term.full"
     "_title" => "Taxonomy term"
     "_title_callback" => "\Drupal\taxonomy\Controller\TaxonomyController::termTitle"
   ]
>>>

As you see, all we actually have to do is to bring over that title callback to views and done.

So I would suggest this ...

Can you add the interdiff pls

@marcelovani #118 is a completely new approach.

It'd be great if the tests from #112 could be combined with #118 to prove we've fixed the original reported bug.

Sure.

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -81,7 +81,7 @@ protected function setUp() {
-  function testTaxonomyTermHierarchy() {
+  function ptestTaxonomyTermHierarchy() {

Disabled tests

Enabled the tests again

I applied patch #124 on latest revision and it's works.

Committed 4b64356 and pushed to 8.0.x and 8.1.x. Thanks!