Problem/Motivation

As raised on #1938062-54: Convert the recent_comments block to a view it might be really useful to provide additional dropdown links
on the block overview page.

Some examples could be an edit link for a menu, and edit link for the view, the aggregator feed configuration etc.

Proposed resolution

Add a new method to all block plugin using deriver to add extra operation links.

Remaining tasks

Review.

User interface changes

API changes

Added new \Drupal\Core\Operations\OperationsProviderInterface to add extra opertaion links.

Data model changes

None

Original report by @dawehner

As raised on #1938062-54: Convert the recent_comments block to a view it might be really useful to provide additional dropdown links
on the block overview page.

Some examples could be an edit link for a menu, and edit link for the view, the aggregator feed configuration etc.

Files: 

Comments

jibran’s picture

Issue tags: +Blocks-Layouts, +Block plugins

Tagging

damiankloip’s picture

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?

dawehner’s picture

It seems to be that operations API is way to complex, not sure. You could for sure always add a new method on the Block Interface that allows you to specify some links depending on your block.

damiankloip’s picture

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

dawehner’s picture

So basically the entityInterface would get a new method which returns the links in a non-render version (something which can be utilized/tested easily).

Then the listing controller and views could ask the entity to return it's links. On top of that the block entity would ask the block plugin about it's links which then would know how to link to the menu edit link.

I vote to make it as easy as possible that we can shift towards the operations api later.

damiankloip’s picture

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.

dawehner’s picture

Status: Active » Needs review
FileSize
5.99 KB
FAILED: [[SimpleTest]]: [MySQL] 52,844 pass(es), 508 fail(s), and 249 exception(s). View

Started with some basic and really simple code adding this to the entity and block plugin interface and using it for edit menu links.

Needs review more in like "needs more discussion".

Status: Needs review » Needs work

The last submitted patch, drupal-1956134-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
661 bytes
6.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1956134-9.patch. Unable to apply patch. See the log in the details link for more information. View

For sure, views breaks this :)

xjm’s picture

xjm’s picture

Actually, marked that one as a duplicate of this.

xjm’s picture

FileSize
17.7 KB

Here's what this patch does:

edit_menu.png

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.
xjm’s picture

Issue tags: +Usability

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".

dawehner’s picture

I agree that it should be actually on both places? I guess users want to save time, if possible.

Bojhan’s picture

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

klonos’s picture

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.

xjm’s picture

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.

dawehner’s picture

I'm wondering whether we should add both test coverage for operation links in general + operations links on blocks + the available operation links on menu block?

xjm’s picture

jibran’s picture

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

Status: Needs review » Needs work
Issue tags: +Usability, +Blocks-Layouts, +Block plugins

The last submitted patch, drupal-1956134-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
6.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1956134-23.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled. Adding vdc tags just because we add vdc tags to pretty much everything ;)

dawehner’s picture

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

jessebeach’s picture

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

jibran’s picture

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

Status: Needs review » Needs work
Issue tags: +Usability, +VDC, +Blocks-Layouts, +Block plugins

The last submitted patch, drupal-1956134-23.patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 57,838 pass(es). View

