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.

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
FileSize
26.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
FileSize
22.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

FileSize
14.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
FileSize
2.68 KB
16.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

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
FileSize
21.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
FileSize
584 bytes
21.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
FileSize
1.51 KB
22.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();
    

    Same.

Etc...

andypost’s picture

Category: Task » Bug report
FileSize
13.96 KB
23.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

tim.plunkett’s picture

Status: Needs review » Needs work

Can we try and do this in a BC compatible way please? Just add a ConfigEntityForm, don't touch EntityFormInterface.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -409,4 +409,15 @@ public function setEntityManager(EntityManagerInterface $entity_manager) {
+  public static function exists($entity_id, array $element, FormStateInterface $form_state) {

+++ b/core/lib/Drupal/Core/Entity/EntityFormInterface.php
@@ -153,4 +153,19 @@ public function setModuleHandler(ModuleHandlerInterface $module_handler);
+  public static function exists($entity_id, array $element, FormStateInterface $form_state);

Do you mean add new ConfigEntityForm with public method but without interface?

tim.plunkett’s picture

Yes. It's a callback, it only needs to be public because of how PHP works. We don't want code from other places calling it as though it were part of the interface.

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev
Oleksiy’s picture

Status: Needs work » Needs review
FileSize
24.26 KB
28.23 KB
11.76 KB

Made a re-roll of the patch #28. Added new ConfigEntityForm.

andypost’s picture

Thanx, Oleksiy!
The only question for me a need in interface for the method

Status: Needs review » Needs work

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

tim.plunkett’s picture

The last patch is missing the ConfigEntityForm.php file, so I can't review that :)

  1. +++ b/core/modules/action/src/ActionFormBase.php
    @@ -78,7 +50,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/block/src/BlockForm.php
    @@ -135,7 +135,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/block_content/src/BlockContentTypeForm.php
    @@ -45,7 +45,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/comment/src/CommentTypeForm.php
    @@ -87,7 +87,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
    @@ -62,7 +62,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/contact/src/ContactFormEditForm.php
    @@ -76,7 +76,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/field_ui/src/Form/EntityDisplayModeFormBase.php
    @@ -88,7 +88,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/filter/src/FilterFormatFormBase.php
    @@ -67,7 +67,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/image/src/Form/ImageStyleFormBase.php
    @@ -64,7 +36,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/node/src/NodeTypeForm.php
    @@ -83,7 +83,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/responsive_image/src/ResponsiveImageStyleForm.php
    @@ -77,7 +77,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/search/src/Form/SearchPageFormBase.php
    @@ -103,7 +103,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/shortcut/src/ShortcutSetForm.php
    @@ -32,7 +32,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/system/src/Form/DateFormatFormBase.php
    @@ -95,7 +84,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => array(get_class($this), 'exists'),
    
    +++ b/core/modules/taxonomy/src/VocabularyForm.php
    @@ -69,7 +42,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    
    +++ b/core/modules/user/src/RoleForm.php
    @@ -37,7 +37,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    

    Please use static::class, not get_class()

  2. +++ b/core/modules/shortcut/src/Form/SwitchShortcutSet.php
    @@ -106,7 +106,7 @@ public function buildForm(array $form, FormStateInterface $form_state, UserInter
    -          'exists' => array($this, 'exists'),
    +          'exists' => ['\Drupal\shortcut\Entity\ShortcutSet', 'load'],
    
    @@ -142,21 +142,6 @@ public function buildForm(array $form, FormStateInterface $form_state, UserInter
    -  public function exists($id) {
    -    return (bool) $this->shortcutSetStorage->getQuery()
    -      ->condition('id', $id)
    -      ->execute();
    -  }
    

    Every other hunk switches from load to an entity query, why is this doing the opposite?

Oleksiy’s picture

Status: Needs work » Needs review
FileSize
29.91 KB
11.19 KB

Ups... Sorry for that. Have updated the patch according to your comments.

Status: Needs review » Needs work

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

Oleksiy’s picture

Status: Needs work » Needs review
FileSize
28.44 KB
1.98 KB

Updated the path to fix the failed tests. Reverted changes for the Shortcut. But Migrate_drupal.Drupal\migrate_drupal\Tests\d6\EntityContentBaseTest fails even on a clean Drupal installation.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Entity/ConfigEntityForm.php
@@ -0,0 +1,41 @@
+  public static function exists($entity_id, array $element, FormStateInterface $form_state) {
+    /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
+    $entity_type = $form_state->getFormObject()->getEntity()->getEntityType();
+    return (bool) \Drupal::entityQuery($entity_type->id())
+      ->condition($entity_type->getKey('id'), $entity_id)
+      ->execute();
+  }

It seems strange to swapping non static methods with injected dependencies for a static method using \Drupal. What's the rationale for this?

alexpott’s picture

Category: Bug report » Task

And this is a bug, because common forms use static load() method for this check.

Not true... ::load() does this:

    $entity_manager = \Drupal::entityManager();
    return $entity_manager->getStorage($entity_manager->getEntityTypeFromClass(get_called_class()))->load($id);

And ::getEntityTypeFromClass() takes into account alters... So any alters that use a new class will work.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.