[PP-1] Empty breadcrumb at node/add and node/add/{content_type} when frontpage view is enabled

Added a proposed resolution

Hey @larowlan, @katherined

Thanks for reporting this and creating this issue with a proposed solution. However, I have one question related to the title to be set on the view.
I have created a patch with frontpage view title as "Node" which results in Breadcrumb displaying as Follows :

Home >> Node >> Add Content.

Example Screenshot:

But, when I se the view title as "Promoted Content" as per the proposed solution, the breadcrumb results in following links :

Home >> Promoted Content >> Add Content.

Example Screenshot:

Shouldn't we keep it "Node" as it seems more relevant with the breadcrumb trail as per the page visited(node edit/create page in this case)?

Keeping the status active for now, for taking this discussion ahead and will make the changes to the patch accordingly.

We don't like showing the word node in UIs, instead we prefer content

Tagging for usability review

Thanks @larowlan

Updated the patch with title : "Content".

Queued for review in #3221110: Drupal Usability Meeting 2021-07-02 or a future meeting.

Thanks.

I followed the steps to reproduce the error and I found one more problem.
I think the problem is not only with the front page because when I visit the basic page that was created, an empty breadcrumb is created as well. Patch #6 will not fix it.
See image attached.

Not all front-end themes output the current page in the breadcrumbs. I think that is how it works in HEAD?

Not all front-end themes output the current page in the breadcrumbs. I think that is how it works in HEAD?

This is correct, by default the current page is not included when the breadcrumb is built, there are contrib solutions which change this and themes can also change this through hooks/templates.

You are right. Sorry for the noise...
I checkout my local project in one commit before #2716019: View titles in breadcrumb and metatag title don't get properly translated and the behavior is the same.
The issue makes sense as it is.
I'll try to join the UX meeting. Thank you all.

Regenerated the patch here. The patch at #6 had a missing piece of code.

Patch from #12 applies to D9.2.1 and resolves the issue.
As the config is optional, you might update it with drush after applying the patch:
drush cim --partial --source=core/modules/node/config/optional

Still waiting for UX team signoff

At today's Drupal Usability Meeting [2021-07-23] we reviewed this issue and determined that there would be a negative impact due to the proposed change. Specifically, by adding a name to the view the default frontpage would now display that name. Additionally, the proposed solution addresses the symptom (the empty break crumb) but not the cause of the problem which is the fact that the "Add content" is preceded in the path by the view.

Instead our recommendation is to open a new issue to address the path via an information architecture change. We suggest that "Add content" be placed with a predecessor of the "Content" page rather than the frontpage which would avoid the need to modify how things currently function and would address the issue of this bug.

This issue should be set to closed won't fix.

All issue participants and commiters are encouraged to feel free to attend the next Drupal Usability Meeting in one week (or later) to further discuss this proposed alternate solution.

I also attended #3225097: Drupal Usability Meeting 2021-07-23, and I would like to add a couple of points:

1. I do not like making a big change to the default home page (visible on the front end of the site) in order to fix a problem with breadcrumbs in the admin theme (back-end pages).
2. A site administrator can work around the problem by setting a title or by disabling the Frontpage view (or just the Page display of that view, if they want to keep the RSS feed).

I am adding (2) to the issue summary.

So, in the UX meeting we discussed and agreed the idea that the best long-term solution would be to move /node/add (and sub-paths) to sit under /admin/content; Thereby fixing/improving the breadcrumb, and creating a better IA (information architecture).

I did a little dig to find any suitable existing issues that might address this, here's what I found:

1. #3005137: Register the node/add path as link template "add-page": not directly related but since the link-template would be fixed, this might be a good issue to also do it in.
2. #2862859: Create a top level, extendable, "Content" admin menu route that behaves like "Structure": an idea and some discussion around making /admin/content behave like Structure, with a links for "Content", Media, Taxonomy, etc; All content-things living under the top-level "Content" item. Definitely a good idea and part of the whole topic of refactoring the admin UI; Could impact this idea though in that the structure would then look like /admin/content/content/add - feels a little weird to have "content" twice. Kind of runs into the same problem that #2377543: Add "Add" item to toolbar. has, in that while Media and Taxonomy are content, they aren't "content types" in the same way that "Article" and "Basic page" are, but site builders would see all of those in the same way, #2377543 hasn't really found an elegant solution to that yet.
3. While probably not doable in #3203618: New “content creation” menu proposal, this would probably have a positive impact that that proposal.
4. That's all I could find that was useful, failing any of those we could just repurpose this issue or open a new one. If in this case, I'd prefer to just close this issue and open a fresh one so none of the context is lost.

