Move block content edit and delete routes under admin/content/block to improve IA for editors and fix breadcrumbs

Added a breadcrumb builder for block_content. Feedback is welcome. As far as I can see these breadcrumbs are totally static. We just need to make sure the route is block_content.add_page or block_content.edit, and then apply the same thing.

If they're static, there's no need for test coverage. There's nothing to test except that BreadcrumbBuilderInterface() is working, and that's already covered by Breadcrumb.

I'm not 100% certain that I'm building the breadcrumbs for admin and admin/structure correctly. I feel like I should be using ChainBreadcrumbBuilderInterface, but I can't work out how I would use that in this context.

Hm. Part of me feels sick for suggesting that we need a _custom_breadcrumb callback for a given route, like we have _custom_title and _custom_access, but I'm going to ponder it.

Also, the breadcrumb API is about to change a bit: #2314599: Use title/url instead of l() for building breadcrumb

It makes me feel better to know that you think so, too. I'm surprised that we don't have a method anywhere that will follow the ancestry of a given route to set the breadrumbs. IE even if I have to set the parent of the given route, the rest of the crumbs shouldn't have to be custom.

I briefly thought that PathBasedBreadcrumbBuilder was the solution (note: I misquoted it as ChainBreadcrumb in #1), but as I said, I can't figure out how to use that in this sort of context. I'll check out the related issue you mentioned and try to reroll appropriately. Thanks!

We do have PathBasedBreadcrumbBuilder but that pre-dated all of the menu changes, including the notion of 'parent' links in {module}.links.menu.yml, I think there is merit in a MenuParentBreadcrumbBuilder - thoughts?
It would probably be more performant as well, as the PathBasedBreadcrumbBuilder invokes the TitleResolver for every link, whereas the menu one could use the menu links.

@larolan - is the title resolver that heavy? it shouldn't be.

We got away from using menu links for breadcrumbs because it made for a coupling that made many people dissatisfied and coupled the needs of breadcrumbs into potentially unrelated menu link.

I think we should jsut keep the simple path-based system for core, and let contrib explore the other options.

Ultimately what's important is:

1) It should be simple enough that the source information for the breadcrumb trail is intuitive. IMO path and menu are both fine in this regard.
2) It should be easily overridden so that contrib can deal with more options. This is already taken care of.
3) It should be lightweight. I don't know if a hypothetical MenuParentBreadcrumbBuilder would be lighter than our current PathBasedBreadcrumbBuilder, but the difference shouldn't be enormous.

So there's not a huge gain to be made by arguing this out. I think we should use path because that's what's already in place and working for extant breadcrumbs. In which case I could use some guidance on how to use PathBasedBreadcrumbBuilder, because it's not obvious to me yet. :)

Perhaps we should decide (in a separate issue) to create a MenuParentBreadcrumbBuilder and make it available, and then we can (in yet another separate issue) compare relative performance to make sure we standardize around the lighter option... But that's all pretty far out of scope for "Block content add/edit/delete needs proper breadcrumbs".

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

   Missing new line

2. +++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
   @@ -0,0 +1,43 @@
   +    $breadcrumb[] = l($this->t('Administration'), 'admin');
   +    $breadcrumb[] = l($this->t('Structure'), 'admin/structure');
   

   should use this->l

Of course the other option is to move block adding and editing underneath the block listing pages.
Do they need to be in /block ?

fwiw I think they make more sense under the block listing pages.

updated the patch per feedback. Happy to update the link generation when that change is final. :)

We do have PathBasedBreadcrumbBuilder but that pre-dated all of the menu changes, including the notion of 'parent' links in {module}.links.menu.yml, I think there is merit in a MenuParentBreadcrumbBuilder - thoughts?
It would probably be more performant as well, as the PathBasedBreadcrumbBuilder invokes the TitleResolver for every link, whereas the menu one could use the menu links.

The title resolver is really just about getting a callable and execute it. I would argue that loading the menu items from the database for a menu based implementation would
require much more steps but yeah nothing here based upon data.

+++ b/core/modules/block_content/src/BlockContentBreadcrumbBuilder.php
@@ -0,0 +1,43 @@
+   return $route_match->getRouteName() == 'block_content.add_page'
+     || $route_match->getRouteName() == 'block_content.edit';

