Updated: Comment #0

Problem/Motivation

(Almost?) All config entities have a machine name form field with an 'exists' callback that checks for an existing entity of that type with a given name. Each config entity has to implement such a callback even though it can easily be done in a generic fashion. What's worse: Even though the best practice is to perform these 'exists' check with an entity query many config entities simply defer this to the 'foo_load()' function which performs completely unnecessary loading. This already happens several times in core and will be even more widespread in contrib, I fear.

Proposed resolution

It seems this is a great use-case for a generic exists() method on the form controller. This only concerns config entities, though, so we need to create a new ConfigEntityFormController.

Remaining tasks

  • Agree that this makes sense
  • Write patch
  • Review patch
  • Commit patch

API changes

No BC-breaks. There's a new ConfigEntityFormController that it's a best practice to extend.

Files: 
CommentFileSizeAuthor
#2 2091871-config-form-controller-exists-2.patch26.48 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2091871-config-form-controller-exists-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Component:entity system» configuration entity system
Issue tags:+D8MI, +Entity Field API

There are other things to consider for config entities. One is how they should handle config context, should they also enforce the context, like ConfigFormBase does?

If we need to do this, then it would be a very strong recommendation to use this as a base case.

Then there's the set/get topic, see #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface. And having one could rely on getExportProperties() to only set known properties on config entities, similar to what the content entity form controllers does.

Tagging to get Gabor in here, let's see if it works :)

Status:Active» Needs review
StatusFileSize
new26.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2091871-config-form-controller-exists-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Unless I'm mistaken #1 is an endorsement of this, right? :-)

Let's see how this works.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityFormController.php
    @@ -0,0 +1,65 @@
    + * Contains \Drupal\Core\Config\Entity\ConfigEntityFormController.
    + */
    + ¶
    +namespace Drupal\Core\Config\Entity;

    An trailing space!!!111

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityFormController.php
    @@ -0,0 +1,65 @@
    +    $entity_info = $this->entity->entityInfo();
    +    $id_key = $entity_info['entity_keys']['id'];
    +    return (bool) $this->queryFactory
    +      ->get($this->entity->entityType())
    +      ->condition($id_key, $entity_id)
    +      ->range(0, 1)
    +      ->count()
    +      ->execute();

    Why EFQ and not just load it? EFQ on config entities is quite slow, because it has to load all of them to process them.

  3. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -21,7 +21,7 @@
    - * Defines an interface which generates a link with route names and parameters.
    + * A Generator creates URL strings based on a specified route.
    +++ b/core/lib/Drupal/Core/Utility/LinkGeneratorInterface.php
    @@ -8,7 +8,7 @@
    - * Defines an interface for a service which generates a link out of a
    + * Defines an interface which generates a link with route names and parameters.

    Unrelated changes?

Status:Needs review» Needs work

The last submitted patch, 2091871-config-form-controller-exists-2.patch, failed testing.

1. PhpStorm--

2. It was said in other issues that this is a performance benefit over loading, as loading is conceptually unnecessary. I totally forgot that EFQ obviously needs to load for config entities. So I guess that should be changed.

3. Yes, sorry. That is a different patch I accidentally included.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 2: 2091871-config-form-controller-exists-2.patch, failed testing.