Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.)

@smustgrave I'm on a train and without access to Slack, so in response to your comment there:

The latest patch is adding a method to block plugins, and calling it in on all block plugins:


+ public function getOperationLinks(): array {
+ $plugin = $this->getPlugin();
+ if (method_exists($plugin, 'getOperationLinks')) {
+ return $plugin->getOperationLinks();
+ }
+ return [];
+ }


Therefore, BlockPluginInterface must add this method.

Previous patches defined a new interface for block plugins to implement optionally:

> If we add getOperationsLinks to BlockBase

which is fine, but BlockPluginInterface must still add this method because that is part of the API we are expecting block plugins to have.

There seems to be some confusion on this issue about the entity and the plugin. Both are called 'block' which doesn't help. There are two totally separate class/interface hierarchies in play here:

- the block entity, which inherits from ConfigEntityBase, implements BlockInterface. This holds the configuration the admin user enters.
- the block plugin, which inherits from BlockBase, implements BlockPluginInterface. This holds the machinery for outputting the block.

Oh wait, it's using method_exists($plugin, 'getOperationLinks').

Let's not do that though -- it's ugly. We should add the method to the block interface.

Then won't I have to update all block plugins now. They all seem to complain about the missing function if I add to BlockPluginInterface

No, just add an empty implementation to BlockBase, all blocks are expected to extend from it

Like this?

Had to add a stub to Broke.php.

All green!

1. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/Broken.php
   @@ -93,4 +93,8 @@ protected function brokenMessage() {
   +  public function getOperationLinks(): array {
   +    return [];
   

   This needs an inherit doc. I'm surprised the code checking didn't complain about it!

2. +++ b/core/modules/block/src/Entity/Block.php
   @@ -354,4 +354,15 @@ public function preSave(EntityStorageInterface $storage) {
   +    if (method_exists($plugin, 'getOperationLinks')) {
   

   We don't need to check the method exists, since it's on the block plugin interface. Every block plugin *must* implement this.

Fixed points in #285

Think the issue summary is fine but am working on CR updates

Yay! I think this is ready!

I don't think we need this CR:

> SystemMenuBlock uses ModuleHandlerInterface and AccountInterface

The BC policy says:

> Particular plugins, whether class based or yaml based, should not be considered part of the public API. References to plugin IDs and settings in default configuration can be relied upon however.

Unfortunately, the patch is not passing the checks.

First up, I apologize in advance for asking some awkward questions here. But I'm concerned about the implicit dependencies this adds between the system module and the menu UI module, as well as the views module and the views UI module.

But first some minor comments

1. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -365,6 +365,25 @@ public function getDefaultOperations(EntityInterface $entity) {
   +        $operation_extra['url']->setOption('query', ['destination' => 'admin/structure/block']);
   

   Per review above from @andypost and myself, I think this should use the existing ::ensureDestination method on this class

2. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -213,4 +213,26 @@ protected function getEntity() {
   +        // Using this weight so this option appears at the top.
   

   This comment doesn't match the code, 50 would put it at the bottom.

3. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentTest.php
   @@ -0,0 +1,78 @@
   +    $this->setUpCurrentUser(['uid' => 1], ['edit any spiffy block content']);
   ...
   +    // At this point, we are logged in as an admin, so the user does
   +    // have the "administer block" permission.
   

   I don't think we should test this with user 1. So perhaps create it with a UID > 1

   User 1 typically bypasses access checks

   It will require granting the 'administer block' permission too, but then we're getting closer to a real test

4. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -232,4 +251,26 @@ public function getCacheContexts() {
   +      $menu = Menu::load($menu_name);
   +      if ($menu->access('edit')) {
   +        $links['menu-edit'] = [
   +          'title' => $this->t('Edit menu'),
   +          'url' => Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name]),
   +          'weight' => 50,
   

   Ok, brace yourself, this is where I get annoying.

   I'm not convinced system module should know about the menu UI module.

   And perhaps that suggests we're missing an API here.

   For example, let's say module A wants to modify the operation links of module B. e.g. in this case menu_ui would want to edit the operation links of system module.

   But then when you think about it, we already have a hook for adding and altering operation links - that's hook_entity_operation and hook_entity_operation_alter

   So if menu_ui cares about an operation for a block provided by system module, it should handle it itself, not the other way around.

   So I think this change should be reversed, and menu_ui module should implement one of those hooks, probably the first one and add this edit link from there.

5. +++ b/core/modules/views/src/Plugin/Block/ViewsBlockBase.php
   @@ -233,4 +245,25 @@ public function getViewExecutable() {
   +    if ($this->moduleHandler->moduleExists('views_ui') && $view->storage->access('edit')) {
   +      $links['view-edit'] = [
   +        'title' => $this->t('Edit view'),
   +        'url' => Url::fromRoute('entity.view.edit_display_form', [
   +          'view' => $view->id(),
   +          'display_id' => $display_id,
   +        ]),
   +        'weight' => 50,
   

   And the same comment here re views ui / views module and the existing hook

So yeah, those last two points are annoying, and sorry they're coming at comment #290. But one good thing is, the test already exists, so the impact shouldn't be as great.

The added bonus of that is we don't need any API changes for those two block plugins, which are commonly extended (yes they're internal, but in reality they do get extended a lot).

Thanks again for keeping this moving @smustgrave, and once again sorry for pushing back this late

I have updated patch #289 and relocated the edit block link to the top of the list on the block layout page.

Thanks for your help updating this patch. In the future, when using a patch workflow, provide interdiffs → for your patches. That allows reviewers to evaluate your changes.

Pushed up some of the feedback from #290.

290.3 will probably need all the tests redone especially if we do 290.4

As far as using the hooks, have to do some trickery to get the name of the menu. Not sure can figure out the view name/display.

So we sure this is the approach to take?

As far as using the hooks, have to do some trickery to get the name of the menu. Not sure can figure out the view name/display.

We should have the block entity in scope.

From there we can get the base block plugin ID.

$plugin = $block->getPlugin();
$plugin_id = $plugin->getBaseId();
if ($plugin_id === 'system_menu_block') {
  $menu_id = $block->getDerivativeId();
  $menu = \Drupal\system\Entity\Menu::load($menu_id);
  $menu_name = $menu->label();
}

If the plugin ID is system_menu_block

Addressed feedback from #290

Another round of feedback 😊 Looking great in general, just relatively nitpicky things (one pattern of which is an actual bug though). Left suggestions on how to harden the test coverage.

Addressed feedback.

Left some comments.
This is looking really tidy now, much smaller diff, nice and clean

Addressed feedback.

Left another review

Addressed feedback.

Hi
if I understood correctly, this would give me option to edit custom blocks in layout builder (option in contextual/operational links in layout builder for editing blocks imported from custom block library/block_content module).
Is there any patch for 10.2 planned?
I tested patch from #207 but there is no option for editing custom blocks in layout builder.

Thanks!

Hi
if I understood correctly, this would give me option to edit custom blocks in layout builder (option in contextual/operational links in layout builder for editing blocks imported from custom block library/block_content module).
Is there any patch for 10.2 planned?
I tested patch from #207 but there is no option for editing custom blocks in layout builder.

Thanks!

This is looking good, no new APIs and some nice UX wins

Thanks for keeping at it @smustgrave!

Saving issue credit

Committed 0fc730b and pushed to 11.0.x. Thanks!
Committed f8a34d3 and pushed to 11.x. Thanks!
Committed 98887e8 and pushed to 10.4.x. Thanks!
Committed 1b83f79 and pushed to 10.3.x. Thanks!

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

Re-roll patch #290 for 10.2.x