Add config validation for plugin IDs

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PluginIdConstraintValidator.php
@@ -0,0 +1,30 @@
+    if (!($plugin_manager instanceof PluginManagerInterface)) {
+      throw new ConstraintDefinitionException('The given constraint contained an invalid plugin manager.');
+    }

This looks untested.

+++ b/core/modules/config/tests/config_test/src/ConfigValidation.php
@@ -74,7 +74,7 @@ public static function validateGiraffes($string, ExecutionContextInterface $cont
-    if ($diff = array_diff(array_keys($mapping), ['llama', 'cat', 'giraffe', 'uuid', '_core'])) {
+    if ($diff = array_diff(array_keys($mapping), ['llama', 'cat', 'giraffe', 'uuid', 'plugin_id', '_core'])) {

This needs an issue to make it more sensible. Created #2925689: ConfigValidation class contains code that is brittle and changing for every addition

+++ b/core/modules/workflows/config/schema/workflows.schema.yml
@@ -11,6 +11,9 @@ workflows.workflow.*:
+      constraints:
+        PluginId:
+          pluginManager: 'plugin.manager.workflows.type'

Can we define a plugin_id type somewhere in core and add validate it, if it defines the plugin manager?

Do we need an upgrade path for all the schema changes?

@jibran How would you upgrade something like this? What we need a change record explaining what module authors can do now.

#10: I also don't see a way to update schema's, a change record should be sufficient.

#6 also still needs to be fixed.

1. +++ b/core/tests/Drupal/KernelTests/Core/Validation/ConstraintsTest.php
   @@ -21,25 +22,53 @@ class ConstraintsTest extends KernelTestBase {
   +  public function testValidation($test_key, $value, $violatioin_count) {
   

   $violation_count?

Reroll of #5, fixed my own nitpick of #12.

#3324150: Add validation constraints to config_entity.dependencies introduced the ExtensionExists, ConfigExists and RequiredConfigDependencies validation constraints.

So I propose that we rename this

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PluginIdConstraint.php
@@ -0,0 +1,20 @@
+ * @Constraint(
+ *   id = "PluginId",
+ *   label = @Translation("Plugin ID", context = "Validation"),
+ * )
+ */
+class PluginIdConstraint extends Constraint {
+
+  public $message = 'The plugin ID %value does not exist.';

to PluginExists.

Even more so because just PluginId implies it's just a string sequence that would be a valid plugin ID, not that it is a valid reference to a plugin ID, i.e. to one that is actually available.

And … as it so happens, that's exactly what we've been doing over at #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints! @phenaproxima contributed a more extensive and flexible version of what's in this patch at https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs?co.... Crediting him for that.

Heck, that even included indirect test coverage already!

Let's add explicit test coverage and add that commit of @phenaproxima's to core in this issue? 🤞

P.S.: changing parent, because this validation constraint would be helpful not only for config entities, but also config in general.

Here's the PluginExists constraint from that issue, with explicit test coverage.

Woah, awesome, thank you so much for that new explicit test, @phenaproxima! Read the code and test coverage in detail, and there's not a single thing I can complain about 🫣🤓

I only have two nits, both of which could easily be addressed on commit — or not at all, depending on what the committer thinks.

1. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
   @@ -0,0 +1,61 @@
   +   * The name of the plugin manager service.
   

   s/name/service ID/

2. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
   @@ -0,0 +1,61 @@
   +   * Optional name of the interface that the plugin must implement.
   

   s/name/FQCN/

This now just needs a change record just like #3324150: Add validation constraints to config_entity.dependencies got one. Keeping at Needs review until that's there, then this is RTBC 🚀🤩

Draft CR added: https://www.drupal.org/node/3330852

I ran the test from #3324984-12: Create test that reports % of config entity types (and config schema types) that is validatable in order to get a list of which parts of core's config schema should get the PluginExists constraint added (in a follow-up, to be sure). Here's what I came up with:

core.entity_form_display.*.*.*:targetEntityType
core.entity_form_display.*.*.*:content.*.type
core.entity_form_mode.*.*.*:targetEntityType
core.entity_view_display.*.*.*:targetEntityType
core.entity_view_display.*.*.*:content.*.type
core.entity_view_mode.*.*.*:targetEntityType
field_config_base:type
field.storage.*.*:type
image.style.*:effects.*.id
language.content_settings.*.*:target_entity_type_id
media.type.*:source
rest.resource.*:plugin_id
search.page.*:plugin
system.action.*:plugin
tour.tour.*:tips.*.plugin
views.view.*:display.*.display_plugin
views.view.*:display.*.display_options.pager.type
views.view.*:display.*.display_options.exposed_form.type
views.view.*:display.*.display_options.access.type
views.view.*:display.*.display_options.cache.type
views.view.*:display.*.display_options.empty.*.plugin_id
views.view.*:display.*.display_options.sorts.*.plugin_id
views.view.*:display.*.display_options.arguments.*.plugin_id
views.view.*:display.*.display_options.filters.*.plugin_id
views.view.*:display.*.display_options.style.type
views.view.*:display.*.display_options.row.type
views.view.*:display.*.display_options.query.type
views.view.*:display.*.display_options.fields.*.plugin_id
views.view.*:display.*.display_options.relationships.*.plugin_id
views.view.*:display.*.display_options.header.*.plugin_id
views.view.*:display.*.display_options.footer.*.plugin_id
workflows.workflow.*.:type

EDIT: Removed block.block.*:plugin and editor.editor.*:plugin because they are covered in subsequent patches.

Thanks for the analysis in #28!

That reminds me of something actually … we should actually be adding a new type: plugin config schema type, and actually use that in all of the places you found in #28. But that'd result in a huge scope. 🤔

How about we:

1. Add type: plugin_id
2. Use it in a single place. Which one? The one that is closest to 100% validatable but isn't yet: editor.editor.*:editor (see #3324984-3: Create test that reports % of config entity types (and config schema types) that is validatable: 🔵 78% editor.editor.*). The beauty there is that you can update the focused Kernel test you added recently (\Drupal\Tests\editor\Kernel\EditorValidationTest) to prove it works with a valid @Editor plugin but not with an invalid one. And then we'll actually know it works in reality because the CKEditor 5 test coverage is very exacting! (See \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::test(), where Editor config entities are explicitly tested for expected validation errors too.)

The change record looks solid 👍 Just added a config schema-based example 😊

I spoke too soon in #29.1:

1. Add type: plugin_id

→ I was thinking something like:

diff --git a/core/config/schema/core.data_types.schema.yml b/core/config/schema/core.data_types.schema.yml
index cfb4a0c6bd..17e32a157a 100644
--- a/core/config/schema/core.data_types.schema.yml
+++ b/core/config/schema/core.data_types.schema.yml
@@ -109,6 +109,14 @@ color_hex:
   type: string
   label: 'Color'
 
+# String containing a valid plugin ID that exists in the provided plugin manager.
+plugin_id:
+  type: string
+  label: 'Plugin ID'
+  constraints:
+    PluginExists:
+      manager: ~
+
 # Complex extended data types:
 
 # Root of a configuration object.

… i.e. just like type: label, type: uuid and type: path are examples of subtypes of type: string, I was proposing type: plugin_id. The problem with that is that all of the types I just mentioned do not have different subsets — there aren't different "buckets" of UUIDs or anything like that.

So — consider #29.1 out of scope. I do think #29.2 is still very valuable though. Do you agree? I can see the case for just committing this as-is, but I think it's stronger with at least one concrete use.

I agree.

This patch adds plugin existence validation for editors.

Huh. I realized that block entities don't have a validation test yet 😳

So I added one (which also tests that the block's plugin ID is validated).

A little bit of cleanup so we're not relying on the internals of action_test as much.

Looks magnificent! 🤩

+++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
@@ -0,0 +1,71 @@
+    if ($constraint->interface) {

I have one tiny remark, but it is mostly preference.

To decrease complexity, we should probably reverse the first if statement here.

if (!$constraint->interface) {
  return;
}

if ($definition instanceof PluginDefinitionInterface) {
...

Since this is just preference, and doesn't actually matter at all, I'll leave this at RTBC. Really good to see that we also got at least a little bit of block entities test coverage as well.

Really good to see that we also got at least a little bit of block entities test coverage as well.

It's inheriting quite a bit of coverage from the base class, too.

Quick review

1. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
   @@ -0,0 +1,61 @@
   +   * The name of the plugin manager service.
   

   is it a name or an ID?

2. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,71 @@
   +  public static function create(ContainerInterface $container) {
   

   should we add the : static return type here

3. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,71 @@
   +    $manager = $this->container->get($constraint->manager);
   +    assert($manager instanceof PluginManagerInterface);
   

   Should we catch \Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException here

4. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,71 @@
   +    $definition = $manager->getDefinition($plugin_id, FALSE);
   

   note to self/other reviewers - no need to catch \Drupal\Component\Plugin\Exception\PluginNotFoundException here because the second argument is FALSE

5. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,71 @@
   +    if ($constraint->interface) {
   

   Yes, I agree with @borisson_ we should flip the logic and return early here to reduce complexity

6. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,71 @@
   +      if ($definition instanceof PluginDefinitionInterface) {
   +        $class = $definition->getClass();
   +      }
   +      elseif (is_array($definition)) {
   +        assert(array_key_exists('class', $definition));
   +        $class = $definition['class'];
   +      }
   +      else {
   +        throw new UnexpectedTypeException($definition, 'array|' . PluginDefinitionInterface::class);
   +      }
   

   in the other issue I reviewed and commented that I personally prefer to refactor away else/elseif - should we have a protected getClass method here that had early returns (and no else/elseif)

7. +++ b/core/modules/block/tests/src/Kernel/BlockValidationTest.php
   @@ -0,0 +1,42 @@
   +    $this->entity->set('plugin', 'non_existent');
   +    $this->assertValidationErrors(["The 'non_existent' plugin does not exist."]);
   

   non_existent is a tame plugin ID for a test @phenaproxima, I was expecting something more humorous 😂

8. +++ b/core/tests/Drupal/KernelTests/Core/Plugin/PluginExistsConstraintValidatorTest.php
   @@ -0,0 +1,128 @@
   +    $this->assertSame("The 'action_test_save_entity' plugin must implement or extend " . MenuInterface::class . '.', (string) $violations->get(0)->getMessage());
   

   personal preference, but sprintf would make this easier to read, feel free to ignore

1. @Wim Leers had the same point, so changed.
2. Done.
3. I don't think so. If the constraint is providing an invalid service ID, that is a straight-up logic error that should immediately break execution.
4. 👍
5. Done.
6. Done. I agree this is simpler, and the fact that I can use a union type, in combination with strict_types, means I can remove the relevant test coverage.
7. Believe me, I wanted to. But the stupid spell checker too often gets in the way of that kind of thing. cspell killed the fun of writing core tests. 😭
8. Decided to ignore this. :)

That failure doesn't look related.

Refactored to use the class resolver, rather than ContainerAwareTrait and ContainerAwareInterface, because it's best to keep the number of container-referencing objects in memory as low as possible.

Crediting Wim and Lee for review.

🤔 That failure didn't look related either.

Back to rtbc, the points in #37 all seem very valid and they have been fixed. I also agree that the refactor to class resolver is a good thing +1.

Refactored to use the class resolver, rather than ContainerAwareTrait and ContainerAwareInterface, because it's best to keep the number of container-referencing objects in memory as low as possible.

I'm not sure I agree here. The problem is that the class resolver can do way more that resolve a service ID. However all of this is not necessary. See \Drupal\Core\Validation\ConstraintFactory::createInstance() - your constraint can inject a plugin manager instance into the constraint :)

#37.3++ 😆

#46: ooh, nice! So don't use ContainerAware* nor ClassResolver, but ContainerFactoryPluginInterface 👍

@phenaproxima reached out to me to explain #46. Here's what I mean in patch form.

+++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
@@ -0,0 +1,81 @@
+   * @phpstan-ignore-next-line
+   */
+  public readonly PluginManagerInterface $pluginManager;

This is because of a PHPStan bug not correctly knowing that you can assign a read only property in a static method after a new static().

Here's a maybe slightly better way of doing the injection. It's tricky because we have to kinda normalise the options - but I think it's okay. Like a manager is required and the this will error nicely.

Improved the exception from create to be the same as if we'd called normalizeOptions(). In writing a unit test for that I got interesting deprecation messages from the debug class loader about needing return types on \Symfony\Component\Validator\Constraint::getRequiredOptions and \Symfony\Component\Validator\Constraint::getDefaultOption - so added them.

+++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
@@ -0,0 +1,92 @@
+  /**
+   * The ID of the plugin manager service.
+   *
+   * @var string
+   */
+  protected string $manager;

There's no need to expose this outside the constraint now so I removed the public. We have the plugin manager for that.

1. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
   @@ -68,20 +69,23 @@ public function __construct(public readonly PluginManagerInterface $pluginManage
   +    if (!isset($configuration['manager']) && !isset($configuration['value'])) {
   

   Pro tip: isset() is variadic and only returns TRUE if all the arguments are set. So this can be if (!isset($configuration['manager'], $configuration['value'])).

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/PluginExistsConstraintTest.php
   @@ -0,0 +1,51 @@
   +   * @testWith ["value"]
   +   *           ["manager"]
   

   🤯 TIL

@phenaproxima re #53.1 - that doesn't work. The situation we have hear is that either value or manager must be set. If I make your change then \Drupal\Tests\Core\Plugin\PluginExistsConstraintTest fails as I would expect.

Yeah, you're right. My bad!

This formulation is perhaps more readable.

+++ b/core/tests/Drupal/Tests/Core/Plugin/PluginExistsConstraintTest.php
@@ -0,0 +1,51 @@
+   * @testWith ["value"]
+   *           ["manager"]

🤩 First @testWith in core! 👍

+++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
@@ -0,0 +1,93 @@
+    if ($plugin_manager_id === NULL) {
+      throw new MissingOptionsException(sprintf('The option "manager" must be set for constraint "%s".', static::class), ['manager']);
+    }
...
+  public function getRequiredOptions(): array {
+    return ['manager'];
+  }

🤔 Why do we need to throw this explicitly if ::getRequiredOptions() already exists?

Is this a consequence of relying on \Drupal\Core\Validation\ConstraintFactory to invoke ::create() on our constraint since #50? Why doesn't \Symfony\Component\Validator\Constraint::normalizeOptions() handle this for us?

(I'm not really opposed, but am kinda concerned: we're about to add many validation constraints, and if we introduce this pattern, it will be copied. So I want to make sure we really want to go this direction.)

Isn't the approach in #48 preferable because it requires no duplication?

Isn't the approach in #48 preferable because it requires no duplication?

In general I would agree that #48 is probably preferable, for the same reasons: it requires no duplication or specialized unit testing, which -- if Wim is correct that this pattern will be copied -- is prone to mistakes.

This being said, I'm not strongly opposed to #56, if it's what we really want. But at least the concerns have been aired.

The problem with #48 is that then you can only create PluginExistsConstraint via the ::create method because it is the only way to set the readonly $pluginManager property. In general I think setting properties via the constructor is preferable. This is also what PHPStan expects. I think that the use-case of a dynamically selecting the service based on constraint configuration is not at all going to be the norm so I don't think the solution in #56 is fine. Like if the service was not dynamic we'd just be injecting it into the constraint validator. This is a special case.

There is another option here. We could derive all the constraints from the list of plugin managers. And then the manager ID would be part of the plugin definition. And we'd have constraints like PluginExists.plugin.manager.block - or something like that. We need to do something like \Drupal\Core\Plugin\PluginManagerPass to discover all the plugin managers - we could even use that container pass to build the list.

This is getting more and more complicated. Would it maybe make sense to just go back to the ContainerAwareInterface implementation in #38?

Let me be clear that I'm not gonna block this patch if people who know more than me choose to go with #56 or a variation thereof. It's complicated, though, and not a pattern previously seen in core, so maybe we should consider making it explicitly final and/or internal, to underline that this is, in fact, a special case.

so maybe we should consider making it explicitly final and/or internal, to underline that this is, in fact, a special case.

I don't think that's necessary, because subclassing validation constraints is very unlikely to be helpful 😅

I think that the use-case of a dynamically selecting the service based on constraint configuration is not at all going to be the norm so I don't think the solution in #56 is fine.

Fair! 👍

We could derive all the constraints from the list of plugin managers. And then the manager ID would be part of the plugin definition. And we'd have constraints like PluginExists.plugin.manager.block

🫣 This seems excessive.

Let's go with #56. It's really only a single line of code that I'm complaining about — other than that this looks solid 👍

Added a release note. We're still handwaving a bit how contrib should declare their own dependency.

Sorry, that's the release note for the wrong issue. Carry on.

It still might, like, need one.

Changing to a feature request to clarify.

I don't think this warrants a release note, a change record is sufficient. There's nothing here that would be disruptive for a site-owner etc, just a new API for developers. As such, removing the tag.

1. Done. I agree this is simpler, and the fact that I can use a union type, in combination with strict_types, means I can remove the relevant test coverage.

   Another win for my 'else indicates a chance to refactor/elseif is a code smell' motto :)

2. 
   +++ b/core/tests/Drupal/Tests/Core/Plugin/PluginExistsConstraintTest.php
   @@ -0,0 +1,51 @@
   +   * @testWith ["value"]
   +   *           ["manager"]
   

   neat

3. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
   @@ -0,0 +1,93 @@
   +    return new static($container->get($plugin_manager_id), $configuration);
   

   I flagged this in my review above (#37.3 and @phenaproxima felt it shouldn't be caught, but I'm still not 100% convinced.

   A developer could rely on a service ID and that service may not be around yet/anymore because of a module install/uninstall - which could cause this to bubble up to a WSOD to the site user. If it happens during an install/uninstall the site might be in a bad state.

   But then, now we're doing this in the create, we have no other option than an exception, so its kind of moot. With the previous code-path we could return a validation error.

   So, let's leave it as is for now, and hope we don't get a bug report about it later.

4. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraint.php
   @@ -0,0 +1,93 @@
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   

   We could add a :static return type here, but that feels like something we'd do in bulk across core with a phpstan rule to enforce it rather than a piecemeal approach

5. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,60 @@
   +    assert(array_key_exists('class', $definition));
   

   There's a slight edge-case here where this isn't true, if we were ultra conservative we'd return NULL and fail the validation here, so I went to investigate if it's a safe assumption.

   I found \Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass which seems to indicate it's not - i.e. it has code to handle a missing class.

   So in that scenario here, we'd get a TypeError which would halt execution (WSOD) as we've got strict types on and we're returning a NULL instead of a string.

   So I think we need to allow for that, i.e. make the return type ?string.

   is_a has mixed as it's first argument, so we'd need no other changes https://3v4l.org/dQDrG

6. +++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
   @@ -0,0 +1,60 @@
   +  protected static function getClass(array|PluginDefinitionInterface $definition): string {
   

   getClass is awfully generic, there is an outside chance that Symfony adds a method with a similar name, should we make this getPluginClass just to be safe?

So tl;dr, I think we should be conservative and change the name and return type on getClass

Fine to self-RTBC if you agree and make those changes. Ping me for another review

Turns out that PluginExistsConstraintValidator:: getClass() is actually unnecessary code. The plugin system pretty much ensures that 'class' is set and provides a helper that throws exceptions when it's not. We can use this helper and let it throw exceptions when this info is not present which is consistent with what would happen if you tried to instantiate a plugin with the same ID via its plugin manager. This helper is already used throughout the plugin system to do exactly what we want.

Addressed #68 and self-rtbcing as per that comment.

+++ b/core/lib/Drupal/Core/Plugin/Plugin/Validation/Constraint/PluginExistsConstraintValidator.php
@@ -32,7 +32,7 @@ public function validate(mixed $plugin_id, Constraint $constraint) {
-    if (!is_a(static::getClass($definition), $constraint->interface, TRUE)) {
+    if (!is_a(DefaultFactory::getPluginClass($plugin_id, $definition), $constraint->interface, TRUE)) {

🔥 love it

Committed to 10.1.x and published change record. Nice result folks

Yay! This allowed me to simplify #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints, and the good news is that this indeed shows an increase in the schema validatability in the test coverage I'm proposing for that — see #2164373-37: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … for the numbers.

Furthermore, we should adopt this everywhere where type: string is actually pointing to a plugin. Issue with initial patch: #3332593: Adopt PluginExists validator in relevant places.

This is now helping us at #3361534-15: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate! 🥳