Updated: Comment #23

Problem/Motivation

function hook_entity_operation_alter(array &$operations, \Drupal\Core\Entity\EntityInterface $entity) {}
was added in
#2004408-34: Allow modules to alter the result of EntityListController::getOperations
(there were some followups for random fails, so the history there is a bit confusing)

That hook was useful in #2004428: Less ugly operations altering

But, the hook did not effect some of the entities, blocks was one of the ones the hook did not effect.
See #17-#24 in #2004428-17: Less ugly operations altering

Proposed resolution

Make the hook work for blocks by implementing getOperations() in blocks and using buildOperations().
Add tests for blocks in block. It is the concern of blocks to test it's own stuff.

Remaining tasks

  • none

User interface changes

No.

API changes

No.

Blocking

Files: 
CommentFileSizeAuthor
#29 interdiff.txt1.51 KBpenyaskito
#29 drupal8.block-module.2027857-29.only-test.patch1.91 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 57,301 pass(es), 1 fail(s), and 0 exception(s). View
#29 drupal8.block-module.2027857-29.patch4.14 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 57,009 pass(es). View
#21 drupal8.block-module.2027857-21-testonly.patch1.91 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 56,478 pass(es), 1 fail(s), and 0 exception(s). View
#21 drupal8.block-module.2027857-21.patch4.17 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,056 pass(es). View
#21 interdiff-2027857-6-21.txt4.36 KBYesCT
#11 drupal8.block-module.2027857-11-justtests-shouldfail.patch9.25 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 58,009 pass(es), 1 fail(s), and 0 exception(s). View
#11 drupal8.block-module.2027857-11.patch11.51 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,991 pass(es). View
#11 interdiff-2027857-8-11.txt7.88 KBYesCT
#8 drupal8.block-module.2027857-8-test-only.patch9.56 KBkfritsche
FAILED: [[SimpleTest]]: [MySQL] 58,017 pass(es), 1 fail(s), and 0 exception(s). View
#8 drupal8.block-module.2027857-8.patch11.82 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 56,780 pass(es). View
#8 interdiff-6-8.txt13.88 KBkfritsche
#5 drupal8.block-module.2027857-6.patch5.53 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 58,043 pass(es). View
#5 interdiff-4-6.txt2.23 KBkfritsche
#4 drupal8.block-module.2027857-3.patch3.3 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#4 interdiff-1-3.txt1.84 KBYesCT
#1 drupal8.block-module.2027857-1.patch3.27 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 58,048 pass(es), 1 fail(s), and 0 exception(s). View

Comments

YesCT’s picture

Component: menu system » block.module
Status: Active » Needs review
FileSize
3.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,048 pass(es), 1 fail(s), and 0 exception(s). View

oops a bit of confusion between the similar issues.

kfritsche’s picture

This needs to be fixed in Core.

$this->buildOperations($entity) must be used to create the operations array in the Block List Controller. Also the buildRow() method should be used if possible and call parent.

YesCT’s picture

Issue tags: -Needs tests

noticed some things while working on this and menu issue.
build -> built
also the hook function needs to use *this* module name,
and we need to enable the test module so the hook has effect.

we expect this to fail, because of #2

ready for review.

YesCT’s picture

FileSize
1.84 KB
3.3 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

forgot the patch.

kfritsche’s picture

FileSize
2.23 KB
5.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,043 pass(es). View

BlockListController is using now buildOperations() so this should go green.

Don't know if we really need buildRow() here as the rows are grouped by region which makes it more complicated to use buildRow() here. Any comments about appreciated. But the main problem of the issue should be fixed with the patch.

tim.plunkett’s picture

I agree there is no need for buildRow().

