Introduce hook_entity_operation()

Well, we definitely need something like this at least.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.php
@@ -34,6 +35,27 @@ class EntityListControllerTest extends UnitTestCase {
+   * @var \Drupal\user\RoleStorageControllerInterface|\PHPUnit_Framework_MockObject_MockObject
...
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface|\PHPUnit_Framework_MockObject_MockObject
...
+   * @var \Drupal\Core\Entity\EntityTypeInterface|\PHPUnit_Framework_MockObject_MockObject

These look weird :)

We do that kind of style all over the place.

Yup, sorry, I had a very quick look and didn't notice it was a test.

+ *
+ * @return array
+ *   Operations array as returned by
+ *   \Drupal\Core\Entity\EntityListControllerInterface::getOperations().

It's usually normal for the hook docs to have the specification for the array structure, and the retrieval function to reference the hook docs.

Yeah, this certainly looks like a more sensible change to me. Why the hell do we have the alter hook in a method that is not a part of the interface? That doesn't really make alot of sense...

+1 to that change in principle.

Does getOperations() always get all of them?

What if you only wanted to show some of them?

1. +++ b/core/lib/Drupal/Core/Entity/EntityListController.php
   @@ -112,6 +112,24 @@ protected function getLabel(EntityInterface $entity) {
   +   *   self::getOperations().
   

   I guess this should not use $this yet? :)