Rerolling patch because of error.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -530,6 +530,13 @@ public function isTranslatable() {
+  ¶
+  /**
+   * Implements \Drupal\Core\Entity\EntityInterface::getOperationLinks().

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -323,4 +323,12 @@ public function getNGEntity();
+  public function getOperationLinks();
+  ¶

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -224,4 +224,11 @@ public function submit($form, &$form_state) {
+  ¶
+  /**
+   * Implements \Drupal\block\BlockInterface::getOperationLinks().
+   */

+++ b/core/modules/block/lib/Drupal/block/BlockListController.phpundefined
@@ -240,6 +240,7 @@ public function buildForm(array $form, array &$form_state) {
+		  $links = array();

@@ -248,6 +249,7 @@ public function buildForm(array $form, array &$form_state) {
+		  $links += $entity->getOperationLinks();

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -172,4 +172,13 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
+   * Implements \Drupal\Core\Entity\EntityInterface::getOperationLinks().

+++ b/core/modules/menu/lib/Drupal/menu/Plugin/Block/MenuBlock.phpundefined
@@ -30,5 +30,19 @@ public function build() {
+  ¶
...
+   * Implements \Drupal\block\BlockInterface::getOperationLinks().

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.phpundefined
@@ -42,4 +42,18 @@ public function build() {
+   * Implements \Drupal\block\BlockInterface::getOperationLinks().

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1140,6 +1140,13 @@ public function onChange($property_name) {
+  ¶

Let's provide an @inheritdoc and remove all this kind of whitespace/tabs ...

elachlan’s picture

FileSize
6.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1956134-28_0.patch. Unable to apply patch. See the log in the details link for more information. View

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.

dawehner’s picture

Why are you not sure about the {@inheritdoc}?

elachlan’s picture

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

Where does it need it?

jibran’s picture

Category: feature » task
Status: Needs work » Needs review
FileSize
5.19 KB
7.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1956134-33.patch. Unable to apply patch. See the log in the details link for more information. View

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.

dawehner’s picture

#33: 1956134-33.patch queued for re-testing.

dawehner’s picture

+++ b/core/modules/menu/lib/Drupal/menu/Plugin/Block/MenuBlock.phpundefined
@@ -31,4 +31,18 @@ public function build() {
+   * Implements \Drupal\block\BlockInterface::getOperationLinks().

{@inheritdoc}

Status: Needs review » Needs work

The last submitted patch, 1956134-33.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
6.96 KB
PASSED: [[SimpleTest]]: [MySQL] 56,444 pass(es). View

Reroll and fixed #35.

dawehner’s picture

That allows us to provide better UX, so +1 (though i mostly wrote the code).

afeijo’s picture

Status: Needs review » Reviewed & tested by the community

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

jessebeach’s picture

FileSize
34.53 KB

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.

Blocks___Drupal_D8_Dev.png

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

afeijo’s picture

@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

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
jessebeach’s picture

@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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
7.77 KB
PASSED: [[SimpleTest]]: [MySQL] 56,491 pass(es). View

This time it even works.

afeijo’s picture

Status: Needs review » Reviewed & tested by the community

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)

jessebeach’s picture

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

Blocks___Drupal_D8_Dev.png

"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.

Blocks___Drupal_D8_Dev.png

elachlan’s picture

FileSize
7.77 KB
PASSED: [[SimpleTest]]: [MySQL] 56,450 pass(es). View

Changed to lower case.

elachlan’s picture

Status: Reviewed & tested by the community » Needs work
dawehner’s picture

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

jessebeach’s picture

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.

Bojhan’s picture

Capitalised is the new standard.

afeijo’s picture

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

May I? In this issue or a new one?

elachlan’s picture

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

afeijo’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
PASSED: [[SimpleTest]]: [MySQL] 56,674 pass(es). View

Here it is, elachlan :)

elachlan’s picture

FileSize
8.53 KB

Looks good! Test passed.

screencap

elachlan’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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
elachlan’s picture

Issue tags: -Usability, -VDC

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.

elachlan’s picture

Issue tags: +Usability, +VDC

I don't know why the tags were removed.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block_links-1956134-60.patch. Unable to apply patch. See the log in the details link for more information. View

There we go.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

dawehner’s picture

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

The ViewUI class is a decorator around entites, so everytime you add new methods, you have to implement that. In general I would like to see some views related links on the View entity object.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -Needs tests, -VDC, -Blocks-Layouts, -Block plugins

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

Status: Needs review » Needs work
Issue tags: +Usability, +Needs tests, +VDC, +Blocks-Layouts, +Block plugins

The last submitted patch, block_links-1956134-60.patch, failed testing.

jibran’s picture

Issue tags: +Needs reroll

Needs reroll as per #65.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block_links-1956134-60_0.patch. Unable to apply patch. See the log in the details link for more information. View

just a rerole, there are no tests yet.

Status: Needs review » Needs work

The last submitted patch, block_links-1956134-60.patch, failed testing.

garphy’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es). View

Rerolled.

benjy’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll

grisendo’s picture

Status: Needs work » Needs review
FileSize
5.73 KB
PASSED: [[SimpleTest]]: [MySQL] 60,259 pass(es). View

Rerolled

andypost’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

Yeah, let's open a follow up for that.

andypost’s picture

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Tagging for reroll.

jbloomfield’s picture

Assigned: Unassigned » jbloomfield

Attempting a patch re-roll.

jbloomfield’s picture

Assigned: jbloomfield » Unassigned
Status: Needs work » Needs review
FileSize
5.77 KB
PASSED: [[SimpleTest]]: [MySQL] 59,305 pass(es). View

Re-rolled patch.

First time so be nice :)

benjy’s picture

  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.

jbloomfield’s picture

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.

dawehner’s picture

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.

There is no rule at all, who has to fix things. If you can fix those, please do it. Thank you!

jbloomfield’s picture

Assigned: Unassigned » jbloomfield

Will do, thanks.

Patch Re-roll.

jbloomfield’s picture

Assigned: jbloomfield » Unassigned
FileSize
5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es). View
615 bytes

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  
jbloomfield’s picture

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.

jbloomfield’s picture

Assigned: Unassigned » jbloomfield
Status: Needs review » Active

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

jbloomfield’s picture

Assigned: jbloomfield » Unassigned
Status: Active » Needs review
FileSize
5.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,328 pass(es). View
927 bytes

Re-rolled patch again to add a @todo.

jbloomfield’s picture

Issue tags: -Needs reroll

Removed "Needs reroll" tag

jbloomfield’s picture

Assigned: Unassigned » jbloomfield
Status: Needs review » Needs work

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.

jbloomfield’s picture

Assigned: jbloomfield » Unassigned
Status: Needs work » Needs review
FileSize
5.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block_links-1956134-88.patch. Unable to apply patch. See the log in the details link for more information. View
1.28 KB

Fixed the "Edit Menu" path.

New patch and interdiff.

tim.plunkett’s picture

dawehner’s picture

88: block_links-1956134-88.patch queued for re-testing.

The last submitted patch, 88: block_links-1956134-88.patch, failed testing.

klonos’s picture

Status: Needs review » Needs work
dawehner’s picture

Status: Needs work » Needs review
FileSize
5.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch block_links-1956134.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 93: block_links-1956134.patch, failed testing.

larowlan’s picture

Assigned: Unassigned » larowlan

rerolling

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
24.29 KB
7.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,845 pass(es). View

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

dawehner’s picture

Thank you so much for the reroll!

  1. +++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
    @@ -174,4 +174,12 @@ public function getVisibilityCondition($instance_id);
    +  /**
    +   * Returns a list of operation links available for this block.
    +   *
    +   * @return array
    +   *   Array of operation links.
    +   */
    +  public function getOperationLinks();
    

    I wonder whether we should extract the operation links interface from blocks and entities into a generic one?

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

    On the longrun we could think of using a link object but this would be inconsistent with the other operations for now.

larowlan’s picture

FileSize
2.94 KB
8.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,868 pass(es). View

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

Dave Reid’s picture

+++ 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?

dawehner’s picture

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?

We can indeed and we should, see #2302563: Access check Url objects

larowlan’s picture

Status: Needs review » Postponed
Related issues: +#2302563: Access check Url objects
Xano’s picture

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'.

larowlan’s picture

Status: Postponed » Needs work

Blocker is in, needs reroll to remove access check

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,908 pass(es), 36 fail(s), and 2 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 105: block-links-1956134.105.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
605 bytes
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,895 pass(es), 34 fail(s), and 2 exception(s). View

Menu route name changed

Status: Needs review » Needs work

The last submitted patch, 107: block-links-1956134.107.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
73.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
811 bytes
larowlan’s picture

FileSize
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,894 pass(es), 33 fail(s), and 2 exception(s). View

Wrong patch

The last submitted patch, 109: block-links-1956134.109.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 110: block-links-1956134.110.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
9.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,917 pass(es). View

menu ui module doesn't always exist

dawehner’s picture

I am fine with the current status, anyone up for a review?

tim.plunkett’s picture

+++ 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?

larowlan’s picture

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.

dawehner’s picture

FileSize
8.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,184 pass(es). View

@tim.plunkett
Great idea!

larowlan’s picture

+1 RTBC, thanks @dawehner

tim.plunkett’s picture

  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 :)

larowlan’s picture

@tim.plunkett

  1. Good catch
  2. Sounds reasonable - any suggestions
  3. boo - but yeah
dawehner’s picture

FileSize
7.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,187 pass(es). View
3.66 KB

Thank you for the review!

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

Meh.

tim.plunkett’s picture

+++ 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--

dawehner’s picture

FileSize
2.79 KB
7.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,187 pass(es). View

meh!!!! SO yeah, why does storm not solve all the problems!

Wim Leers’s picture

Status: Needs review » Needs work

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch block_links-1956134-125.patch. Unable to apply patch. See the log in the details link for more information. View
566 bytes

@Wim Leers
Thank you for your review.

So you back to the almighty operations API which will just be impossible to solve. Note: This patch just exposes entity operations onto plugins.
Everyone would love to have one, which would allow you to configure which contextual links / entity operations / sibling local tasks you want to see, but yeah I don't think this is feasible at least in 8.0.x
For example noone came up with an idea how to support things like creation links.

No need for a fully qualified class here.

storm--

Wim Leers’s picture

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?

dawehner’s picture

@Wim Leers
Maybe I don't 100% get your point.

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

Well, mostly because these links are used as part of the entity listing as well, admin/structure/block is the one you will look at. Do you really want to rewrite all of the operations
in core to support contextual links as part of this issue? 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.

Wim Leers’s picture

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?

dawehner’s picture

Oh I see, well on an abstract level you certainly could. On a congrete level this would require a lot of work, as for example you would have to provide multiple groups: one for the entity type, and one for the entity type together with the specific block plugin.

Wim Leers’s picture

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

olli’s picture

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

olli’s picture

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?

Xano’s picture

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.

dawehner’s picture

@olli
Yes, this is what we need at that given point in time. At the same time I would bet that we need a reroll as well.

Status: Needs review » Needs work

The last submitted patch, 125: block_links-1956134-125.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

.

olli’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,897 pass(es). View
2.06 KB

Here's a reroll.

olli’s picture

  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?

mgifford’s picture

FileSize
7.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,194 pass(es). View

this should be a straight re-roll.

RavindraSingh’s picture

  • +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
    @@ -54,11 +62,14 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MenuLinkTreeInterface $menu_tree, MenuActiveTrailInterface $menu_active_trail, ModuleHandlerInterface $module_handler) {
    

    Line needs to break after if chars count > 80

  • +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
    @@ -211,4 +223,21 @@ protected function getRequiredCacheContexts() {
    +
    

    Remove White Space

  • tim.plunkett’s picture

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

    RavindraSingh’s picture

    Oh yes, it is 80. Corrected

    tim.plunkett’s picture

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

    lokapujya’s picture

    FileSize
    23.65 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,842 pass(es). View
    3.35 KB

    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?

    lokapujya’s picture

    FileSize
    7.54 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,893 pass(es). View
    7.54 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,850 pass(es). View

    Wrong patch. Redo.

    lokapujya’s picture

    FileSize
    1.66 KB

    and the interdiff.

    dawehner’s picture

    Great work!. It is good to see, that this issue got momentum again.

    I would love to see edit links to views but we can for sure do that later as well.

    larowlan’s picture

    FileSize
    7.48 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,774 pass(es). View

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

    andypost’s picture

    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

    larowlan’s picture

    FileSize
    860 bytes
    7.5 KB
    FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch helpful-block-links.151.patch. Unable to apply patch. See the log in the details link for more information. View

    fixes #150

    andypost’s picture

    Status: Needs review » Reviewed & tested by the community

    Thanx :)

    Wim Leers’s picture

    Status: Reviewed & tested by the community » Needs review
    +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
    @@ -205,4 +217,21 @@ protected function getRequiredCacheContexts() {
    +  public function getOperationLinks() {
    +    $menu = $this->getDerivativeId();
    +
    +    $links = [];
    +    if ($this->moduleHandler->moduleExists('menu_ui')) {
    +      $links['menu-edit'] = [
    +        'title' => $this->t('Edit menu'),
    +        'url' => Url::fromRoute('entity.menu.edit_form', ['menu' => $menu]),
    +        'weight' => 50,
    +      ];
    +    }
    +    return $links;
    +  }
    

    Seeing this made me wonder: "how is this different than contextual links?".

    … turns out I wondered the same 6 months ago. See #124#132.

    Sorry, but I really think we should formulate a proper answer to that. I imagine a committer would ask that question too.

    mgifford’s picture

    Who is best to formulate a proper answer to "how is this different than contextual links?"

    Just nudging this issue as it's been sitting here for 4 months. Can someone get assigned to look at this?

    dawehner’s picture

    Seeing this made me wonder: "how is this different than contextual links?".

    … turns out I wondered the same 6 months ago. See #124 — #132.

    Sorry, but I really think we should formulate a proper answer to that. I imagine a committer would ask that question too.

    Well, this is not only about your loved contextual links but also about links on the block UI.

    Status: Needs review » Needs work

    The last submitted patch, 151: helpful-block-links.151.patch, failed testing.

    MattA’s picture

    Issue tags: +Needs reroll

    Didn't think so...

    jgeryk’s picture

    Assigned: Unassigned » jgeryk

    Going to reroll

    jgeryk’s picture

    Assigned: jgeryk » Unassigned
    Issue tags: -Needs reroll
    FileSize
    7.56 KB
    PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,011 pass(es). View

    rerolled

    MattA’s picture

    Status: Needs work » Needs review
    EclipseGc’s picture

    Status: Needs review » Needs work
    +++ b/core/modules/block/src/BlockListBuilder.php
    @@ -342,6 +343,10 @@ public function getDefaultOperations(EntityInterface $entity) {
    +    if ($entity instanceof OperationsProviderInterface) {
    +      $operations += $entity->getOperationLinks();
    +    }
    +
    
    +++ b/core/modules/block/src/Entity/Block.php
    @@ -54,7 +55,7 @@
    +class Block extends ConfigEntityBase implements BlockInterface, EntityWithPluginCollectionInterface, OperationsProviderInterface {
    

    This seems completely unnecessary to me. The block entity can have this method without needing some instanceof check. If we want to implement the interface explicitly on the entity, that's fine (or have BlockInterface extend it), but the ListBuilder certainly doesn't need to explicitly check. We know this is part of the contract.

    The rest of this patch seems pretty straight forward and obvious. Having plugins implement the interface (and subsequent instanceof checks as appropriate) is super sensible. Likewise, to Wim's question, it seems to me as though the contextual links code should probably call this as well.

    Eclipse

    mgifford’s picture

    Status: Needs work » Needs review
    FileSize
    6.71 KB
    PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,770 pass(es). View

    Ok, then we should be able to nix this too.

    +use Drupal\Core\Operations\OperationsProviderInterface;

    andypost’s picture

    1. diff --git a/core/modules/block/src/Entity/Block.php b/core/modules/block/src/Entity/Block.php
      old mode 100644
      new mode 100755
      

      not needed

    2. +++ b/core/modules/block/src/Entity/Block.php
      @@ -54,7 +55,7 @@
      -class Block extends ConfigEntityBase implements BlockInterface, EntityWithPluginCollectionInterface {
      +class Block extends ConfigEntityBase implements BlockInterface, EntityWithPluginCollectionInterface, OperationsProviderInterface {
      

      this needs CR

    3. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
      @@ -41,6 +45,11 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa
      +   * @var \Drupal\Core\Extension\ModuleHandlerInterface.
      ...
      +  protected $moduleHandler;
      

      nit, needs description

    4. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
      @@ -53,11 +62,14 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa
      +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
      +   *   The module handler service.
      

      The module handler.

    EclipseGc’s picture

    Can we get interdiffs? Makes subsequent reviews a LOT faster.

    Eclipse

    catch’s picture

    Priority: Normal » Major
    Issue tags: +blocker
    lokapujya’s picture

    Providing an interdiff.

    EclipseGc’s picture

    Status: Needs review » Needs work
    +++ b/core/modules/block/src/BlockListBuilder.php
    @@ -347,10 +346,6 @@ public function getDefaultOperations(EntityInterface $entity) {
    -      $operations += $entity->getOperationLinks();
    

    This still needs to be done, but since it's on the entity directly, there's no need to check the instanceof check since this is a builder literally for that entity.

    Honestly, there's a lot of extra code here, odds are in the BlockListBuilder, we could circumvent doing anything with the Entity at all (in terms of new interfaces and such) and instead do:

    
    if ($entity->getPlugin() instanceof OperationsProviderInterface) {
      $operations += $entity->getPlugin()->getOperationLinks();
    }
    
    

    That would circumvent all the additional code for the block Entity itself and get straight to the job of checking the block plugin for operation links (which has to happen anyway).

    I don't like all the additional code added to the Block plugin just to check if menu_ui is installed. Perhaps that's unavoidable, but it sucks.

    Eclipse

    Version: 8.0.x-dev » 8.1.x-dev

    Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

    Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    yoroy’s picture

    Version: 8.1.x-dev » 8.2.x-dev
    Issue tags: +ux-workflow

    Version: 8.2.x-dev » 8.3.x-dev

    Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    dawehner’s picture

    FileSize
    6.74 KB

    Just rerolled the patch and addressing the feedback of @andypost

    andypost’s picture

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

    Maybe better to move that to menu_ui module?

    tim.plunkett’s picture

    Priority: Major » Normal

    Can this be closed now that Settings Tray is in core?

    Bojhan’s picture

    I am not sure? Accessing these settings from the blocks page is still not that easy?

    tim.plunkett’s picture

    Well instead of editing it from admin/structure/block, you would be able to edit it from the front end

    Version: 8.3.x-dev » 8.4.x-dev

    Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    jibran’s picture

    Title: Provide helpful editing links on "admin/structure/block" for different blocks (menu, views etc.) » Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.)
    Issue summary: View changes
    Status: Needs work » Needs review
    Issue tags: -Needs issue summary update
    FileSize
    2.67 KB
    11.14 KB
    17.06 KB

    Re: #177: I still think we should keep this.
    Re: #173: I think it is fine. If we move the links then we need a hook alter for this.

    • Rerolled the patch.
    • Conflict resolution file is attached.
    • Added extra operation links to all deriver blocks ack -l `find core/ -type f -path \*src/Plugin/Block\*` --match deriver.
    • Added missing piece which is removed in #163 so #162 is still pending. I think adding a new interface is fine.
    • Updated issue summary.
    • Added tests for \Drupal\views\Plugin\Block\ViewsBlockBase and \Drupal\block_content\Plugin\Block\BlockContentBlock

    Status: Needs review » Needs work

    The last submitted patch, 178: provide_helpful_editing-1956134-178.patch, failed testing.

    jibran’s picture

    Status: Needs work » Needs review
    FileSize
    2.61 KB
    19.67 KB

    Fixed the fails.

    dawehner’s picture

    +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
    @@ -185,4 +186,18 @@ protected function getEntity() {
    +    return [
    +      'block-edit' => [
    +        'title' => $this->t('Edit block'),
    +        'url' => $entity->toUrl('edit-form')->setOptions([]),
    +        'weight' => 50,
    +      ],
    
    +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
    @@ -181,4 +195,21 @@ public function getCacheContexts() {
    +      $links['menu-edit'] = [
    +        'title' => $this->t('Edit menu'),
    +        'url' => Url::fromRoute('entity.menu.edit_form', ['menu' => $menu]),
    +        'weight' => 50,
    +      ];
    

    Shouldn't we apply access checking here?

    jibran’s picture

    Re #181: is it needed? I'm not sure.
    @dawehner told me to make my case whether this issue is still helpful or not.

    I have never use outside_in or quickedit on a production site in D8. I was never a fan of IPE in D7 as well but I like that views blocks have the ability to override views settings. I even use ctools_views module which further enhances this functionality so for me, this is more in line with that.