It always felt just wrong that the custom blocks are exposed as part of the frontend, even you basically make the visual appearance to be in the admin area (both breadcrumb and admin theme). Can't we just move it to the proper place where we want the user believe it to be?

Patch does what it advertises. Screenshot attached and marked as RTBC.

Sorry to bump back, but we're still discussing alternate approach to just move the functionality.

@bohjan what are your thoughts on moving block/add and block/{id}/edit underneath admin/structure/blocks/ where the block content library sits.

I think that is a better approach.

Given that custom blocks aren't useful unless they are added on the block page as well, there is simply no reason to let the user be moved around in the nowhere.

I am removing the Novice tag from this issue because:

Avoid issues that are:
...
contentious or lacking consensus, because new contributors might be misdirected or get stuck in the middle of a debate

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

Is this leveraging the latest thinking?

Is this still relevant? I noticed the home button isn't there. Which seems to be the default behavior for content and media so wonder if it's not an issue

This is still relevant,

Even more-so when we add #1984588: Add Block Content revision UI

Also, I think this is a bug

If #2862564: Move Custom block library to Content is committed first, it may have an impact for this issue.

Assuming that, I wonder if the simpler and more robust solution is to simply change the path to /admin/content/block/<block>, that would achieve the same result but without needing a Breadcrumb Builder.

That would be possible with a BC layer once #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name is committed.

At the Usability Group we are actively working on #3318110: [meta] Reorganize Block items in the administration menu, this issue is quite relevant so I'm going to it to a meeting to get input on how best to approach this problem. As I do think there's value in the breadcrumb and path being consistent, and unlike nodes, users aren't going directly to blocks, the block content routes are primarily accessed from the Admin UI.

Should this be postponed until the block types and custom library are moved?

Yep

I presented this issue at #3331562: Drupal Usability Meeting 2023-01-13, participants were: myself, @benjifisher, @rkoller and @simohell.

There was general agreement with the idea that the breadcrumb should lead back to the Custom Block Library, and given the work on #2862564: Move Custom block library to Content it was felt it made sense to keep this issue as a follow-up to that, and include this issue within the scope of #3318110: [meta] Reorganize Block items in the administration menu.

There was agreement that rather than doing work to just updating the breadcrumb, we should instead update the route paths to sit under the Custom Block Library, as I detailed in comment #29.

Doing that would also fix the breadcrumb, while keeping the URLs consistent, and would likely be more straightforward to implement (based on my experience of making custom breadcrumb builders they can be a bit tricky, so the simpler this is to implement the better).

Another advantage of just updating the paths is that if the Custom Block Library route changed in any way in the future, the breadcrumb would stay consistent with the Custom Block Library without requiring any future work to it.

