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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Also we need to figure out better dx for sortable entities #2074875: Reload entities in DraggableListController::submitForm()

joachim’s picture

> - 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.

joachim’s picture

Status: Active » Needs review
FileSize
2.75 KB

Here's a patch to get the ball rolling.

What is the convention for documenting extra annotation properties that new components invent?

joachim’s picture

IRC 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.

joachim’s picture

Further 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.

xjm’s picture

Issue tags: +Configuration system
joachim’s picture

Updated patch with all of the above changes.

joachim’s picture

Definitely helps if I rename the class files as well as the classes!!!

joachim’s picture

... and namespace the exception.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ConfigEntityAccessControllerBase.php
    @@ -0,0 +1,42 @@
    +    if (!isset($entity_info['access_controller_permission'])) {
    

    This actually seems slightly useful.

  2. +++ b/core/lib/Drupal/Core/Entity/ConfigEntityListControllerBase.php
    @@ -0,0 +1,41 @@
    +class ConfigEntityListControllerBase extends EntityListController implements EntityListControllerInterface, EntityControllerInterface {
    

    We already have ConfigEntityListController.

  3. +++ b/core/lib/Drupal/Core/Entity/ConfigEntityListControllerBase.php
    @@ -0,0 +1,41 @@
    +  public function buildHeader() {
    ...
    +    $header['operations'] = t('Operations');
    ...
    +  public function buildRow(EntityInterface $entity) {
    ...
    +    $row['operations']['data'] = $this->buildOperations($entity);
    

    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.

larowlan’s picture

This looks great, thanks for working on tests and more enhancements

andypost’s picture

And also need to check weightKey and sortability of the list controller

joachim’s picture

> 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.

larowlan’s picture

Ran 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.

larowlan’s picture

larowlan’s picture

tim.plunkett’s picture

Title: provide controller classes to simplify creation of config entities » Provide a simple list controller for config entities
FileSize
1.1 KB

Please 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.

larowlan’s picture

Title: Provide a simple list controller for config entities » provide controller classes to simplify creation of config entities
Issue tags: +Prague DX smackdown
joachim’s picture

> 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.

larowlan’s picture

Title: provide controller classes to simplify creation of config entities » Provide a simple list controller for config entities

cross-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.

joachim’s picture

> 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.

Berdir’s picture

Also 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.

tim.plunkett’s picture

This entire class is useless if you want to customize anything.

tim.plunkett’s picture

Let 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

public function buildHeader() {
  $header = parent::buildHeader();
  $header = array_slice($row, 0, 2, TRUE) + array(
    'default' => t('Default'),
    'recipients' => t('Recipients'),
  ) + array_slice($header, 0, NULL, TRUE);
  return $header;
}

But with the current class in core, you just have

public function buildHeader() {
  $header['category'] = t('Category');
  $header['recipients'] = t('Recipients');
  $header['selected'] = t('Selected');
  return $header + parent::buildHeader();
}

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.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Issue summary: View changes
Status: Needs review » Closed (works as designed)

See #23/24