Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

FileSize
370 bytes

done with @effulgentsia and @Gabor

YesCT’s picture

Status: Active » Needs review
YesCT’s picture

Status: Needs review » Needs work

oops. patch of nothing

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

this should have some code.

also, fixed $uri = that was missing and some type casting to the function definition

Berdir’s picture

+++ b/core/includes/entity.api.phpundefined
@@ -538,6 +538,24 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+function hook_entity_operations_alter(array &$operations, \Drupal\Core\Entity\EntityStorageControllerInterface $entity) {

Wrong interface.

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -131,6 +131,7 @@ public function buildRow(EntityInterface $entity) {
+    \Drupal::moduleHandler()->alter('entity_operation', $operations, $entity);

This should use the inject thing now.

You just need to add two lines and some documentation.

add implements EntityControllerInterface.

Add a method like this:
/**
* {@inheritdoc}
*/
public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
return new static(
$entity_type,
$entity_info,
$container->get('plugin.manager.entity')->getStorageController($entity_type)
$container->get('module_handler')
);
}

Change the controller aguments to

$entity_type, $ntity_info, EntityStorageControllerInterface $storage, ModuleHandlerInterface $module_handler.

Assing module handler to $this->moduleHandler (document it) and $entity_info, call $this->moduleHandler->alter()

YesCT’s picture

@Berdir helped me to injected the moduleHandler

Could add php unit test.

YesCT’s picture

hook function definition did not match the function name being called.
changed to operation instead of operations. (@Berdir says similar to menu hooks)

Status: Needs review » Needs work
Issue tags: -API addition, -API clean-up, -D8MI, -language-config

The last submitted patch, operations_alter-2004408-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#7: operations_alter-2004408-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, operations_alter-2004408-7.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +API addition, +API clean-up, +D8MI, +language-config

#7: operations_alter-2004408-7.patch queued for re-testing.

YesCT’s picture

@fago says should have a hook for tab too.

fago’s picture

Triggered by this we discussed operations stuff today: #1839516: Introduce entity operation providers

YesCT’s picture

@fago does that help with the tabs too?

How do people feel about doing this for now, and then redoing it when/if #1839516 gets in?

YesCT’s picture

@fago Can we skip the alter hook for tabs? #2011332: Add alter for adding config translation tab

fago’s picture

Nop, that would not help with the tabs :/

How do people feel about doing this for now, and then redoing it when/if #1839516 gets in?

Sounds reasonable.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Agreed with @fago, this can be a stop-gap that #1839516: Introduce entity operation providers can remove, if it gets in. There are lots of deliberations going on there still....

Gábor Hojtsy’s picture

Once again this is a blocker for #1952394: Add configuration translation user interface module in core, because otherwise we need to do ungodly amounts of alters on forms and page arrays which would make that inacceptable in core. But there are numerous use cases.

Berdir’s picture

However, as long as that is not in core, it means that we have no (core) test coverage for this.

So we probably need to implement this hook somewhere in a test module for a test (or real) config entity that already uses the list controller and then verify that the operations can be changed. Otherwise we risk to break this as we refactor things.

Gábor Hojtsy’s picture

#1952394: Add configuration translation user interface module in core has plenty of test coverage that rely on this hook working. We can go and add specific tests for this but if that is the first use case for this in core and it only has 3.5 weeks to get in anyway, it does not sound like we'll be without test coverage for long.

alexpott’s picture

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

I wholeheartedly agree with #19... since we will probably refactor this definitely needs tests...

Lets create a test implementation of the hook...

Gábor Hojtsy’s picture

Ok sounds like the test would:

- add an alter to a test module
- have a test that enables that test module
- in the alter, add a new operation to entity lists (can have random name)
- in the test, check that the config entity listings carry that operation (like contact categories list or menus list)

Berdir’s picture

Yes, that.

Also, I was just wondering, maybe we can use this for field_ui to add manage fields/display operations instead of module_exists() checks, see e.g. Drupal\contact\CategoryListController::getOperations(). What I think we don't know yet is how to connect the message and category.

We can totally look into that in a follow-up. Or, if we have an easy solution for that, we could use that as test coverage...

penyaskito’s picture

Assigned: Unassigned » penyaskito
Status: Needs work » Needs review
FileSize
6.79 KB
2.94 KB

Added test according to #22.

penyaskito’s picture

Status: Needs review » Needs work

The last submitted patch, operations_alter-2004408-24.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.37 KB
6.34 KB

Code review:
this re-uses the admin/people/roles listing
to make it more independent... maybe it should add a test role?

I dont know if that is enough of a concern to make it needs work.

Other than that... I noticed these nits (which are not blocking). I just fixed them.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
@@ -0,0 +1,55 @@
+ * Definition of Drupal\system\Tests\Entity\EntityOperationsTest.

(test standards need updating...
#1869794: Update tests coding standards doc and make consistant with 1354 where appropriate )

lets go with
Contains \Drupal\system\Tests\Entity\EntityOperationsTest.

Comment on the class is:
* Tests for entity operations.

Is this the only place entity operations are tested? Because that sure sounds like it does more than just test if an operation in added to the operations drop down in lists of entities.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
@@ -0,0 +1,55 @@
+   * Check hook_entity_operation_alter hook is executed.

Checks ...
or
Tests ...
so it's third person verb

I added an @see to the test entity hook.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
@@ -0,0 +1,55 @@
+  }
+}