That was a bit of brain dump, but let's see where we go...

I think the scope of this issue is breadcrumbs

We should add a breadcrumb builder for node/add that mimics the preferred IA, so we don't need to do the more disruptive change of uris

@larowlan Yeah, I did consider that as well, but at some point it just makes sense to also have the URL consistent; That said, we should avoid a BC-break, and changing the URL is probably that.

What if we add the Breadcrumb Builder in 9.3.x, but marked it as deprecated for removal in 10.0.x, then in 10.0.x to change the URLs that way avoiding a BC-break but still allowing us to move on from what I would consider a legacy URL structure?

That said, we should avoid a BC-break, and changing the URL is probably that.

In theory we could retain the old URLs, issue a deprecation warning and then issue a 301.
Not sure its worth that much effort though.

So I think the action points here are to repurpose this issue around fixing the breadcrumbs, and then a more broader issue around resolving the IA.

There's an existing issue for block content that may interest you too. #2501691: Change content-types, comment-types, and block-types URLs

Oh and #2937066: Inconsistent breadcrumb navigation in custom block library

Re titling and doing an issue summary update

Can you confirm that the proposed resolution under Current Proposal is reasonsable @AaronMcHale @benjifisher?

Thanks @larowlan, yes I think that all makes sense and aligns with our thinking. I've added to the proposed resolution for making the breadcrumb builder deprecated, and to create a follow-up issue, targeted at 10.x for removing it and changing the node add paths to sit under admin/content.

Thanks,
-Aaron

@larowlan
I've created a breadcrumb builder and adding breadcrumbs to the node/add and node/add/{node_type} page.
I haven't included interdiff in the patch as it is a completely new approach.

I have a few doubts regarding following code:

    if ($route_match->getRouteName() === 'node.add_page') {
      $breadcrumb->addLink(Link::createFromRoute($this->t('Content'), 'view.content.page_1'));
    }

To generate the "admin/content" breadcrumb, we need dependency on views module. Because "admin/content" is created by views.
Is it the correct approach?
Because we will end up up having views module dependency on something which is inside node module.

@mohit_aghera

system.admin_content should work instead of view.content.page_1.

system.admin_content is defined in in system.routing.yml and is essentially the lowest possible fall-back if the View doesn't exist or even if Node isn't installed:

system.admin_content:
  path: '/admin/content'
  defaults:
    _controller: '\Drupal\system\Controller\SystemController::overview'
    link_id: 'system.admin_content'
    _title: 'Content'
  requirements:
    _permission: 'access administration pages'

Node and Views are essentially just overriding this but it's still valid.

ohh, yeah. That's nice.
Updating patch.
Thanks for the catch.

• Update the route in the breadcrumb and using from system module.
• All the failing test cases failing in #28 are passing on local now.

Looking great, just some cleanup suggestions.

1. +++ b/core/modules/node/src/NodeBreadcrumbBuilder.php
   @@ -0,0 +1,44 @@
   +    if ($route_match->getRouteName() === 'node.add_page' || $route_match->getRouteName() === 'node.add') {
   +      return TRUE;
   +    }
   

   could this be simplified to

   $route_name = $route_match->getRouteName();
   return in_array($route_name, ['node.add_page', 'node.add'], TRUE);
   

   It would also make it scale better in the future, as we could easily add another route without adding another ||

2. +++ b/core/modules/node/src/NodeBreadcrumbBuilder.php
   @@ -0,0 +1,44 @@
   +      $breadcrumb->addLink(Link::createFromRoute($this->t('Content'), 'system.admin_content'));
   ...
   +    elseif ($route_match->getRouteName() === 'node.add') {
   +      $breadcrumb->addLink(Link::createFromRoute($this->t('Content'), 'system.admin_content'));
   +      $breadcrumb->addLink(Link::createFromRoute($this->t('Add Content'), 'node.add_page'));
   +    }
   

   We add this both times, so we can simplify this code to

   $breadcrumb->addLink(Link::createFromRoute($this->t('Content'), 'system.admin_content'));
   if ($route_match->getRouteName() === 'node.add') {
     $breadcrumb->addLink(Link::createFromRoute($this->t('Add Content'), 'node.add_page'));
   }
   
   

please review the patch.

The bug from the issue is getting fixed.
There is testing for the fix.
The points of @larowlan have been addressed.
For me it is RTBC.