+++ b/core/modules/block/lib/Drupal/block/BlockListController.phpundefined
@@ -274,6 +265,22 @@ public function buildForm(array $form, array &$form_state) {
+      'href' => 'admin/structure/block/manage/' . $entity->id() . '/configure',
...
+      'href' => 'admin/structure/block/manage/' . $entity->id() . '/delete',

This should use $entity->uri(), see the other list controllers.

+++ b/core/modules/block/tests/block_test_hook_operation/block_test_hook_operation.moduleundefined
@@ -0,0 +1,19 @@
+function block_test_hook_operation_entity_operation_alter(array &$operations, \Drupal\Core\Entity\EntityInterface $entity) {

Do we really need to implement this for each one? Can't it just enable all of the config entities and test them?

Also this is misnamed, the _hook_ can't be in there.

And the EntityInterface should be `use`'d.

YesCT’s picture

@tim.plunkett Thanks. OK. dont use hook in a name. I'll remember. :)

Don't we only "use" something if it's used more than once?
Maybe I'm reading this wrong:

Note that if a class is used more than once in multiple hook signatures, it must still be "use"ed, and then only the short names of the class should be used in the function.

https://drupal.org/node/1353118

Should we move the test module to the entity module?

Also, I've only made issues to add tests for menu, views, blocks..

Can't it just enable all of the config entities and test them?

are you suggesting we duplicate all the tests from config_translation?

http://drupalcode.org/project/config_translation.git/tree/HEAD:/lib/Drup...
Most are in one file, but views is in separate.
http://drupalcode.org/project/config_translation.git/blob/HEAD:/lib/Drup...
http://drupalcode.org/project/config_translation.git/blob/HEAD:/lib/Drup...

kfritsche’s picture

FileSize
13.88 KB
11.82 KB
PASSED: [[SimpleTest]]: [MySQL] 56,780 pass(es). View
9.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,017 pass(es), 1 fail(s), and 0 exception(s). View

New patch which uses the $entity->uri() now.

Concerning the tests, I found a already existing test for the entity operation in the system module.
I moved the block test code there and also added the tests for the other entity operations.
So everything is there now. More or less its the same as in config_translation.

I think its much better in this way now.

kfritsche’s picture

Issue tags: +sprint
YesCT’s picture

I'll test this and review it now. :)

YesCT’s picture

FileSize
7.88 KB
11.51 KB
PASSED: [[SimpleTest]]: [MySQL] 57,991 pass(es). View
9.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,009 pass(es), 1 fail(s), and 0 exception(s). View

A. The fix looks good.

B. This looks really great. :) Good location for the tests.

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
    @@ -32,11 +48,23 @@ public static function getInfo() {
    -    // Create and login user.
    -    $this->web_user = $this->drupalCreateUser(array(
    +    $permissions = array(
    +      'access site-wide contact form',
    +      'administer blocks',
    +      'administer contact forms',
    +      'administer filters',
    +      'administer menu',
           'administer permissions',
    -    ));
    -    $this->drupalLogin($this->web_user);
    +      'administer shortcuts',
    +      'administer site configuration',
    +      'administer taxonomy',
    +      'administer users',
    +      'administer views',
    +    );
    +
    +    // Create and log in user.
    +    $admin_user = $this->drupalCreateUser($permissions);
    +    $this->drupalLogin($admin_user);
    

    Is changing from a web_user with fewer permissions to an admin user with more permissions removing some test that was here before or changing what it was testing before?

    Answering my own question:
    oh, I think this test was only doing the foreach role on the roles page, so it's ok here to just use the admin user like it is.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
    @@ -45,14 +73,191 @@ public function setUp() {
    +   * Tests custom block lists to see if test_operation link is added to operations.
    ...
    +   * Tests contact forms lists to see if test_operation link is added to operations.
    

    changing the comment to refer to test_operation pushes this over 80 chars.

    And in some other places.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
    @@ -45,14 +73,191 @@ public function setUp() {
    +    // Create a test role to decouple looking for test_operation operations
    +    // link so this does not test more than necessary.
    +    $role_id = Unicode::strtolower($this->randomName(16));
    +    $this->drupalCreateRole(array(), $role_id);
    +
    +    // Get the role listing.
         $this->drupalGet('admin/people/roles');
    -    $roles = user_roles();
    -    foreach ($roles as $role) {
    -      $uri = $role->uri();
    -      $this->assertLinkByHref($uri['path'] . '/test_operation');
    -      $this->assertLink(format_string('Test Operation: @label', array('@label' => $role->label())));
    -    }
    +
    +    $test_operation_link = 'admin/people/roles/manage/' . $role_id . '/test_operation';
    +    // Test if the link to test_operation the role is on the page.
    +    $this->assertLinkByHref($test_operation_link);
    

    Mmm. The old test here was looking for the test_operation of "each" role. But we are not sure there were any.

    The new test is just looking for the link on one role.

    Is it worth merging the approaches? Making a test role, and then *also* doing a foreach?

    I could see some strange bug one day that might do something like only the first item in the list gets the operation added.

    Maybe we should make a few test things, and do the foreach. Is it worth it to do that? (I did not do that.)

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
    @@ -45,14 +73,191 @@ public function setUp() {
    +    // Test if the link to test_operation the vocabulary is on the page.
    

    heh. This used to make sense when it was "Test if the link to translation the vocab..." Fixed.

    +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
    @@ -45,14 +73,191 @@ public function setUp() {
    +    // Views UI List 'admin/structure/views'.
    

    This is I think left over from when we had all the tests in one giant function and we had section headers. Also, it's not a sentence. I think with the nice functions this is not needed anymore. Taking it out.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
    @@ -7,19 +7,35 @@
    +   * @var array
    +   */
    +  public static $testViews = array('test_view');
    +  /**
        * Modules to enable.
    

    missing newline. added it.

oh, also when the method for the views got pasted in, the newline before the last } on the class was lost. I put the newline back in.

Gábor Hojtsy’s picture

I think all of the roles should have a translate tab/link, at least all those already in core. If you add a new one, if you have more than one language, it should also show up proper.

Gábor Hojtsy’s picture

Title: blocks: hook_entity_operation_alter() does not work with EntityListController » Blocks operations cannot be altered; fix and add tests for all entity lists
Issue tags: +Needs issue summary update

All right, so I got the scoop on this. We are altering in the operation and the prior tests only tested roles but tested all of them. The new tests test a lot more things, but it would be a pain if they would test all the items on all the pages.

This badly needs an issue summary update, but otherwise amazing. So not yet RTBC unfortunately :/

Gábor Hojtsy’s picture

In other words, the alter works the same way on all rows. Testing all rows on all entity list items sounds like *overkill*, it does not seem like we would gain something out of that, so the patch looks good not to test all lines in all lists :)

YesCT’s picture

updated the issue summary and marked #2023739: menu: hook_entity_operation_alter() does not work with EntityListController as a duplicate of this issue.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs issue summary update

Yay, thanks!

tstoeckler’s picture

The added tag looks like a cross-post.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
@@ -45,14 +74,189 @@ public function setUp() {
+  function doCustomBlockTypeListTest() {
...
+  function doContactFormsListTest() {
...
+  function doFormatsListTest() {
...
+  protected function doMenuListTest() {
...
+  function doShortcutListTest() {
...
+  function doUserRoleListTest() {
...
+  function doViewListUITest() {

Needs to be protected

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
@@ -45,14 +74,189 @@ public function setUp() {
+  /**
+   * Tests formats lists to see if test_operation link is added.
+   */
+  function doFormatsListTest() {
+    // Create a test format to decouple looking for test_operation operations
+    // link so this does not test more than necessary.
+    $filter_format = entity_create('filter_format', array(
+      'format' => Unicode::strtolower($this->randomName(16)),
+      'name' => $this->randomName(),
+    ));
+    $filter_format->save();
+
+    // Get the format listing.
+    $this->drupalGet('admin/config/content/formats');
+
+    $test_operation_link = 'admin/config/content/formats/manage/' . $filter_format->id() . '/test_operation';
+    // Test if the test_operation link is on the page.
+    $this->assertLinkByHref($test_operation_link);

I know that this is a duplicate test... there is a high chance we are introducing duplicate tests here. This issue should just fix the block operation alterability. Everything else is scope creep... sorry.

YesCT’s picture

OK.
So we will just do the test for blocks here, (and not use the doPattern here).

We will make the do... tests protected in config_translation where we use this same pattern.

Regarding

I know that this is a duplicate test... there is a high chance we are introducing duplicate tests here.

We talked about this, and since it is testing the addition of a link added by the alter, it's not actually a duplicate. :)

I'll work on this.
--------
After this issue, we should make a generic test that can be extended instead of putting a separate independent test in each place.
Once we do that, then we can use *that* in config_translation also.

YesCT’s picture

Assigned: Unassigned » YesCT
YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
4.36 KB
4.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,056 pass(es). View
1.91 KB
FAILED: [[SimpleTest]]: [MySQL] 56,478 pass(es), 1 fail(s), and 0 exception(s). View

I went back to #5 (aka 6). And redid the fixes asked for after that.

kfritsche’s picture

Status: Needs review » Reviewed & tested by the community

Tested it manually with the config_translation module with ugly form_alters removed. Translate link shows up in block structure. So this works.

Also locally the Test passed for me.

RTBC if testbot agrees.

YesCT’s picture

Title: Blocks operations cannot be altered; fix and add tests for all entity lists » Blocks operations cannot be altered

updating title as we have changed back to original strategy of just testing blocks.
I'll update the issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

added the issue to make tests generic

YesCT’s picture

Issue summary: View changes

added some fancy 'concern of' words.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

YesCT’s picture

YesCT’s picture

Issue summary: View changes

updating comment number that it's updated as.

YesCT’s picture

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
YesCT’s picture

tag!

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,009 pass(es). View
1.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,301 pass(es), 1 fail(s), and 0 exception(s). View
1.51 KB

interdiff is a diff between patches.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, back to RTBC assuming it passes.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed dfee120 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

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

Anonymous’s picture

Issue summary: View changes

blocking config translation in core, via the need to remove the ugly alter.