I made a config entity module for the Examples project which was as simple as I could possibly make it: #2084301: Add a Config Entity example.
Having done that, it seems to me that the amount of code needed to define a config entity could do to be reduced. I think a lot of contrib modules are going to be using config entities, as they make the most sense to use as soon as you have any kind of list of things the user can add to. (Eg: flag, simplenews, nodequeue, context, and so on.)
From what I've found so far, it seems that a lot of them will be repeating code, and using a lot of code to accomplish minor things.
Some things that have stood out when working on the example module:
- you need to subclass EntityAccessController when all you probably want to say is that you have a permission 'administer foobar config' for all operations on the entities
- you need to subclass ConfigEntityListController even if you only want a list of entities that has the label and status.
Both of these could be eased by the addition of controller classes aimed at this use case.
Related
#2089767: Provide a ConfigDeleteFormBase to cover common functionality of config entity delete forms
#2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation
Comments
Comment #1
andypostAlso we need to figure out better dx for sortable entities #2074875: Reload entities in DraggableListController::submitForm()
Comment #2
joachim CreditAttribution: joachim commented> - you need to subclass ConfigEntityListController even if you only want a list of entities that has the label and status.
This one should be easy.
> - you need to subclass EntityAccessController when all you probably want to say is that you have a permission 'administer foobar config' for all operations on the entities
This one needs some figuring out.
We need somewhere for an entity type to tell the default controller class what permission to use.
Comment #3
joachim CreditAttribution: joachim commentedHere's a patch to get the ball rolling.
What is the convention for documenting extra annotation properties that new components invent?
Comment #4
joachim CreditAttribution: joachim commentedIRC review:
14:52alexpott:
joachim: so rather than DefaultConfigEntityAccessController we'd probably call it ConfigEntityAccessControllerBase
14:52joachim:
fair enough. I made the name up :)
14:53alexpott:
joachim: instead of user_access() you can use the hasPermission method on $account
Leaving as needs review. Would be good to get opinions on my adding a new property to the entity type -- no sense in rerolling until that's been discussed.
Comment #5
joachim CreditAttribution: joachim commentedFurther discussion:
15:05alexpott:
joachim: config entity already dumps undefined keys into it such as config_prefix
So that's okay :)
And also we should handle the case that the permission is not defined in the entity info. I think the best thing here is to throw an exception. This controller requires that property -- good DX is to let the developer know that they're not using it properly.
All that remains is bikeshedding the 'access_controller_permission' name :D
I'll reroll later today.
Comment #6
xjmComment #7
joachim CreditAttribution: joachim commentedUpdated patch with all of the above changes.
Comment #8
joachim CreditAttribution: joachim commentedDefinitely helps if I rename the class files as well as the classes!!!
Comment #9
joachim CreditAttribution: joachim commented... and namespace the exception.
Comment #10
tim.plunkettThis actually seems slightly useful.
We already have ConfigEntityListController.
These should call parent.
No config entities outside of our testing one have a UI simple enough that they only want a label. I see no use case for this, and would suggest removing it and focusing on the access controller.
Comment #11
larowlanThis looks great, thanks for working on tests and more enhancements
Comment #12
andypostAnd also need to check
weightKey
and sortability of the list controllerComment #13
joachim CreditAttribution: joachim commented> No config entities outside of our testing one have a UI simple enough that they only want a label
In core. We don't know what contrib will do. Surely providing a quick class like this is worth the improvement in DX against the cost of it potentially being useless.
The bottom line is that having this would smooth over a rough bit of DX. Even if you think it's a bit pointless.
Because without it, it's one more class you have to create (and if you're starting out, *figure out* how to create) before your code does something that is recognizably useful. That's extra hurdles. That's extra points at which developers give up and turn away and think 'D8 is crazy'. This is part of what DX is about.
Comment #14
larowlanRan out of time to post my patch today but brings it down to three classes and no routing.
Two classes if the lits controller argument can be made.
Comment #15
larowlanSplit part of my patch into #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation
There's another one coming for delete forms
Comment #16
larowlanRelated #2089767: Provide a ConfigDeleteFormBase to cover common functionality of config entity delete forms
Comment #17
tim.plunkettPlease open a separate issue for the access controller suggestion.
If you really think this will be useful to someone, then it must be because they plan on providing no other functionality. Therefore it should be final.
If they actually want a custom list controller they can extend the base like everyone else.
Comment #18
larowlanComment #19
joachim CreditAttribution: joachim commented> If you really think this will be useful to someone, then it must be because they plan on providing no other functionality. Therefore it should be final.
Well, sort of.
I think this is useful because it removes one of the several classes that you have to make *before* you can have a working config entity. It's fewer bits of code to create before the developer can try enabling the code they have so far, and see something actually working. Getting feedback from your code is very important in developing.
Even if any developer who uses this then replaces it only an hour later with their own list controller which does custom stuff, this class will have improved DX. It provides a stepping stone in the leap from nothing to the bunch of classes you need.
Comment #20
larowlancross-post
Fwiw I think the use case for the list controller is minimal.
We already have:
EntityListController and ConfigEntityListController adding another seems overkill.
EntityListController by default provides an operations column. That serves the purpose you've described ie to see something working.
But the access controller is a definite win and dovetails nicely with #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation although I called it admin_permission there, but same result.
Comment #21
joachim CreditAttribution: joachim commented> EntityListController by default provides an operations column. That serves the purpose you've described ie to see something working.
After a fashion.
You look at your config entities list and go 'Huh? Only operation links? What's wrong there then? Oh crap, I need ANOTHER controller class???' etc etc.
Comment #22
BerdirAlso not sure I get the final there. Quite a few config entities use the draggable list controller. If this is in a separate final class, then this is useless for them.
See https://github.com/Berdir/tmgmt/blob/8.x-1.x/lib/Drupal/tmgmt/Entity/Con..., for example. Not quite sure what to do with the form id and my different label, I'd be more than fine with a generated form id there as a default implementation.
Comment #23
tim.plunkettThis entire class is useless if you want to customize anything.
Comment #24
tim.plunkettLet me clarify #23.
If you were to use this proposed class, and then later you wanted to add a column in between Label and Operations (like almost every single list controller in core does).
Using the CategoryListController as an example
You would be forced to use this code
But with the current class in core, you just have
In order to save people from that, or worse,
$row['label'] => $entity->label();
which is a security hole, or wondering why the hell list controllers are so hard, we provide one they can safely extend.Comment #24.0
tim.plunkettUpdated issue summary.
Comment #25
tim.plunkettSee #23/24