During the Usability Meeting, we also noted that (as seen in the screenshot in comment #28) when on any of the custom block local tasks, the breadcrumb contains the ID of the block. This is not very useful, and in the case of Node and other content entities we use the name of the entity. We felt that this should be fixed, but could be done in a follow-up issue.

Added follow-up issue #3333569: Breadcrumb for local tasks of Custom Blocks should contain the name of the custom block not the entity ID.

I am changing the title to PP-2 since this issue is postponed on #2862564: Move Custom block library to Content and that issue is postponed.

Personally, I am not a big fan of this method of keeping track. I will be happy if someone tells me that I should not update the title like this.

I am also explaining under "Remaining tasks" why this issue is postponed.

I think #3333569: Breadcrumb for local tasks of Custom Blocks should contain the name of the custom block not the entity ID should be resolved here too

@larowlan I created a separate issue just to help keep the scope tight, I'm a fan of well-scoped issues. Since we've basically made the solution to this issue about updating the route paths, and that issue is more specifically about fixing what's shown in the breadcrumb.

I'm actually not entirely sure what's causing the entity ID to be displayed instead of the block title. At a glance, the entity annotation leads me to DefaultHtmlRouteProvider, which leads me to EntityController, and at that point I stopped before diving further into the rabbit hole, but it's a very standard setup so that the very least that issue is probably a good place to use to do more digging and testing.

If we find it's easy to fix in this issue at the same time, then fine by me, but at the very least the problem is now documented.

I have provided a patch in https://www.drupal.org/project/drupal/issues/3333569

Thank you @alorenc! I completely missed that there was a routing.yml file, in my very quick digging, I presumed that because it is using a route provider that it was coming from there. I also completely forgot that the Revision Route Provider does not actually extend DefaultHtmlRouteProvider, my bad. Maybe one day all core entity types will use Route Providers for everything, but that is an entirely different issue, one can dream though :D

@AaronMcHale yes, you're correct I didn't realise we still weren't using route providers for block content entities, so it will require a separate fix, thanks @alorenc

#2723615: Use entity route providers for all content entity types would also solve it

@benjifisher: Would it not still be "[PP-1]" though, since this issue is only postponed on one issue. If the "[PP-N]" in issue titles started tracking all the issues postponing the issues that the issue is postponed on, I could see that getting confusing fast. (I realise that was probably a bit hard to parse, hopefully you get what I mean)

I think we can action this now

Updated remaining tasks now that this is no longer postponed.

Actually think this could be closed. Appears to already be fixed.

for the revisions tab it seems fixed by this issue #3333569: Breadcrumb for local tasks of Custom Blocks should contain the name of the custom block not the entity ID but the edit tab of a block still only contains the home link in the breadcrumb. for the edit tab the breadcrumb shows Home while for the delete and revision tab the breadcrumb shows Home > [Name of the custom block]

FYI the solution should probably be generic as I see the same behavior for media items.

The proposed solution is less about making changes to the breadcrumb, and more about changing the path of block content entities to sit under the block content library, thereby also making the breadcrumb more useful.

That would also have to be generic I think. We don't do that for media or content.

If we changed the URL to be /block/1/edit though vs just /block/1 when editing that may fix the breakcrumbs but think the issue is we don't have a view route like we do for nodes.

I am assigning this issue to myself and working on it today. After 3 days, feel free to re-assign it if I forget to.

At first I considered updating paths like /block/add as part of this issue. I still like the idea, but it is out of scope for this issue and should be done at the same time as other entity types. Maybe someone already said as much.

I was also a little worried about using the path /admin/content/block-content/{block_content} because there is no fixed part after /admin/content/block-content. What if we do eventually change the path of the block-add form, and someone uses add for {block_content}? Then I noticed the condition block_content: \d+ and I stopped worrying.

3772 is ready to review.

Surprisingly, I updated only one line in the tests. It helps to search for /block/ that is not preceded by admin/structure and to ignore fixtures.

I am worried that we should add a redirect for /block/{block_content}/translations. This may also be a problem for #3318112: [PP1] Move "Block layout" from Structure to Appearance.

Since #1984588: Add Block Content revision UI was already fixed, I update the revision paths in the entity annotations and the update function. I think it is OK to update the update function because it is not yet part of a release. It might cause a little trouble for those of us who work with 10.1.x regularly. We might also update the screenshots in the change record for that issue. I do not see any reference to the path, but the breadcrumbs are visible in the screenshots.

I notice that the block ID, not the title, shows up in the breadcrumb for the delete form and the translations page. In order to manage scope, I do not plan to do anything about it for this issue.

Edit: I meant #2987964: Move custom block types admin link to admin/structure, not #3318112.

Edit: It turns out that I missed a few (OK, more than two dozen) lines that had to be changed in tests.

I’m not sure how this works, can’t see the code, and I’m on my phone, but just wanted to add that /admin/structure is not a guaranteed path since there are modules that can rename the admin path. I maintain a site where the previous developer used it to rename it from /admin/ to /backend/ presumably because the previous non-Drupal site used that path.

@dalemoore:

Thanks for pointing that out.

I had a quick look at that module. From the description (project page and README) and the event subscriber, it looks as though it works by altering routes. I am pretty sure that the changes here will "play well" with that module.

Example: assume that module is configured to change admin to backend

Before this issue, the entity.block_content.edit_form route has the path /block/{block_content} (for example, /block/17). The module would not change it.

After this issue, the entity.block_content.edit_form route has the path /admin/content/block-content/{block_content}. The module would change it to /backend/content/block-content/{block_content}.

Sounds great! I wasn’t worried since this (D7) site will be merged into our new D10 site but figured I’d pop in and advocate for those who do use it just in case. Thanks!

Can the issue summary be updated as well? Seems to be changing the route to something else from the proposed solution.

After hacking the tests to figure out why they were failing, I rebased and force-pushed to simplify the git log.

I am updating the issue summary. I think that I implemented what was intended, but the proposed resolution was not very explicit.

I am making some further changes to the issue summary. The first line of the "Proposed resolution" section was

The Custom Block Entity Route paths will be changed from /block/ to /admin/content/block/.

Before #2862564: Move Custom block library to Content, the "Custom block" list at /admin/content/block-custom was the "Custom block library" at /admin/structure/block/block-content. The line here was either a mistake or a guess as to where the page would end up.

I am fixing that line and changing "Custom block library" to "Custom block" list. The parts in quotation marks are the page titles.

We also have the renaming ticket where I think we are changing the page to Block Content

I added #3351750: Create BC redirects for children of changed paths. In order to manage scope, I would like to handle redirects for child pages in that issue, not this one. (If that issue is fixed first, then it should be easy enough to handle the redirects in this issue.)

I would like to get this issue done in time for 10.1.0-alpha1 because the path changes are disruptive and because this issue updates an update function. I think it is all right if the redirects for child paths are added later. Another reason to handle these redirects in a separate issue is that we already have child paths that need redirects after #2987964: Move custom block types admin link to admin/structure.

I am adding screenshots to this issue, and I will add them to the change record. I already updated the text part of the change record.

The MR already includes new tests and fixes old ones. I am removing the tag for needing tests.

Applied the MR locally.

Couldn’t get the update hook to run without changing the number to 10201.

After the update ran, cleared cached and all the breadcrumbs appear to match the screenshots in the issue summary.

The update hook does not have to work from one version of 10.1.x to the next, until we get to the alpha release of 10.1.0. In fact, we might not guarantee an upgrade path until the beta release.

That is why I have been saying that the update-update is a reason to get this issue done before the alpha release.

If you want to test the upgrade path, then

1. Check out some version of Drupal 10.0 (such as 10.0.x or 10.0.7).
2. composer install
3. Install Drupal.
4. Check out the branch for this issue from the issue fork.
5. composer install
6. Execute update functions (drush updatedb or visit update.php in a browser).

As brought up in the slack channel #block-content with you, me, and @larowlan this should be able to run on 10.1.x also.

Re comment #60:

The Custom Block Entity Route paths will be changed from /block/ to /admin/content/block/.

Before #2862564: Move Custom block library to Content, the "Custom block" list at /admin/content/block-custom was the "Custom block library" at /admin/structure/block/block-content. The line here was either a mistake or a guess as to where the page would end up.

When I originally wrote the issue summary, I wrote it with the assumption that by the time we got to this issue, the path would end up being /admin/content/block, the change from /admin/content/block-custom to /admin/content/block looks like it is being done as part of #3318549: Rename Custom Block terminology in the admin UI. So if that gets committed first, we can then update the IS again.

Looking at the MR here, I think we should postpone this issue on #3318549: Rename Custom Block terminology in the admin UI, given that issue is changing the block content overview path, I think it makes more sense to get that one committed, then update this issue with the updated path.

Now that #3318549: Rename Custom Block terminology in the admin UI is fixed, I have updated the MR, so this issue is again ready for review.

Re Comment #67: #3318549 already updated the update function, so I think it is OK to do it again in this issue, as long as we get this issue fixed before 10.1.0-alpha1.

Going to mark this as I did a fresh install with the changes already applied and the breadcrumbs are working much better then before.

One minor question regarding the duplicate route.

Tagging as a 10.1 should have. Not the end of the world, but ideally.

Adjusting title to reflect the resolution

Test appear green so assuming the duplicate just wasn't needed.

Committed to 10.1.x and updated the change record

I updated the release notes for this (but neglected to mention above)

Awesome to see this get committed!

But also because it now opens up the possibility of looking at this for other entity types (taxonomy, media, etc) where there's a strong case for making the path and breadcrumb work in the admin UI, while also not breaking the breadcrumb for other accessing those entity types on the front-end. That's a much harder problem to crack, this was definitely the low hanging fruit one!