Add an ConfigEntityForm with an exists() method

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

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?

2: 2091871-config-form-controller-exists-2.patch queued for re-testing.

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',

Another approach

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

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)

That might have been fixed by #2307681: getEntityTypeFromStaticClass - poor performance.

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

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

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

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

Fix image styles

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

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

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

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

+++ 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?

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.

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

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

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?

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

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.

+++ 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?

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.

rerolled the patch

#52 is totally wrong

Reroll patch #40.

Please ignore #56.

#57 patch applied successfully and test cases passed. Fixed coding standard issues as well.

Patch from #60 could not be applied.

Rerolled patch attached.

Additionnaly removed excess whitespace in DateFormatFormBase.php.

Tested #61 on 9.1, exists() method has been added and white space has also been removed

Re-roll the patch for 9.2

Rerolled patch #65 for 9.5.x.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.