As per discussion in earlier in the issue, the Breadcrumb Builder still needs to be marked as deprecated for removal in 10.x, and we need a follow-up issue targeted at 10.x for actually changing the paths to sit under /admin/content along with removing the Breadcrumb Builder (since it's basically just a stop-gap/compatibility layer).

Hi @AaronMcHale
I've updated patch to make the service as a deprecated service.

I've also created a follow-up issue here https://www.drupal.org/project/drupal/issues/3227628
Please review its description and scope.

I think we can just mark the class @internal

And add a docblock stating that it may be removed.

With all the work that needs to be done to keep up to date with our dependencies, we may not even get to large routing changes in time for D10.

Added "@internal" and doc block.
Let's see how it goes.

+++ b/core/modules/node/node.services.yml
@@ -51,3 +51,8 @@ services:
+    deprecated: The "%service_id%" service is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. See https://www.drupal.org/project/drupal/issues/3227628

+++ b/core/modules/node/src/NodeBreadcrumbBuilder.php
@@ -0,0 +1,47 @@
+ *   Breadcrumb builder service is is deprecated in 9.3.x and it will be removed
+ *   in 10.0.0. Currently it's main purpose is to add proper breadcrumbs for
+ *   certain use cases. Once we refactor node creation URLs it will be removed.
+ *   See: https://www.drupal.org/project/drupal/issues/3220437 and
+ *   https://www.drupal.org/project/drupal/issues/3227628

+++ b/core/modules/node/tests/src/Kernel/NodeBreadcrumbTest.php
@@ -0,0 +1,31 @@
+<?php
+
+namespace Drupal\Tests\node\Kernel;
+
+use Drupal\KernelTests\KernelTestBase;
+use Drupal\node\NodeBreadcrumbBuilder;
+
+/**
+ * Tests that the service "node.breadcrumb" has been deprecated.
+ *
+ * @group Test
+ * @group legacy
+ */
+class NodeBreadcrumbTest extends KernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['node'];
+
+  /**
+   * @group legacy
+   */
+  public function testNodeBreadcrumbServiceDeprecation() {
+    $this->expectDeprecation('The "node.breadcrumb" service is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. See https://www.drupal.org/project/drupal/issues/3227628');
+    $breadcrumb = \Drupal::service('node.breadcrumb');
+    // Ensure at least one assertion.
+    $this->assertInstanceOf(NodeBreadcrumbBuilder::class, $breadcrumb);
+  }
+
+}

All these lines can be removed. We are not going to add the deprecation.

@daffie don't we need to mark it as deprecated?
I think it was mentioned in #36

Besides, this is the only reason that test cases are failing.

@daffie don't we need to mark it as deprecated?

It was discussed on Slack and the conclusion was that we do not deprecate it. And instead we mark the new class as @internal. As @internal we are allowed to remove the class in D10 without a BC break. The problem with deprecation functionality is that you cannot use it in core without the testbot failing. Which happened in this issue. @larowlan came up with marking the class as @internal as an alternative for deprecation.

@mohit_aghera: Added you to the discussion on Slack.

I know we don't have a policy for it yet, but this also feels like the perfect use case for final, let me discuss with others

FWIW I think something that is being added as a deprecated-in-advance BC shim is one of the few clear cases for final. I'm concerned about us adding it to extension points that we call internal but are still if we are honest extension points, but this isn't one of those cases IMO. I don't understand why we would not just also deprecate it though and mark the test legacy? Sorry, missed something there.

It was discussed on Slack and the conclusion was that we do not deprecate it. And instead we mark the new class as @internal. As @internal we are allowed to remove the class in D10 without a BC break.

I disagree with this though. It's our policy to provide best-effort deprecation and continuous upgrade path even for internal APIs.

So the issue here is:

- we can't deprecate something that there is no replacement for
- we won't have a replacement until at least D10

So we need to do the following

- mark it as internal
- make the class final
- add a to-do linking to the broader followup about changing the paths, and say 'deprecate in {url}'
- when the followup is done, we can deprecate it as there will be no active uses in core
- then we can remove it in the next major release
- if the path changes can't happen until D10, then that would be D11.
- if the path changes happen in D9 (with a BC shim) then that would be D10

We just have to stage this folks

The first stage is internal, with a clear signal via final and a to-do that this code is on borrowed time and will be deprecated in the future. We just can't do that yet because there's no replacement

Implemented following changes:
- marked class as internal
- made the class final
- Include a @todo with relevant information.

  * @internal
- *   Breadcrumb builder service is is deprecated in 9.3.x and it will be removed

There's a double "is" there.

@AaronMcHale
I think it was in an older comment. I've already updated it.
Probably you might be seeing in the section of the removed lines.

