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

Tagging

At the moment this will be really difficult, as there is no easy/nice way to add to the operation links in list controllers. Me and Gabor were talking about this last week. I think this will be dependent on some sort of operations API?

Yeah, that would work ok. I guess that it would be nice if we had this for configuration entities in general.

Yep, we talked about this is IRC, I think that is a good plan forward. We need to ask this on a per entity level, so we get the flexibility. So probably the getOperations() method on the controller can also ask the entity for it's links. The implementation across entities can get/add this data how they please.

Related (I think): #1956644: Add "edit" to dropbuttons on the block admin page.

Actually, marked that one as a duplicate of this.

Here's what this patch does:

Two out-of-scope things I noticed:

• The existing operation links on the block admin page haven't been capitalized yet.
• The existing links are also missing some context for accessibility and translatability.

My original suggestion in #1938062-54: Convert the recent_comments block to a view was for the block library page or the block instance editing form rather than the theme/display block instance admin listing, but we should probably consider all of the above and see which are the most helpful and non-confusing.

For custom blocks, I think we might want to make the link say "Edit block content".

Just wondering, if there is overlap between instance and object settings? E.g. menu instance settings, and menu settings?

See also:
#1956504: Add block instance delete to block contextual links
#1956496: Custom block contextual links should differentiate between editing/deleting the block instance and the custom block entity

I think that it is best not to expose the term "instance" the same as it is not a good idea to expose "plugin". How about we have "Edit block"/"Delete block" and "Edit block type"/"Delete block type". Code-wise we'll know that by "type" we actually refer to the respective plugin and UI-wise the non-"type" actions refer to the configurations.

Also, using the term "Delete" all over does not feel right. Perhaps we should use "Remove" when the actual thing is simply removed from where it was placed and reserve "Delete" for when we are actually deleting.

I think that it is best not to expose the term "instance" the same as it is not a good idea to expose "plugin". How about we have "Edit block"/"Delete block" and "Edit block type"/"Delete block type"

Agreed about "instance". The problem though is that "block type" is already the bundle (like the content type) of custom blocks. Even if it weren't, it would be "Menu block," "Views block," "Content block," etc.

Related: #889378: Create taxonomy fields from the taxonomy administration page

#9: drupal-1956134-9.patch queued for re-testing.

+1 to this integration. I half-expected this existed already.

#23: drupal-1956134-23.patch queued for re-testing.

Rerolling patch because of error.

Removed the white spaces.

Not sure about providing @inheritdoc.

Also when I manually tested it, it seems like the functionality described in #12 did not work.

I am new and don't know how to go about doing it.

Where does it need it?

If #1938062: Convert the recent_comments block to a view is block on this how come this is a feature request. Rerolled #23
@elachlan Thank you for contributing to drupal please see https://drupal.org/node/1354#inheritdoc for more info on inheritdoc.

Reroll and fixed #35.

+1
it is very nice to have that handy Edit link there!
(I had to ccache and save the blocks page to reflect it)

I applied the patch in #37 and cleared cache; saw no change. I reinstalled; saw no change. This is what I'm seeing for the recent comments View block.

I'm a little lost as to what I should do to see a change. Hint?

@jessebeach
did you try what I wrote? save the blocks admin page, no need to change anything
after I did that, the page noticed the new menu items

@afeijo, I just tried that and I saw no change. Is there some file I could put a breakpoint in to verify that the code is running correctly or not?

Yay, now I just need to apply the patch, hit F5, and the Operations buttons show the Edit menu link

I did it 3 times with git reset --hard, F5 (gone), git apply -v {patch file}, F5 (show)

Alright! I'm getting an "Edit menu" option for my custom menu and the build-in menus.

"Edit menu" should be lowercase as "edit menu" to follow the pattern currently established in the list of operations.

I'm still not seeing an option to edit a view on the the "Recent comments" view block.

Changed to lower case.

I thought we decided somewhere to use uppercase for new links?