2. +++ b/core/lib/Drupal/Core/Entity/EntityListController.php
   @@ -176,13 +194,9 @@ public function buildRow(EntityInterface $entity) {
   -    $this->moduleHandler->alter('entity_operation', $operations, $entity);
   

   Thank you! This living here was a mistake, for sure.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.php
   @@ -49,10 +71,32 @@ protected function setUp() {
   +    $list->getOperations($this->role);
   

   Shouldn't we assert the actual return value from getOperations() too?

4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.php
   @@ -49,10 +71,32 @@ protected function setUp() {
   +    $this->entityListController = new TestEntityListController($this->entityType, $this->roleStorage, $this->moduleHandler);
   ...
   +    $list = new EntityListController($this->entityType, $this->roleStorage, $this->moduleHandler);
   

   What's going on here? Are we testing with TestEntityListController or EntityListController? :)

5. +++ b/core/lib/Drupal/Core/Entity/EntityListController.php
   @@ -112,6 +112,24 @@ protected function getLabel(EntityInterface $entity) {
   +    $operations = array_merge($this->moduleHandler->invokeAll('entity_operation', array($entity)), $this->getDefaultOperations($entity));
   

   This means defaultOperations will always win over the hook declared operations if there is a key clash. Not saying it's wrong, but is that the desired behaviour?

I don't see anymore issues with this.

drupal_2165725_36.patch no longer applies.

error: patch failed: core/modules/system/entity.api.php:699
error: core/modules/system/entity.api.php: patch does not apply

I think this needs probably a change record, and draft change records are now required before commit. Please also search for existing change records that might need updates and link them here (or confirm that there are none). Thanks!

Looking at the two change records... the new draft at https://drupal.org/node/2198231 looks good, but https://drupal.org/node/2019575 is then a little confusing.

Can we merge the two? I.e., add everything from https://drupal.org/node/2019575 to the new draft at https://drupal.org/node/2198231, and then once this is committed, we can make the new change record live and unpublish the old one. Make sure to add a reference to #2004408: Allow modules to alter the result of EntityListController::getOperations on the new draft as well, I guess. Thanks!

Also, maybe the bottom section of https://drupal.org/node/1799642 needs an update?

41: drupal_2165725_41.patch queued for re-testing.

The only thing that's worth marking this needs work for is that config_translation_entity_operation_alter() should be re-purposed to config_translation_entity_operation_info().

Also, as I'm writing this I realize the following: Why is this not called hook_entity_operation_info()? I think that would be more consistent.

I also don't really know why #1839516: Introduce entity operation providers was abandoned. I liked that issue a lot more ... :-/

The rest are nitpicks:

1. +++ b/core/lib/Drupal/Core/Entity/EntityListController.php
   @@ -93,6 +93,24 @@ protected function getLabel(EntityInterface $entity) {
   +    $operations = array_merge($this->moduleHandler()->invokeAll('entity_operation', array($entity)), $this->getDefaultOperations($entity));
   

   Super minor but since we are using keyed arrays, I think something like

   $operations = $this->getDefaultOperations();
   $operations += $this->moduleHandler()...
   

   would be clearer.

2. +++ b/core/modules/system/entity.api.php
   @@ -756,6 +756,27 @@ function hook_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\Entit
   + *   Operations array as returned by
   

   Maybe "An array of operations as ..."

3. +++ b/core/modules/system/entity.api.php
   @@ -765,10 +786,13 @@ function hook_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\Entit
   +  // Alter the title and weight.
      $operations['translate'] = array(
   -    'title' => t('Translate'),
   -    'weight' => 50,
   -  ) + $entity->urlInfo('my-custom-link-template');
   +    'title' => t('Translate @entity_type', array(
   +      '@entity_type' => $entity->getEntityTypeId(),
   +    )),
   +    'weight' => 99,
   +  );
    }
   

   This is not altering but overriding the entire 'translate' operation.

4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.php
   @@ -49,10 +86,55 @@ protected function setUp() {
   +    \Drupal::setContainer($this->container);
   

   I know that that is not your fault but it is nothing but insane that we have to do this in a "unit" test.

This looks great. Sadly I have one more problem with this, see 2.

1. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
   @@ -23,6 +23,7 @@
   + *     "list_builder" = "\Drupal\Core\Config\Entity\ConfigEntityListBuilder",
   
   +++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php
   @@ -164,11 +154,9 @@ public function buildForm(array $form, array &$form_state, $entity_type_id = NUL
   +        '#links' => $this->entityManager->getListBuilder('field_instance_config')->getOperations($instance),
   

   So we're introducing a list builder here, but only using it for the operations? Hmm... It's definitely better than calling the hook manually, so yeah. And that's a step in the right direction, because field instances really should be using a *proper* list builder.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityListBuilderTest.php
   @@ -49,10 +86,55 @@ protected function setUp() {
   +    $operations = array(
   +      $this->randomName() => array(
   +        'title' => $this->randomName(),
   +      ),
   +    );
   ...
   +    $this->assertInternalType('array', $operations);
   +    $this->assertArrayHasKey('edit', $operations);
   +    $this->assertInternalType('array', $operations['edit']);
   +    $this->assertArrayHasKey('title', $operations['edit']);
   +    $this->assertArrayHasKey('delete', $operations);
   +    $this->assertInternalType('array', $operations['delete']);
   +    $this->assertArrayHasKey('title', $operations['delete']);
   

   I might be missing something but this is not actually testing that the operation that got added is there?

Awesome, looks great!

Committed/pushed to 8.x, thanks!

1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldInstanceListBuilder.php
   @@ -0,0 +1,90 @@
   +
   

   Unnecessary new line.

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldInstanceListBuilder.php
   @@ -0,0 +1,90 @@
   +    throw new \Exception('This class is only used for operations and not for building lists.');
   

   This seems a bit weird, but is a pre-existing condition. Having at least an exception now helps.

Thus, patch looks good and come with test coverage for the previous problem. RTBC again then.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListBuilderTest.php
--- /dev/null
+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldInstanceConfigListBuilder.php

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldInstanceConfigListBuilder.php
@@ -0,0 +1,92 @@
+/**
...
+ * Contains \Drupal\field_ui\FieldInstanceListConfigBuilder.
...
+class FieldInstanceListConfigBuilder extends ConfigEntityListBuilder {

I don't think this works, because the filename doesn't match the class name.

Regardless we should add some tests for this. Since this broke HEAD already that means we are lacking in this area.

Oops, sorry, I missed that. Looks great!

This unblocks some other refactoring as well as some contrib stuff, so re-categorizing as a task vs. a feature request, since we're not committing those.

I didn't look at the patch at all, but since catch had already committed it earlier and it was rolled back due to a regression, now fixed + test covered, this seems good to go.

Committed and pushed to 8.x. Thanks!

Follow-up, patch on the way: #2235347: Field instance operations are out of order