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

Files: 
CommentFileSizeAuthor
#14 2091871-14.patch16.41 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,674 pass(es), 108 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt2.68 KBandypost
#10 2091871-config-forms2-10.patch14.4 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,242 pass(es), 131 fail(s), and 24 exception(s).
[ View ]
#8 2091871-config-forms-8.patch22.65 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,416 pass(es), 108 fail(s), and 3 exception(s).
[ View ]

Comments

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

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

EDIT
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

StatusFileSize
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
StatusFileSize
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::getEntityTypeFromStaticClass()
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)
Drupal\Core\Controller\FormController->getContentResult(Symfony\Component\HttpFoundation\Request)
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)
Drupal\Core\DrupalKernel->handle(Symfony\Component\HttpFoundation\Request)

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