Updated: Comment #0


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

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.

#28 2091871-exists-28.patch23.44 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,974 pass(es).
[ View ]
#28 interdiff.txt13.96 KBandypost
#26 2091871-exists-26.patch22.47 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,974 pass(es).
[ View ]


Berdir’s picture

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 :)

tstoeckler’s picture

Status:Active» Needs review
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.

Berdir’s picture

  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.

tstoeckler’s picture

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.

jibran’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

andypost’s picture

Issue summary:View changes
Status:Needs work» Needs review
new22.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,416 pass(es), 108 fail(s), and 3 exception(s).
[ View ]

Re-roll, suppose there could be different approach that used in core now

-        'exists' => '\Drupal\responsive_image\Entity\ResponsiveImageMapping::load',
+        'exists' => array(get_class($this), 'exists'),

only DateFormatFormBase needs #field_prefix

Only ShortcutSet is left

core/modules/action/src/ActionFormBase.php:81:        'exists' => array(get_class($this), 'exists'),
core/modules/block/src/BlockForm.php:76:        'exists' => array(get_class($this), 'exists'),
core/modules/block_content/src/BlockContentTypeForm.php:39:        'exists' => array(get_class($this), 'exists'),
core/modules/comment/src/CommentTypeForm.php:78:        'exists' => array(get_class($this), 'exists'),
core/modules/config/tests/config_test/src/ConfigTestForm.php:36:        'exists' => array(get_class($this), 'exists'),
core/modules/contact/src/CategoryForm.php:41:        'exists' => array(get_class($this), 'exists'),
core/modules/entity/src/Form/EntityDisplayModeFormBase.php:91:        'exists' => array(get_class($this), 'exists'),
core/modules/field_ui/src/FieldOverview.php:194:            'exists' => array($this, 'fieldNameExists'),
core/modules/filter/src/FilterFormatFormBase.php:70:        'exists' => array(get_class($this), 'exists'),
core/modules/image/src/Form/ImageStyleFormBase.php:67:        'exists' => array(get_class($this), 'exists'),
core/modules/menu_ui/src/MenuForm.php:118:        'exists' => array($this, 'menuNameExists'),
core/modules/node/src/NodeTypeForm.php:53:        'exists' => array(get_class($this), 'exists'),
core/modules/responsive_image/src/ResponsiveImageMappingForm.php:55:        'exists' => array(get_class($this), 'exists'),
core/modules/search/src/Form/SearchPageFormBase.php:106:        'exists' => array(get_class($this), 'exists'),
core/modules/shortcut/src/Form/SwitchShortcutSet.php:116:          'exists' => array($this, 'exists'),
core/modules/shortcut/src/ShortcutSetForm.php:35:        'exists' => '\Drupal\shortcut\Entity\ShortcutSet::load',
core/modules/simpletest/tests/src/WebTestBaseTest.php:105:        'exists',
core/modules/simpletest/tests/src/WebTestBaseTest.php:112:        'exists',
core/modules/system/src/Form/DateFormatFormBase.php:106:        'exists' => array(get_class($this), 'exists'),
core/modules/taxonomy/src/VocabularyForm.php:44:        'exists' => array(get_class($this), 'exists'),
core/modules/user/src/RoleForm.php:40:        'exists' => array(get_class($this), 'exists'),
core/modules/views_ui/src/ViewAddForm.php:80:        'exists' => '\Drupal\views\Views::getView',
core/modules/views_ui/src/ViewDuplicateForm.php:44:        'exists' => '\Drupal\views\Views::getView',
andypost’s picture

Title:Add an ConfigEntityFormController with an exists() method» Add an ConfigEntityForm with an exists() method
Issue summary:View changes
andypost’s picture

new14.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,242 pass(es), 131 fail(s), and 24 exception(s).
[ View ]

Another approach

The last submitted patch, 8: 2091871-config-forms-8.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 10: 2091871-config-forms2-10.patch, failed testing.

andypost’s picture

static can't be used in base classes, needs work

andypost’s picture

Status:Needs work» Needs review
new2.68 KB
new16.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,674 pass(es), 108 fail(s), and 0 exception(s).
[ View ]

Fixed Imagestyle test, config tests are failed because class ConfigQueryTest extends ConfigTest

