Add a Views field handler for multiple entity operations

Can't you always pull the entity type/entity ID from the actual metadata available on the running view?

Cool, nice start! I meant to open this issue but obviously forgot...

You can remove a few things from the handler, like the storage controller, and do something more like this. Sorry, this was quicker/easier than explaining.

Handlers also have a getEntityType() method, which might be what Daniel means in #2? So maybe just pass in the EntityManager instead too.

I guess you also didn't get round to doing the destination stuff. How were you planning on implementing that?

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,116 @@
   +   * Constructor.
   +   *
   +   * @param array $configuration
   +   * @param string $plugin_id
   +   * @param array $plugin_definition
   +   * @param \Drupal\Core\Entity\EntityListControllerInterface $entity_list
   +   *    The list controller of the entity type.
   +   */
   

   No docs are better than these docs.

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,116 @@
   +      '#title' => t('Include destination'),
   

   $this->t()

3. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,119 @@
   +      'description' => '',
   

   Nice description +! i think it is pointless to require getInfo on unit tests.

4. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,119 @@
   +    $this->plugin = $this->getMockBuilder('\Drupal\views\Plugin\views\field\EntityOperations')
   ...
   +      ->getMock();
   ...
   +      ->setMethods(array('drupalGetDestination', 'getEntity', 't'))
   ...
   +      ->setConstructorArgs(array($configuration, $plugin_id, $plugin_definition, $this->entityList))
   

   Any reason why we don't extend the class as in other tests?

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -95,9 +90,27 @@ public function buildOptionsForm(&$form, &$form_state) {
   -    if ($entity = $this->getEntity($values)) {
   -      return $this->entityList->buildOperations($entity);
   +    $entity = $this->getEntity($values);
   +    if ($entity) {
   

   I'm not seeing the point of this change? Just a wasted additional line of code. Can you revert or just assume that an entity will be loaded (we do both)?

2. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,119 @@
   +    $this->plugin = $this->getMockBuilder('\Drupal\views\Plugin\views\field\EntityOperations')
   +      ->setConstructorArgs(array($configuration, $plugin_id, $plugin_definition, $this->entityList))
   +      ->setMethods(array('drupalGetDestination', 'getEntity', 't'))
   +      ->getMock();
   

   Yeah, I think we should extend the class instead, not test using a mock.

3. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,119 @@
   +    $this->assertInternalType('array', $this->plugin->buildOptionsForm($form, $form_state));
   ...
   +    $this->assertInternalType('array', $this->plugin->defineOptions());
   ...
   +    $this->assertInternalType('array', $build);
   ...
   +    $this->assertInternalType('array', $build);
   

   This is not that useful. We're not really testing anything.

Maybe we should also override the query method here aswell, same as the Links handler.

Have you tested this yet? I guess not as there is no views data implementation :)

We already had a chat on IRc, but ...

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,125 @@
   +      $operations = $this->entityManager->getListController($this->getEntityType())->getOperations($entity);
   

   This can't/shouldn't really live in here. render() will get called for every row. That's a call to getEntityType, getListController(), getOperations(), and getEntity() on every row. You can add the entityType and ListController to an init method.

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,125 @@
   +          $operation['options']['query'] += $this->drupalGetDestination();
   

   We can probably do a similar thing to destination aswell? This is not going to change per row, so better to not call drupal_get_destination() every time.

+++ b/core/modules/node/node.views.inc
@@ -214,6 +214,14 @@ function node_views_data() {
+  $data['node']['operations'] = array(
+    'field' => array(
+      'title' => t('Operations links'),
+      'help' => t('Provide links to perform content operations.'),
+      'id' => 'entity_operations',
+    ),
+  );
+

Also, just adding this ad hoc for node is not what we want. we need to add this for entity types generally. We don't have to use it yet, but we want to be able to.

@Xano
This sounds like a great idea.

Overall, I do love what this patch is trying to achieve. Being able to remove x number of field handlers and replace with one is a win for sure.

1. +++ b/core/modules/node/config/views.view.content.yml
   @@ -352,6 +304,57 @@ display:
   +          empty_zero: ¶
   

   oops

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,123 @@
   +      '#description' => $this->t('Include a <code>destination

   parameter in the link to return the user to the original view upon completing the link action.'),
   

   Not sure why this needs to be wrapped in code tags? Can we remove those?

3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,123 @@
   +    $operations = $this->entityManager->getListBuilder($this->getEntityType())->getOperations($entity);
   

   Why do we need to get the list builder every time? I think we could just store it.

4. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,123 @@
   +  }
   

   Nitpick: empty line after query() closing brace.

5. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,123 @@
   +   * The entity manager used for testing.
   

   "The mock entity manager"? - We know this will be used in the test :)

6. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,123 @@
   +      'name' => '\Drupal\views\Plugin\views\field\EntityOperations',
   

   Not a good name. Should be "Field: Entity operations" I think.

7. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,123 @@
   +    $this->assertInternalType('array', $form);
   +    $this->assertArrayHasKey('destination', $form);
   ...
   +    $this->assertInternalType('array', $options);
   +    $this->assertArrayHasKey('destination', $options);
   ...
   +    $this->assertInternalType('array', $build);
   ...
   +    $this->assertInternalType('array', $build);
   

   These still don't really test much :(

8. +++ b/core/modules/views/views.views.inc
   @@ -128,6 +128,16 @@ function views_views_data() {
   +          'title' => t('Operations links'),
   

   'Operation links' instead? 'Operations links' sounds weird. Maybe that's just me.

1. +++ b/core/modules/node/config/views.view.content.yml
   @@ -352,6 +304,57 @@ display:
   +        operations:
   +          id: operations
   +          table: node
   +          field: operations
   +          relationship: none
   +          group_type: group
   +          admin_label: ''
   +          label: Operations
   +          exclude: false
   

   I do hate it that you cannot longer choose the visual representation, which you could do before with the dropbutton.

2. +++ b/core/modules/node/config/views.view.content.yml
   @@ -352,6 +304,57 @@ display:
   +          empty_zero: ¶
   

   TRAILING SLASH!

3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,123 @@
   +  public function render(ResultRow $values) {
   +    $entity = $this->getEntity($values);
   +    $operations = $this->entityManager->getListBuilder($this->getEntityType())->getOperations($entity);
   +    if ($this->options['destination']) {
   +      foreach ($operations as &$operation) {
   +        if (!isset($operation['options']['query'])) {
   +          $operation['options']['query'] = array();
   +        }
   +        $operation['options']['query'] += $this->drupalGetDestination();
   +      }
   +    }
   +    $build = array(
   +      '#type' => 'operations',
   +      '#links' => $operations,
   +    );
   +
   +    return $build;
   +  }
   

   We should do something different. the getLinks on Links.php should be able to retrieve all the entity operations, so you can use dropbutton etc. This would basically be one field for the operations and one for the actual rendering.

Well sure, we can implement the API support first and then get the dropbutton in.

Thank you for ignoring my previous suggestion.

Well sure, we can implement the API support first

Yeah you are so right.

Well, having two total different ways in views to provide links and render them in some ways is odd.
I don't really get why my approach does not directly solve the issue? Is it because there are test failures at the moment or does not include your parts?

I can't join IRC during the day.

Not sure what you talk about. Views is not an entity list and will never be. It is a broken concept and should have been kept to only work with config entities. On the other hand views shipped with this kind of link support at least since D7, so you can easily argue that support the views way is more semantic correct.

In context of related issue it makes sense to expose operations for all entities by unified plugin

+++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
@@ -0,0 +1,117 @@
+  public function render(ResultRow $values) {
+    $entity = $this->getEntity($values);
+    $operations = $this->entityManager->getListBuilder($entity->getEntityTypeId())->getOperations($entity);

let's check first that entity type has list builder

+++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
@@ -0,0 +1,123 @@
+    // Skip rendering if the entity doesn't even have a list builder.
+    if (!$this->entityManager->hasController($entity->getEntityTypeId(), 'list_builder')) {
+      return array();

+++ b/core/modules/views/views.views.inc
@@ -136,6 +136,16 @@ function views_views_data() {
+    if ($entity_type->getBaseTable()) {
+      $data[$entity_type->getBaseTable()]['operations'] = array(

suppose it makes sense to add the same check additionally to base table

As written in march #2165989-32: Add a Views field handler for multiple entity operations we want to be able to implement things nicely. Therefore I created an issue which does it the views way,
which then would make this handler redundant again: #2324961: Allow field handlers to return multiple links

@Xano
I know you disagree and this is fine, but at least once both issues are in we can share a hell lot of code.

I d like to see both abilities in core:
1) operations as dropdown - like most of admin forms are
2) separate links, because I used a lot special UIs when only single 'edit' or 'change' is needed

To be honest I think we actually want to have some basic integration test here as well.

1. +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,118 @@
   +    $operations = $this->entityManager->getListBuilder($entity->getEntityTypeId())->getOperations($entity);
   

   It feels kinda wrong, but yeah this is for some reasons the "official" ""API"" at the moment.

2. +++ b/core/modules/views/src/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,179 @@
   +/**
   + * @coversDefaultClass \Drupal\views\Plugin\views\field\EntityOperations
   + */
   +class EntityOperationsUnitTest extends UnitTestCase {
   +
   

   don't we need a group here?

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -147,6 +147,15 @@ public function getViewsData() {
   +    if ($this->entityManager->hasHandler($this->entityType->id(), 'list_builder')) {
   

   Couldn't we just call $this->entityType->hasHandlerClass(..) instead?

2. +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,118 @@
   +  public function query() {
   +  }
   

   Maybe we should add a comment in the empty body here that we don't want to ensure anything/create any aliases.

3. +++ b/core/modules/views/src/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,179 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'description' => '',
   +      'name' => '\Drupal\views\Plugin\views\field\EntityOperations',
   +      'group' => 'Views Plugins',
   +    );
   +  }
   

   I think we can just remove this now and add @group instead.

4. +++ b/core/modules/views/src/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,179 @@
   +  public function testUsesGroupBy() {
   +    $this->assertFalse($this->plugin->usesGroupBy());
   +  }
   

   This is bordering on pointless, but I guess it's there now :)

Oh yeah, so much work is needed.

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -147,7 +147,7 @@ public function getViewsData() {
   +    if ($this->entityType->hasListBuilderClass()) {
   

   That was just a general suggestion, so yes, using that looks great.

2. +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   @@ -113,6 +113,7 @@ public function render(ResultRow $values) {
   +    // There is nothing to ensure or add for this handler.
   

   Sorry, can you add to this and say we purposely are not calling parent or something?

Otherwise, this is RTBC.

I've tested this patch manually.

The operations list has the correct links, and the dropbutton renders correctly in Views preview.
But when I go to my actually page (admin/products in this case), the list is rendered bare, without the dropbutton, even though it has the necessary classes.
The top div has class="dropbutton-wrapper", but no dropbutton-processed, so maybe the required JS just wasn't included / ran?

Yes. Also tried disabling the aggregation completely. It's not that.

I still think that we should have a dedicated integration test here because what we actually do here is not complex logic
but rather an integration between various bits.

Re rolled the patch as the content view has changed.

The interdiff contains the test I've added for this.

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -147,6 +147,15 @@ public function getViewsData() {
   +    if ($this->entityType->hasListBuilderClass()) {
   +      $data[$base_table]['operations'] = array(
   +        'field' => array(
   +          'title' => $this->t('Operations links'),
   +          'help' => $this->t('Provides links to perform entity operations.'),
   +          'id' => 'entity_operations',
   +        ),
   +      );
   +    }
   

   Are we sure that this if() makes really sense here? I could easily imagine that entity types don't want to use entity list classes at all, but instead always use views, in which case they would not have the operations available.

2. +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,120 @@
   +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity.manager'));
   

   Just in case you want to, please split it up into multiple lines, as it makes things easier to read / adapt in the future.

3. +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   @@ -0,0 +1,120 @@
   +      'bool' => TRUE,
   

   no need for bool TRUE anymore.

Thanks for the review @dawehner
Fixed comments in #105 and the failing test.

Some nitpicks :(

1. +++ b/core/modules/views/src/Tests/Handler/FieldEntityOperationsTest.php
   @@ -0,0 +1,58 @@
   + * Definition of Drupal\views\Tests\Handler\EntityOperations.
   

   Contains ... and wrong classname :(

2. +++ b/core/modules/views/src/Tests/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -0,0 +1,169 @@
   +/**
   + * @file Contains \Drupal\views\Tests\Plugin\views\field\EntityOperationsUnitTest.
   

   new line after @file

Sorry about those, fixed comments in #108

Thank you, especially for fixing this annoying things, which are kind of a Drupalism.

This issue introduces a new feature, so per https://www.drupal.org/core/beta-changes, we should postpone it to 8.1.x or later.

Any reasons why we shouldn't postpone this?

@alexpott
A handler that includes all possible operations for an entity is one of the most common needed handlers for entity listings done via views, and the one most frequently reinvented in D7 contrib. Sure, the end result can be faked by declaring a bunch of individual link handlers, then merging their results together, but this puts the pressure and boilerplate on the contrib authors.

@alexpott
Yeah this for sure improves the development experience, but it also fixes for example admin/content

You don't see the translation link on there (remember all those optional handlers discussion?). Instead it just provides all exposed operations the user has access to.

#113 suggest that this is actually a regression fix then :)

As per #113/#114 reclassifying as a regression fix. Aren't there other admin views we should update?

Plus can someone add the beta evaluation template to the IS.

Agreed the following views has to be updated:

• user_admin_people
• content (already covered)

Modified admin/people to use operation links as for #116

Added beta evaluation summary table

Awesome!

RTBC if green.

EDIT: dawehner beat me to it.

This issue should not be considered to be unfrozen. We are choosing to proceed because it fixes a regression and it fixes a bug.

Looking at the tests it appears that the

Bug because the operation links displayed currently in views might show operations that user has not access to

is not actually tested. Let's add a test to NodeAdminTest that users don't get operations they don't have access to.

Can we add a assertion somewhere in content_translation that the expected operation link is available.

Here's a test on content translation that shows the operations and that users with not enough permissions will not get them displayed.

And we're back. Thanks, Pedro.

Just in time to miss beta5...

+    if ($this->entityType->hasListBuilderClass()) {

Are we sure that this if() makes really sense here? I could easily imagine that entity types don't want to use entity list classes at all, but instead always use views, in which case they would not have the operations available.

In that case the operations field should not be available, because the list builder is used to generate the operations links, and a fatal error occurs when trying to access the undfined list builder.

I tested this on an entity without a list builder. To make it work, I specified EntityListBuilder as list builder. It is indeed strange that I need to specify a list builder to avoid using a list builder... but I gather from #92 that the idea of moving the operations definition to a concept on its own has already been lengthily discussed and eventually discarded.

TL;DR: I think the hasListBuilderClass() check should remain.

Otherwise: nice!

+++ b/core/modules/views/src/Tests/Plugin/views/field/EntityOperationsUnitTest.php
@@ -0,0 +1,170 @@
+      ->will($this->returnValue($entity_type_id));
...
+      ->will($this->returnValue($operations));
...
+      ->will($this->returnValue($list_builder));
...
+      ->will($this->returnValue($entity_type_id));
...
+      ->will($this->returnValue($operations));
...
+      ->will($this->returnValue($list_builder));

Not a blocker at all, just given a hint for the future: There is a thing called ->willReturnexists as a shortcut.

+1 for the RTBC though.

Committed 3f29fd9 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

+++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
@@ -0,0 +1,124 @@
+        if (!isset($operation['options']['query'])) {
+          $operation['options']['query'] = array();
+        }
+        $operation['options']['query'] += drupal_get_destination();

I think this should be $operation['query']? Otherwise the destination option does not have any effect.

@Arla please file follow-up

Created followup: #2419905: Views field 'Operations links' fails to set destination