classes usually have a blank line before the closing }
https://drupal.org/node/1353118

+++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
@@ -381,7 +381,7 @@ function entity_test_entity_form_display_alter(EntityFormDisplay $form_display,
- * Implements hook_entity_presave()
+ * Implements hook_entity_presave().

@@ -390,10 +390,22 @@ function entity_test_entity_presave(EntityInterface $entity) {
- * Implements hook_entity_predelete()
+ * Implements hook_entity_predelete().

I agree these are not to standards, but lets do that clean up in a different issue. (we might have an issue for it already. let's check).
I'm being picky about the other things though because they are new lines or otherwise being changed because of the purpose of this issue.

Removing the needs tests tag, since this has tests now.

YesCT’s picture

Status: Needs review » Needs work

Tests failing

Argument 2 passed to Drupal\Core\Entity\EntityListController::__construct() must be an array, object given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/action/lib/Drupal/action/ActionListController.php on line 47 and defined

...

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -36,17 +46,34 @@ class EntityListController implements EntityListControllerInterface {
-  public function __construct($entity_type, EntityStorageControllerInterface $storage) {
+  public function __construct($entity_type, array $entity_info, EntityStorageControllerInterface $storage, ModuleHandlerInterface $module_handler) {

Can we change the order and put the new parameter last and make it optional?

Otherwise, we will be updating all the places that call the constructor, right?

tim.plunkett’s picture

Only ActionListController overrides __construct(), that's the only one that would need to be changed.
Also, EntityManager::getRenderController() would need to be changed.

I think those are reasonable to change.

YesCT’s picture

OK.
(between #7 and #24, #1846172: Replace the actions API went in.)

penyaskito’s picture

Status: Needs work » Needs review
FileSize
9.43 KB
3.1 KB

Followed @tim.plunkett suggestions at #30.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good, the test looks complete. Thanks for your effort!

alexpott’s picture

Title: Allow modules to alter the result of EntityListController::getOperations » Change notice: Allow modules to alter the result of EntityListController::getOperations
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Committed 3173573 and pushed to 8.x. Thanks!

Not 100% sure if we need a change notice here - or maybe some need to be updated.

tim.plunkett’s picture

This is causing a random failure.

Drupal\system\Tests\Entity\EntityOperationsTest 16 1 0
Message Group Filename Line Function Status
Link with label Test Operation: #svG\\:B found. Other EntityOperationsTest.php 54 Drupal\system\Tests\Entity\EntityOperationsTest->testEntityOperationAlter()
Screen Shot 2013-06-12 at 11.01.35 AM.png

tim.plunkett’s picture

Title: Change notice: Allow modules to alter the result of EntityListController::getOperations » Random failure: Allow modules to alter the result of EntityListController::getOperations
Status: Active » Needs review
FileSize
1.48 KB

I get fails locally 4/10 times with HEAD.
0/10 with the patch.

If the role label contains any special characters, it will fail unless check_plain'd

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Random test failure

Yes, looks good. Once again, formatString()--.

YesCT’s picture

Issue tags: -Random test failure

Yeah, I see too.
rtbc from me too.
If it fails on tests in core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.php
it's the random failure and should be re-tested.

YesCT’s picture

Issue tags: +Random test failure
penyaskito’s picture

/me is embarrassed. Thanks @timplunkett!

alexpott’s picture

Title: Random failure: Allow modules to alter the result of EntityListController::getOperations » Change notice: Allow modules to alter the result of EntityListController::getOperations
Status: Reviewed & tested by the community » Active
Issue tags: -Random test failure +Needs change record

Committed 0a01a89 and pushed to 8.x. Thanks!

Back to needing a change notice...

alexpott’s picture

Issue tags: +Random test failure

This is still causing the occasional random test failure... head just failed for the following...

Link with label Test Operation: 2t):|k& found.

alexpott’s picture

Status: Active » Needs review
FileSize
729 bytes
735 bytes
Gábor Hojtsy’s picture

Oh, ha, hum, good find!

Gábor Hojtsy’s picture

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

Status: Reviewed & tested by the community » Active
Issue tags: -Random test failure

Committed e689237 and pushed to 8.x.

Back for the change notice....

Gábor Hojtsy’s picture

Title: Change notice: Allow modules to alter the result of EntityListController::getOperations » Allow modules to alter the result of EntityListController::getOperations
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

Added two lines to the general change notice at https://drupal.org/node/1799642/revisions/view/2667814/2727793 (and added this issue to that change notice as well).

Also added a standalone change notice at https://drupal.org/node/2019575

YesCT’s picture

Using this works for some entities, but not for views or menus, #2004428-24: Less ugly operations altering.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -207,7 +207,7 @@ public function getRenderController($entity_type) {
-        $this->controllers['render'][$entity_type] = $class::createInstance($this->container, $entity_type, $this->getDefinition($entity_type));
+        $this->controllers['render'][$entity_type] = $class::createInstance($this->container, $this->getDefinition($entity_type), $entity_type);

Can someone explain what this line is for? I don't see anything else in this patch related to render controllers.

jibran’s picture

At #48 I have added minor nit picks in Entity listing pages can be provided by a list controller and added some code to change notice. IMO to have a code in change notice always helps to read an understand.

Berdir’s picture

tim.plunkett’s picture

Title: Allow modules to alter the result of EntityListController::getOperations » [HEAD BROKEN] Allow modules to alter the result of EntityListController::getOperations
Status: Fixed » Reviewed & tested by the community
FileSize
1.94 KB

This completely breaks the views UI, I have no idea how this passed before...

tim.plunkett’s picture

Category: task » bug
Priority: Major » Critical
webchick’s picture

Title: [HEAD BROKEN] Allow modules to alter the result of EntityListController::getOperations » Allow modules to alter the result of EntityListController::getOperations
Category: bug » task
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks a lot!

I'm a bit concerned though that this didn't flag anything in the tests. Do we need a follow-up for that, or was it a fluke?

webchick’s picture

Tim confirms we do have ample Views UI test coverage so this must've been some kind of fluke.

tim.plunkett’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
1.2 KB

We had one unit test for the previous signature of the constructor.

Before:
Views List Controller Unit Test 0 passes, 1 fail, and 0 exceptions

After:
Views List Controller Unit Test 1 pass, 0 fails, and 0 exceptions

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed that, too. :)

Status: Fixed » Closed (fixed)

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

Berdir’s picture

This still has random failures sometimes, see #2032565: Test failures due to special character (combinations) in random strings for the fun details.

damiankloip’s picture

Not sure why buildOperations() was chosen as a home for the alter hook? #2165725: Introduce hook_entity_operation() Changes this.

penyaskito’s picture

Assigned: penyaskito » Unassigned
Issue summary: View changes