Perhaps, but I'd rather keep consistent with the pattern that's there now, even if it's the wrong one. Being inconsistent looks worse than capitalized or uncapitalized.

Capitalised is the new standard.

I'd love to do the patch to capitalize all labels

May I? In this issue or a new one?

afeijo, just do it in this issue for the ones relevant to the menu we are working on.

Here it is, elachlan :)

Looks good! Test passed.

Needs a reroll...

curl https://drupal.org/files/drupal-1956134-54.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8006  100  8006    0     0   3600      0  0:00:02  0:00:02 --:--:--  4649
error: patch failed: core/modules/block/lib/Drupal/block/BlockListController.php:240
error: core/modules/block/lib/Drupal/block/BlockListController.php: patch does not apply

I believe the problems with BlockListController.php are caused by the changes done in #2027857: Blocks operations cannot be altered

It added this:

/**
   * {@inheritdoc}
   */
  public function getOperations(EntityInterface $entity) {
    $uri = $entity->uri();
    $operations = array();
    $operations['configure'] = array(
      'title' => t('configure'),
      'href' => $uri['path'],
      'options' => $uri['options'],
    );
    $operations['delete'] = array(
      'title' => t('delete'),
      'href' => $uri['path'] . '/delete',
      'options' => $uri['options'],
    );

    return $operations;
  }

So I think we just have to change that instead.

I don't know why the tags were removed.

Back to RTBC.