Drupal\Core\Entity\Exception\AmbiguousEntityClassException: Multiple subclasses provide an entity type for Drupal\config_test\Entity\ConfigTest. in Drupal\Core\Entity\Entity::getEntityTypeFromStaticClass() (line 474 of core/lib/Drupal/Core/Entity/Entity.php).
Drupal\Core\Entity\Entity::load(antelope, [Array], Drupal\Core\Form\FormState)
call_user_func(Drupal\config_test\Entity\ConfigTest::load, antelope, [Array], Drupal\Core\Form\FormState)
form_validate_machine_name([Array], Drupal\Core\Form\FormState, [Array])
call_user_func_array(form_validate_machine_name, [Array])
Drupal\Core\Form\FormValidator->doValidateForm([Array], Drupal\Core\Form\FormState)
Drupal\Core\Form\FormValidator->doValidateForm([Array], Drupal\Core\Form\FormState, config_test_form)
Drupal\Core\Form\FormValidator->validateForm(config_test_form, [Array], Drupal\Core\Form\FormState)
Drupal\Core\Form\FormBuilder->processForm(config_test_form, [Array], Drupal\Core\Form\FormState)
Drupal\Core\Form\FormBuilder->buildForm(Drupal\config_test\ConfigTestForm, Drupal\Core\Form\FormState)
call_user_func_array([Array], [Array])
Drupal\Core\Controller\HtmlPageController->getContentResult(Symfony\Component\HttpFoundation\Request, [Array])
Drupal\Core\Controller\HtmlPageController->content(Symfony\Component\HttpFoundation\Request, [Array])
call_user_func_array([Array], [Array])
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Symfony\Component\HttpFoundation\Request, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)

Status:Needs review» Needs work

The last submitted patch, 14: 2091871-14.patch, failed testing.

Status:Needs work» Needs review

Berdir queued 14: 2091871-14.patch for re-testing.

Berdir’s picture

Status:Needs review» Needs work

The last submitted patch, 14: 2091871-14.patch, failed testing.

andypost’s picture

no, that is not fixed... and it looks strange that it hard to inherit entity classes...
It looks that neither #8 no #10 works

andypost’s picture

Issue tags:+API addition, +Needs reroll

This still needs work http://cgit.drupalcode.org/drupal/tree/core/modules/action/src/ActionFor...
Otoh this could be done latter

andypost’s picture

Status:Needs work» Needs review
Issue tags:-API addition, -Needs reroll
new21.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,626 pass(es), 152 fail(s), and 20 exception(s).
[ View ]

Another approach, the entity class could be overriden in contrib so forms shouldl use actual class name to load entity

andypost’s picture

As patch in #2465751: Remove config_test_load() and replace with entity_load shows there's still confusion which entity class to load

Status:Needs review» Needs work

The last submitted patch, 21: 2091871-exists-21.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:-D8MI+Configurables, +Configuration system
Related issues:+#2465751: Remove config_test_load() and replace with entity_load
new584 bytes
new21.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,897 pass(es), 74 fail(s), and 0 exception(s).
[ View ]

Fix image styles

Status:Needs review» Needs work

The last submitted patch, 24: 2091871-exists-24.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+Needs issue summary update
new1.51 KB
new22.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,974 pass(es).
[ View ]

Trying the fix from related issue, seems better to implement the exists() method

tim.plunkett’s picture

  1. +++ b/core/modules/field_ui/src/Form/EntityDisplayModeFormBase.php
    @@ -103,19 +103,20 @@ public function form(array $form, FormStateInterface $form_state) {
    -    return (bool) $this->queryFactory
    -      ->get($this->entity->getEntityTypeId())
    -      ->condition('id', $element['#field_prefix'] . $entity_id)
    -      ->execute();
    +    /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $entity */
    +    $entity = $form_state->getFormObject()->getEntity();
    +    return (bool) $entity::load($element['#field_prefix'] . $entity_id);

    How is this an improvement? Seems lazier.

  2. +++ b/core/modules/filter/src/FilterFormatFormBase.php
    @@ -186,22 +186,6 @@ public function form(array $form, FormStateInterface $form_state) {
    -  public function exists($format_id) {
    -    return (bool) $this->queryFactory
    -      ->get('filter_format')
    -      ->condition('format', $format_id)
    -      ->execute();



andypost’s picture

Category:Task» Bug report
new13.96 KB
new23.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,974 pass(es).
[ View ]

So here's a more simplified version.
And I think better to add this method to EntityFormInterface so content entity forms could reuse that.

And this is a bug, because common forms use static load() method for this check.
#2465751: Remove config_test_load() and replace with entity_load shows that this approach is not compatible when entities are extended with hook_entity_type_alter() and while core supports this better to use common method.

Also @berdir mention about performance (for config entities)

berdir> andypost: I'm not sure. the entity query approach is slower I think, because we need to list all IDs, but we are working on optimizations for that. So, I don't care much about which approach we chose

andypost’s picture