@mohit_aghera Ohh you're right, sorry completely missed that! Clearly it's too close to lunch time and my brain is elsewhere hehe ;)

The followup is created.
All points of @larowlan have been addressed.
Back to RTBC.

I've just posted this on #3227628: [PP-2] Remove dedicated "node.breadcrumb" breadcrumb builder but repeating here because I think it affects the solution chosen here and how temporary or not it might be.

Moving the node/add pages under admin/ is really quite a big change that needs quite a lot more work than is suggested in the issue summary, There is a toggle to display node creation pages in the frontend theme i.e. not the admin theme. This will break the moment we move the node creation paths. At the moment \Drupal\node\EventSubscriber\NodeAdminRouteSubscriber takes care of marking these non-admin paths as admin paths if the node_settings::use_admin_theme config is set to TRUE. All of this will need much thought. Also the entire use case of user generated content will be affected.

1. +++ b/core/modules/node/src/NodeBreadcrumbBuilder.php
   @@ -0,0 +1,48 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function build(RouteMatchInterface $route_match) {
   +    $breadcrumb = new Breadcrumb();
   +    $breadcrumb->addLink(Link::createFromRoute($this->t('Home'), '<front>'));
   +    $breadcrumb->addLink(Link::createFromRoute($this->t('Administration'), 'system.admin'));
   +    $breadcrumb->addLink(Link::createFromRoute($this->t('Content'), 'system.admin_content'));
   +    if ($route_match->getRouteName() === 'node.add') {
   +      $breadcrumb->addLink(Link::createFromRoute($this->t('Add Content'), 'node.add_page'));
   +    }
   +    $breadcrumb->addCacheContexts(['route']);
   +    return $breadcrumb;
   +  }
   

   I think this code needs to respect the node_settings::use_admin_theme config setting. If that is set to false we shouldn't be making the node.add route part of the admin breadcrumbs. Also I wonder what happens if node authors don't have access to system.admin_content.

2. +++ b/core/modules/node/src/NodeBreadcrumbBuilder.php
   @@ -0,0 +1,48 @@
   +    $breadcrumb->addLink(Link::createFromRoute($this->t('Content'), 'system.admin_content'));
   

   What if the view has a title - currently we'd be using that - right? Shouldn't we be using it here if available?

I think this code needs to respect the node_settings::use_admin_theme config setting. If that is set to false we shouldn't be making the node.add route part of the admin breadcrumbs. Also I wonder what happens if node authors don't have access to system.admin_content.

Yep that's a great point, the Core URL Breadcrumb Builder does perform access checking on the links and I believe it omits links that the user does not have access to, so we probably need that here as well.

@AaronMcHale I've updated following changes in the patch.
It would be great if you can have a quick review from UX perspective.
- Hide the "Content" link if user doesn't have 'access administration pages' permission
- Hide the "Content" link if user doesn't have 'node.settings:use_admin_theme' is set to FALSE
- Consider the title of the frontpage view if it is already set.

We can use the Title Resolver service to get the title and not have a hidden dependency on Views, we can also use the Access Manager Service to check route access and not hardcode any assumptions about what permissions may or may not be used.

The code we need is basically all in Drupal\system\PathBasedBreadcrumbBuilder, not a direct copy-paste but close enough.

- Added access check service.
- Introduction to title manager service is causing duplications and not working as expected. Any thoughts?

Hiding patch in favor of MR.

Tests should be added for node edit page as well. Because same issue appears on node edit page.

Updated old patch from #12 for 9.3.x because I have a project that uses that specific patch. Please don't use this patch.

Over in #3344200: PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths I'm proposing that we do not add empty titles to the breadcrumb as this is obviously wrong. That would fix the issue and make the breadcrumbs correct based on the current node.add path. I think the solution here might then be unnecessary. And then we can consider re-scoping this issue to be about moving the node.add path dependent on node_settings::use_admin_theme config setting.

@alexpott: 100%, let's at least postpone this issue on #3344200: PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths and see what happens.

So if we do the current implementation on #3344200: PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths you get a breadcrumb of Home >> Node - it creates a title of Node based on the path.

With the changes in #3344200: PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths the breadcrumb on /node/add becomes just "Home" and /node/add/foo becomes Home > Add content. IMO this is fine. The discussion about moving node/add/* under admin/content has been had elsewhere but I don't think there's a conclusion yet. As the node subsystem maintainer, I am heavily against moving it for similar reasons mentioned in #58

With that said, IMO this issue could be closed as a duplicate.