Problem/Motivation
There is no sane way to add an operation to listings on configuration entities.
Will clean up the code in #1952394: Add configuration translation user interface module in core.
Proposed resolution
Add an alter step to getOperations().
Remaining tasks
- initial patch to demonstrate concept
- discuss
User interface changes
No
API changes
Yes
Comment | File | Size | Author |
---|---|---|---|
#57 | vdc-2004408-57.patch | 1.2 KB | tim.plunkett |
#53 | entity-type-2004408-53.patch | 1.94 KB | tim.plunkett |
#44 | operations-2004408-44.will-fail.patch | 735 bytes | alexpott |
#44 | operations-2004408-44.patch | 729 bytes | alexpott |
#37 | operations-2004408-37.patch | 1.48 KB | tim.plunkett |
Comments
Comment #1
YesCT CreditAttribution: YesCT commenteddone with @effulgentsia and @Gabor
Comment #2
YesCT CreditAttribution: YesCT commentedComment #3
YesCT CreditAttribution: YesCT commentedoops. patch of nothing
Comment #4
YesCT CreditAttribution: YesCT commentedthis should have some code.
also, fixed $uri = that was missing and some type casting to the function definition
Comment #5
BerdirWrong interface.
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()
Comment #6
YesCT CreditAttribution: YesCT commented@Berdir helped me to injected the moduleHandler
Could add php unit test.
Comment #7
YesCT CreditAttribution: YesCT commentedhook function definition did not match the function name being called.
changed to operation instead of operations. (@Berdir says similar to menu hooks)
Comment #9
Berdir#7: operations_alter-2004408-7.patch queued for re-testing.
Comment #11
aspilicious CreditAttribution: aspilicious commented#7: operations_alter-2004408-7.patch queued for re-testing.
Comment #12
YesCT CreditAttribution: YesCT commented@fago says should have a hook for tab too.
Comment #13
fagoTriggered by this we discussed operations stuff today: #1839516: Introduce entity operation providers
Comment #14
YesCT CreditAttribution: YesCT commented@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?
Comment #15
YesCT CreditAttribution: YesCT commented@fago Can we skip the alter hook for tabs? #2011332: Add alter for adding config translation tab
Comment #16
fagoNop, that would not help with the tabs :/
Sounds reasonable.
Comment #17
Gábor HojtsyAgreed 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....
Comment #18
Gábor HojtsyOnce 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.
Comment #19
BerdirHowever, 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.
Comment #20
Gábor Hojtsy#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.
Comment #21
alexpottI wholeheartedly agree with #19... since we will probably refactor this definitely needs tests...
Lets create a test implementation of the hook...
Comment #22
Gábor HojtsyOk 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)
Comment #23
BerdirYes, 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...
Comment #24
penyaskitoAdded test according to #22.
Comment #25
penyaskitoFor #23, created #2015829: Move field operations in contact.module to field_ui follow-up.
Comment #27
YesCT CreditAttribution: YesCT commentedCode 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.
(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.
Checks ...
or
Tests ...
so it's third person verb
I added an @see to the test entity hook.
classes usually have a blank line before the closing }
https://drupal.org/node/1353118
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.
Comment #28
YesCT CreditAttribution: YesCT commentedHere it is #2015849: Some standards fixes in entity_test.module and EntityApiTest.php, motivated by "DatabaseStorageController can't catch exceptions"
Comment #29
YesCT CreditAttribution: YesCT commentedTests failing
...
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?
Comment #30
tim.plunkettOnly 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.
Comment #31
YesCT CreditAttribution: YesCT commentedOK.
(between #7 and #24, #1846172: Replace the actions API went in.)
Comment #32
penyaskitoFollowed @tim.plunkett suggestions at #30.
Comment #33
Gábor HojtsyChanges look good, the test looks complete. Thanks for your effort!
Comment #34
alexpottCommitted 3173573 and pushed to 8.x. Thanks!
Not 100% sure if we need a change notice here - or maybe some need to be updated.
Comment #35
tim.plunkettThis 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()
Comment #37
tim.plunkettI 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
Comment #38
BerdirYes, looks good. Once again, formatString()--.
Comment #39
YesCT CreditAttribution: YesCT commentedYeah, 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.
Comment #40
YesCT CreditAttribution: YesCT commentedComment #41
penyaskito/me is embarrassed. Thanks @timplunkett!
Comment #42
alexpottCommitted 0a01a89 and pushed to 8.x. Thanks!
Back to needing a change notice...
Comment #43
alexpottThis is still causing the occasional random test failure... head just failed for the following...
Link with label Test Operation: 2t):|k& found.
Comment #44
alexpottComment #45
Gábor HojtsyOh, ha, hum, good find!
Comment #46
Gábor HojtsyComment #47
alexpottCommitted e689237 and pushed to 8.x.
Back for the change notice....
Comment #48
Gábor HojtsyAdded 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
Comment #49
YesCT CreditAttribution: YesCT commentedUsing this works for some entities, but not for views or menus, #2004428-24: Less ugly operations altering.
Comment #50
tstoecklerCan someone explain what this line is for? I don't see anything else in this patch related to render controllers.
Comment #51
jibranAt #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.
Comment #52
BerdirOpened #2021063: Use hook_entity_operation_alter() for manage fields and manage display links as a possible follow-up for this.
Comment #53
tim.plunkettThis completely breaks the views UI, I have no idea how this passed before...
Comment #54
tim.plunkettComment #55
webchickCommitted 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?
Comment #56
webchickTim confirms we do have ample Views UI test coverage so this must've been some kind of fluke.
Comment #57
tim.plunkettWe 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
Comment #58
webchickCommitted and pushed that, too. :)
Comment #60
BerdirThis still has random failures sometimes, see #2032565: Test failures due to special character (combinations) in random strings for the fun details.
Comment #61
damiankloip CreditAttribution: damiankloip commentedNot sure why buildOperations() was chosen as a home for the alter hook? #2165725: Introduce hook_entity_operation() Changes this.
Comment #62
penyaskito