This patch has a complete lack of tests.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Entity/Menu.phpundefined
@@ -60,4 +60,16 @@ class Menu extends ConfigEntityBase implements MenuInterface {
+    if (user_access('administer menu')) {

We should not be using user_access here anymore. We probably will have to use Drupal::request() here but that's better than calling out to user_access().

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1243,4 +1243,12 @@ public function mergeDefaultDisplaysOptions() {
+  public function getOperationLinks() {
+    return $this->storage->getOperationLinks();
+  }

AFAICS there is no getOperationLinks() method on Drupal\views\Plugin\Core\Entity\View

#60: block_links-1956134-60.patch queued for re-testing.

Needs reroll as per #65.

Rerolled.

Needs a reroll

Rerolled

custom block have no 'edit/configure' link now, seeps this hunk could be added here

Filed issue with patch #2139141: BlockAccessController::checkAccess() is not executed if any hook_block_access() returns data

Tagging for reroll.

Attempting a patch re-roll.

Re-rolled patch.

First time so be nice :)

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -397,5 +397,12 @@ protected function routeProvider() {
   +  ¶
   

   remove white space.

2. +++ b/core/modules/system/lib/Drupal/system/Entity/Menu.php
   @@ -90,4 +90,16 @@ public function isLocked() {
   +    if (user_access('administer menu')) {
   

   Should be \Drupal::currentUser()->hasPermission()

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php
   @@ -42,4 +45,18 @@ public function build() {
   +    list(, $menu) = explode(':', $this->getPluginId());
   

   Maybe we should add a @todo to replace once http://drupal.org/node/1874498 lands.

Hi benjy,

As I am new to this, is it up to me to fix your changes above or the original patch creator? I don't mind either way, I was just doing my first patch re-roll.

Cheers
John.

Will do, thanks.

Patch Re-roll.

Re-rolled the patch and also created an interdiff.

There are already @todo's throughout the file mentioned below.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php  

Benjy, sorry I have just spotted what you meant about the @todo.

In the public function getOperationLinks() it needs one. I will re-roll another patch later tonight.

Will re-roll another patch to add a @todo.

Re-rolled patch again to add a @todo.

Removed "Needs reroll" tag

Ok, so I have tested this on a fresh D8 install and the "Edit Menu" link fails.

It goes to a path of "/admin/structure/menu/manage/admin/edit"

The actual path should be "/admin/structure/menu/manage/admin"

Will fix the "Edit Menu" link.

Fixed the "Edit Menu" path.

New patch and interdiff.

This seems related to #1839516: Introduce entity operation providers

#2152377: Failed to change the status to 'needs work' after test failure

rerolling

Not sure where the changes to Menu entity came in, removed them.
Added some tests.
Screenshot

98.1 fixed
98.2 agreed, started off using Url objects but then realised that list builders don't work that way.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -134,4 +145,24 @@ protected function getRequiredCacheContexts() {
+    if ($this->currentUser->hasPermission('administer menu')) {
+      $links['menu-edit'] = array(
+        'title' => $this->t('Edit menu'),
+        'route_name' => 'menu_ui.menu_edit',
+        'route_parameters' => array(
+          'menu' => $menu,
+        ),
+        'weight' => 50,

Seems like extraneous implementer effort here to manually check access here. Could we automatically determine if the user has access to that link based on the route_name and route_parameters provided?

Postponed on #2302563: Access check Url objects

What exactly does a BlockPluginInterface do? What are the operations for? If the operations a block instance provides are specific to that instance, then this approach is okay. If the operations are not specific to that instance, but to all instances of that plugin, then we need to let plugins define an operations provider.

In short: plugin instances should have a single task. It's not good if we instantiate plugins for different 'scopes'.

Blocker is in, needs reroll to remove access check

Addresses 100
Re 103: Yes specific to an instance - eg example here applies to specific instance of menu block derivative (SystemMenuBlock) - we have derivative and plugin id in scope.

Menu route name changed

Wrong patch

menu ui module doesn't always exist

+++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
@@ -26,7 +27,7 @@
-interface BlockPluginInterface extends ConfigurablePluginInterface, PluginFormInterface, PluginInspectionInterface, CacheableInterface, DerivativeInspectionInterface {
+interface BlockPluginInterface extends ConfigurablePluginInterface, PluginFormInterface, PluginInspectionInterface, CacheableInterface, DerivativeInspectionInterface, OperationsProviderInterface {

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -12,7 +12,7 @@
+interface EntityInterface extends AccessibleInterface, OperationsProviderInterface {

I can't tell if its just that this patch only does a couple conversions, or if this is all of it, but I'm hesitant to force this on each interface. Why not let the specific classes (or even just the base class) implement it, and add a simple instanceof check to the relevant calls?

Block content will add one (see postponed ref issue)
Views will use it.
We have base implementations for both so there is no work for most plugins.

+1 RTBC, thanks @dawehner

1. +++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
   @@ -12,6 +12,7 @@
   +use Drupal\Core\Entity\OperationsProviderInterface;
   

   This can be removed now.

2. +++ b/core/lib/Drupal/Core/Entity/OperationsProviderInterface.php
   @@ -0,0 +1,23 @@
   + * Contains \Drupal\Core\Entity\OperationsProviderInterface.
   ...
   + * Defines an interface for providing operations links.
   

   I think either this should be moved out of the Entity namespace, or the description should be much more specific about what it is used for.

3. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -86,6 +88,19 @@ protected function setUp() {
   +      'mail' => 'tony@magicalponies.com',
   

   Currently magicalponies.com is not registered (what an opportunity!) but I think it should be example.com or something just in case :)

@tim.plunkett

1. Good catch
2. Sounds reasonable - any suggestions
3. boo - but yeah

+++ b/core/modules/block/src/Entity/Block.php
@@ -42,7 +42,7 @@
+class Block extends ConfigEntityBase implements BlockInterface, EntityWithPluginBagsInterface, \Drupal\Core\Operations\OperationsProviderInterface {

@@ -181,7 +181,7 @@ public function getVisibility() {
+    if ($plugin instanceof \Drupal\Core\Operations\OperationsProviderInterface) {

PHPStorm--

This very much reminds me of contextual links. So:

1. Why doesn't this just use the contextual links system (of ContextualLinkManager + *.links.contextual.yml, rather than creating a similar (I'd say parallel) system?
2. Alternatively, why isn't the contextual links system removed in favor of this one, so that contextual links also uses OperationsProviderInterface?
3. The contextual links system allows other modules to define new operations. This doesn't AFAICT. That prevents us from adding e.g. a Translate operation. Isn't that a problem?

Code review

Just one stupid nitpick:

+++ b/core/modules/block/src/Entity/Block.php
@@ -41,7 +42,7 @@
+class Block extends ConfigEntityBase implements BlockInterface, EntityWithPluginBagsInterface, \Drupal\Core\Operations\OperationsProviderInterface {

No need for a fully qualified class here.

For example noone came up with an idea how to support things like creation links.

But that's not an operation on an existing thing, so … I don't see why that's needed?

I know that my question kind of leads to the "almighty operations API" discussion, but OTOH, we want to make sure we don't have two ways for doing the same thing. And this is even worse: it forces us to define the same things twice! Contextual links *are* for operations after all — I'd argue it should be renamed to Operation Links in D9.
So while I don't want to slow things down unnecessarily, I think it's essential to have a discussion here about contextual links. I don't understand how this issue could've come to comment 125 without having discussed contextual links at all? :/

Conclusion: Am I missing something, or why can't we reuse *.contextual.links.yml for these operation links?

On top of that contextual links are tight to a specific visual representation atm. which we will need to decouple in order to make this happen.

Oh! I think I know see why you misunderstand me :)

I didn't mean use contextual links at admin/structure/block to provide operations links, I meant use the data that powers contextual links at admin/structure/block to provide operations links!

I think the data in *.links.contextual.yml is directly reusable?

But the "multiple groups" thing is already the case for contextual links and works fine there, right?

Couldn't find an issue to add an "Edit view" link so I filed #2346285: Provide 'Edit view' link on "admin/structure/block".

So, what is the way forward here? Is #125 the one to review?

Not sure how #126-#130 would work but adding an "Edit view" link on top of #125 in ViewsBlockBase was easy even for me =). Would it be possible to have a similar method that would automatically add contextual links for my block? Even without adding them in *.links.contextual.yml?

OperationsProviderInterface was already added elsewhere, but no code in core uses it yet. I opened #2398601: Improve OperationsProviderInterface to improve on what we have now.

Here's a reroll.

1. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -211,4 +223,21 @@ protected function getRequiredCacheContexts() {
   +    if ($this->moduleHandler->moduleExists('menu_ui')) {
   +      $links['menu-edit'] = [
   +        'title' => $this->t('Edit menu'),
   +        'url' => Url::fromRoute('entity.menu.edit_form', ['menu' => $menu]),
   

   Would it be possible to load the menu and check if it has an edit-form link rather than checking moduleExists()? Also, shouldn't this check if user can access the url or edit the menu?

2. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -173,6 +189,14 @@ public function testSystemMenuBlockConfigDependencies() {
   +    $links = $block->getOperationLinks();
   +    $menu_link = [
   +      'title' => 'Edit menu',
   +      'url' => Url::fromRoute('entity.menu.edit_form', ['menu' => $this->menu->id()]),
   +      'weight' => 50,
   +    ];
   +    $this->assertEqual($links, ['menu-edit' => $menu_link]);
   

   How about adding another method to test getOperationLinks?

this should be a straight re-roll.

Disregard the first part of #141. It's an 80 character limit but for comments.

Oh yes, it is 80. Corrected

Regardless, we don't break lines of PHP no matter how long they are.

Moved the test to it's own method.

So, we don't need the viewsUI operation links in this patch that was removed in #117?

Wrong patch. Redo.

and the interdiff.

re-roll - I think this is rtbc now but have worked on it a few times, so disqualified

Patch looks great, except one nit.
RTBC +1

+++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
@@ -177,6 +193,27 @@ public function testSystemMenuBlockConfigDependencies() {
+  public function testOperationLinks() {
+
+    $block = entity_create('block', array(

extra line is not needed, also use Block::create() according #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls

fixes #150

Thanx :)