Inconsistent breadcrumb navigation in custom block library

The issue here is that by changing paths, we might be breaking things.

We can however, fix breadcrumbs without changing paths, by adding a new breadcrumb builder.

I think avoiding the BC break by adding a builder may be the path of least resistance.

This is looking great!

Some observations

1. +++ b/core/modules/block_content/block_content.services.yml
   @@ -4,3 +4,7 @@ services:
   +      - { name: breadcrumb_builder, priority: 100}
   

   100 is quite high - if we do need 100, can we document in a comment in the services.yml why it is so high - i.e. Needs to be 100 to run before {some other service}.

   If not, lets make it something lower.

2. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,43 @@
   +    if($route_match->getParameter('block_content_type')) {
   +      return TRUE;
   +    }
   

   This would match on /block/add/{block_content_type}, so we should probably just whitelist the routes we're dealing with. We can't know all routes that include {block_content_type} - there may be many in contrib.

3. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,43 @@
   +    if($route_match->getRouteName() != 'entity.block_content_type.edit_form')
   +    {
   +      if($blocktype = $route_match->getCurrentRouteMatch()->getParameter('block_content_type')) {
   +        $breadcrumb->addLink(Link::createFromRoute($blocktype->label(), 'entity.block_content_type.edit_form', ['block_content_type' => $blocktype->id()]));
   +      }
   +    }
   

   Our coding standards require { to be on same line as the if

4. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,43 @@
   +    $breadcrumb->addCacheableDependency(0);
   

   I don't think we need this.

   If there are caching issues, we should $breadcrumb->addCacheContexts(['url'])->addCaceableDependency($route_match->getParameter('block_content_type');

   I don't think 0 is a valid value

5. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,43 @@
   \ No newline at end of file
   

   whitespace issue here

6. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
   @@ -120,8 +120,11 @@ public function testBlockContentTypeEditing() {
   +      'admin/structure/block/block-content/types' => 'Block types',
   

   Nice!

Also, I agree on the general question of changing the paths, we have #2501691: Change content-types, comment-types, and block-types URLs for that, as its a broader discussion covering other entity types too.

Tried to fix some of the issue still setting this to NW.
fixed below points

1. #17.3
2. #17.4
3. #17.5

Regarding the point #17.1 let #15 make a call on this.

1. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,48 @@
   +class BlockContentBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   

   nit: this needs a class docblock

2. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,48 @@
   +   * @@inheritdoc}
   ...
   +   * @@inheritdoc}
   

   nit - should be {@inheritdoc}

3. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,48 @@
   +    if($route_match->getRouteName() == 'block_content.add_form') {
   ...
   +      if ($route_match->getRouteName() != 'entity.block_content_type.edit_form') {
   

   nit, these are string so we should be strict - i.e. === and !==

4. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,48 @@
   +      $breadcrumb->addLink(Link::createFromRoute($this->t('Add custom block'), 'block_content.add_page'));
   

   I am comfortable with that living inside the same trail, even though the path doesn't - but we should get sign off from the usability team. If we do it for /block/add/{type} then we would need to do it for /block/add too I think.

   Can you paste screenshots of the before/after for the main routes impacted by this patch, including block/add and block/add/{type} - that will make their review easier

Can you provide an interdiff for #22 to save needing to re-review the whole patch.

If you don't have local branches and its going to be difficult, let me know and I'll review from scratch

Code looks good to me, we just need the usability team to sign off on this, tagging for review.

Patch #22 can't be applied in 9.2.x.Needs reroll.

Checking patch core/modules/block_content/block_content.services.yml...
Checking patch core/modules/block_content/src/BlockContentBreadcrumbBuilder.php...
Checking patch core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php...
error: while searching for:
    $front_page_path = Url::fromRoute('<front>')->toString();
    $this->assertBreadcrumb('admin/structure/block/block-content/manage/basic/fields', [
      $front_page_path => 'Home',
      'admin/structure/block' => 'Block layout',
      'admin/structure/block/block-content' => 'Custom block library',
      'admin/structure/block/block-content/manage/basic' => 'Bar',
    ]);
    \Drupal::entityManager()->clearCachedFieldDefinitions();

error: patch failed: core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php:120

Rerolled patch #22.Please check it.

Fixed the CS issue

Fixing the cspell issue and fixing a failing test case

So I am not seeing this with claro?

Is this a seven issue? If so this ticket may need to be moved to https://www.drupal.org/project/seven

Fixed CS errors.

More info

You can follow https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... for how to fill in the issue summary field.

Mainly
What are the steps to reproduce? Helps the reviewer and committer.
What's the proposed solution?

Also we will need a tests-only patch to verify that the tests are covering the issue.

This will be resolved when https://www.drupal.org/project/ideas/issues/3318110 lands.

Removing the usability review tag, as the usability group is already focusing on fixing this problem as part of #3318110: [meta] Reorganize Block items in the administration menu and its child issues.

On review this issue (again), I'm closing it as outdated, because changes introduced to 10.1 in #2987964: Move custom block types admin link to admin/structure have inadvertently resolved this issue.

In 10.1, Block types have moved up to the top level of Structure, thus the breadcrumb now correctly takes you to the block types list: