Derivative plugin definitions contain base plugin IDs instead of derivative IDs

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
@@ -73,6 +73,7 @@ public function getDefinition($plugin_id, $exception_on_invalid = TRUE) {
         }
+        $plugin_definition['id'] = $plugin_id;
       }
     }
 
@@ -110,6 +111,7 @@ protected function getDerivatives(array $base_plugin_definitions) {

@@ -110,6 +111,7 @@ protected function getDerivatives(array $base_plugin_definitions) {
           if ($derivative_id && isset($base_plugin_definitions[$plugin_id])) {
             $derivative_definition = $this->mergeDerivativeDefinition($base_plugin_definitions[$plugin_id], $derivative_definition);
           }
+          $derivative_definition['id'] = $plugin_id;
           $plugin_definitions[$plugin_id] = $derivative_definition;

Should we add a short comment here, something like '// Ensure that the plugin id is set to the derivative plugin id'?

Tagging for framework manager review as #2458789: Introduce PluginDefinitionInterface was pushed back.

#2458789: Introduce PluginDefinitionInterface is postponed to 9.x so no need for framework manager review. Let's fix it without #2458789: Introduce PluginDefinitionInterface.

1. +++ b/core/lib/Drupal/Component/Plugin/Definition/MergeablePluginDefinitionInterface.php
   @@ -0,0 +1,39 @@
   +/**
   + * @file
   + * Contains \Drupal\Component\Plugin\Definition\MergeablePluginDefinitionInterface.
   + */
   +
   +namespace Drupal\Component\Plugin\Definition;
   

   @file needs to be removed now.

2. +++ b/core/lib/Drupal/Component/Plugin/Definition/MergeablePluginDefinitionInterface.php
   @@ -0,0 +1,39 @@
   +   * @return $this
   +   */
   +  public function mergeDefaultDefinition(PluginDefinitionInterface $other_definition);
   

   so this doesn't actually return $this, it returns $other_definition, right? I think we use "@return static" (same class, but not this object) in this case? could be wrong.

3. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionInterface.php
   @@ -5,7 +5,7 @@
     *
   - * Object-based plugin definitions MUST implement this interface.
   + * Object-based plugin definitions MUST implement this interface. They can also optionally implement any of the following interfaces.
     *
     * @ingroup Plugin
     */
   

   uh.. what interfaces? :) Also 80 characters..

4. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
   @@ -98,14 +106,17 @@ protected function getDerivatives(array $base_plugin_definitions) {
   -        foreach ($derivative_definitions as $derivative_id => $derivative_definition) {
   +        foreach ($derivative_definitions as $derivative_id => $derivative_plugin_definition) {
   

   is this rename necessary, makes the diff harder to read.

5. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
   @@ -203,35 +214,78 @@ protected function getDeriver($base_plugin_id, $base_definition) {
   +    $plugin_id = NULL;
   +    if ($base_definition instanceof PluginDefinitionInterface) {
   +      if ($base_definition instanceof PluginInspectionDefinitionInterface) {
   +        $plugin_id = $base_definition->getId();
          }
   -      if (!is_subclass_of($class, '\Drupal\Component\Plugin\Derivative\DeriverInterface')) {
   -        throw new InvalidDeriverException(sprintf('Plugin (%s) deriver "%s" must implement \Drupal\Component\Plugin\Derivative\DeriverInterface.', $base_definition['id'], $class));
   +      if ($base_definition instanceof PluginDeriverDefinitionInterface) {
   +        $class = $base_definition->getDeriverClass();
          }
        }
   

   I'm not sure how exactly you use plugin definition objects in plugin.module. you have some kind of decorators, but they will only come in play later, right?

   but afaik at least in core, entity types are still the only example that use objects. Which actually do not support derivers *at all* (":" is not a valid character for an entity type)

   IMHO, it would be acceptable to only solve it here for arrays, which would solve it at least for the core plugin types. Because that would *greatly* simply this patch and remove all the controversial parts. Right now, you're doing much more than what the issue says.

+++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDeriverDefinitionInterface.php
@@ -0,0 +1,40 @@
+interface PluginDeriverDefinitionInterface extends PluginDefinitionInterface, MergeablePluginDefinitionInterface {
...
+  public function setDeriverClass($class);

+++ b/core/lib/Drupal/Component/Plugin/Definition/PluginInspectionDefinitionInterface.php
@@ -0,0 +1,38 @@
+interface PluginInspectionDefinitionInterface extends PluginDefinitionInterface {
...
+  public function setId($id);
...
+  public function getId();

Not that we really need a getProvider() on these, for \Drupal\Core\Plugin\DefaultPluginManager::findDefinitions() to use. Having to work around this currently in #2296423: Implement layout plugin type in core

+++ b/core/lib/Drupal/Component/Plugin/Definition/PluginInspectionDefinitionInterface.php
@@ -0,0 +1,33 @@
+interface PluginInspectionDefinitionInterface extends PluginDefinitionInterface {

I meant "Note". And my idea was to include getProvider/setProvider methods on this interface, rather than add Yet Another Plugin Definition Interface.
We can check for it in DefaultPluginManager in a follow-up.

Also I think this should be marked to be merged into PDI itself for D9. Class, ID, Provider are all really essential, and are Component level stuff (Compare to \Drupal\Component\Annotation\AnnotationInterface)

I withdraw my request to include getProvider here, that is squarely out of scope, and since we can't change PluginDefinitionInterface anyway, it might as well stay separate: #2818653: Allow object-based plugin definitions to be processed in DefaultPluginManager::findDefinitions()

> Most, if not all derivers do not set plugin IDs for derivative definitions explicitly, but provide it by merging in the base field definition for defaults

'base field definition'?

Also, it would really help to have some examples in this explanation.

I believe that was meant to say "base plugin definition", as in the parameter name of the method
public function getDerivativeDefinitions($base_plugin_definition) {

\Drupal\system\Plugin\Derivative\SystemMenuBlock is a concise example:

  public function getDerivativeDefinitions($base_plugin_definition) {
    foreach ($this->menuStorage->loadMultiple() as $menu => $entity) {
      $this->derivatives[$menu] = $base_plugin_definition;
      $this->derivatives[$menu]['admin_label'] = $entity->label();
      $this->derivatives[$menu]['config_dependencies']['config'] = [$entity->getConfigDependencyName()];
    }
    return $this->derivatives;
  }

So, just to check I get it -- what this issues is about is that if I do:

    foreach ($this->somePluginManager->getDefinitions() as $plugin_id => $definition) {

then all the $plugin_id values are correct, and for derivatives will be of the form base_plugin_id:derivative_id, but for derivatives, the $definition item contains only the base_plugin_id, so that multiple $definition values will have the same ID in them.

Where:
- the base_plugin_id is the ID specified in the plugin class annotation alongside the deriver property that gives the deriver class name
- the derivative_id is the ID keyed into $this->derivatives[$plugin_id] in DeriverBase::getDerivativeDefinitions()

If that's the case, then yup, it's a pain, as all plugin deriver classes are having to put this into the definition themselves in getDerivativeDefinitions().

The last patch doesn't apply to D8.7.10.

I have this problem witch field types, I extend from EntityReferenceItem and it's listed with the same label that it, it not use the new label.

Re rolled patch.

Re-roll for 9.1.x.

Kindly ignore last patch , because patch at #55 and #53 are identical , i am triggering test for 9.1.x branch on #53. So patch at #55 is not needed sorry for that.

Re-roll based on last patch that successfully applied (#39).

Rerolled the patch based on #57

Rerolled it again.

Code sniffer issue has been fixed.

Thanks everyone for the recent patches and moving this issue forward. To make it easier to review please provide an interdiff or a diff when uploading a patch.

First of all, thank you all for looking into the issue and try to reroll the patch. There is a major problem with rerolls.

#2296423: Implement layout plugin type in core Added \Drupal\Component\Plugin\Definition\DerivablePluginDefinitionInterface which is almost same as \Drupal\Component\Plugin\Definition\PluginDeriverDefinitionInterface in this patch that is why this patch needs be recreated from #39 again.

All the rerolls since #39 are useless and missing the pieces of the code and no one tried to understand the code changes while removing conflicts so I'm removing commit credit from all the patch rerolls.

To all the who tried to help with the reroll. If you can't understand the code conflict feel free to ask questions here or in the drupal slack contribute channel.

Please, please don't overwhelm the reviewers with too many patches which are not helpful in moving the issue forward.

We need to create a new patch by looking at #39. We don't want to reroll #39.

Updated IS from the discussion #43, #44, and #45.

@jibran, thanks for the clarification.

I read through the issue and noted that Berdir commented in the last paragraph of #37 that it would be fine to solve it here for arrays and simplify the patch. In reply, #39, Xano makes a good argument that not doing this for object based definitions introduces an inconsistency. For myself, I don't like inconsistencies at all and support that idea. However, adding it here does make the patch more complex and perhaps that is one of the factors why this issue hasn't been resolved yet. So, despite my preference for consistency let's stick to solving for arrays here. Anything else can be a followup.

In that light I have made a new patch. And then to see if it actually work I used it in Migration. Specifically, \Drupal\migrate\Plugin\MigrationPluginManager::expandPluginIds() can be removed. That functionality is still needed in tests so it now resides in MigrateTestBase.

So, that isn't going to work. Many of the migrate failures are in \Drupal\migrate\Plugin\Migration::checkRequirements() and even if that passed it would fail in the MigrateLookup service anyway. So, this removes the migrate changes.

This should fix the two failing migration tests. As for the other tests, I don't know. Can someone else look into the remaining failures?

This may fix the migrate failures.

Nice work with recreating the patch.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -981,7 +981,7 @@ public function getHighestId() {
-        if ($migration['id'] === $base_id) {
+        if ($base_id === substr($migration_id, 0, strpos($migration_id, $this::DERIVATIVE_SEPARATOR))) {

Maybe set a base_id as well on all the plugins?

I was thinking the same thing.

Fixing tests and a bit of cleanup.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -981,7 +981,7 @@ public function getHighestId() {
+        if ($base_id === substr($migration_id, 0, strpos($migration_id, $this::DERIVATIVE_SEPARATOR))) {

This can just be strpos($migration_id, $base_id) === 0.

Actually, I prefer if ($base_id === ($migration['base_id'] ?? NULL)) where we actually use the new base_id on the plugin definition.

I have looked into the three remaining errors and haven't made any progress on fixing them. Two are in areas I don't know at all. One is a FuncationalJavascript test and I have never been able to run them locally let alone debug them. Another is in content_moderation\Functional\ModerationActionsTest. I must admit that there are two migration issues related to content_moderation so I will have to dabble in it but I haven't started yet. The final one is Derivative test and it eludes me why adding the base_id to the expected causes the failure.

@hash6, are you still planning to work on this?

I noticed this needs a reroll, so here it is. There are still failing tests.

Fixing fail tests.

1. +++ b/core/tests/Drupal/KernelTests/Core/Plugin/DerivativeTest.php
   @@ -19,10 +19,27 @@ class DerivativeTest extends PluginTestBase {
   +    $expected = $this->mockBlockExpectedDefinitions;
   ...
        foreach ($this->mockBlockExpectedDefinitions as $id => $definition) {
   

   $this->mockBlockExpectedDefinitions should be changed directly, where it is defined.

2. +++ b/core/tests/Drupal/KernelTests/Core/Plugin/DerivativeTest.php
   @@ -19,10 +19,27 @@ class DerivativeTest extends PluginTestBase {
   +      } elseif ($id == "menu:navigation") {
   

   This is violating the coding standards, that's why the test failed. elseif should start a newline.

   But no need to fix it, due to my first point of feedback

Done with the changes advised by @tim.plunkett

Fixing fail test.

Re-rolled this against 9.3.x as previous patch was not applied to 9.3.x automatically.

Changing to needs review so test bot can pick it up.

Addressed my own feedback from #92, switched from patches to a MR, and fixed the tests.

Added a comment about the comments, for the id change I guess we just have to do a change notice and hope that we don't break too many things :)

The last thing to consider is the addition of base_id.

As seen in the changes to content_moderation_action_info_alter() and layout_builder_plugin_filter_block_alter(), it is helpful for any code dealing with checking definitions.

It also covers up a bug in the actions system, see #2916740-117: Add generic entity actions

This came up again at #3313473-33: CKEditor 5 plugin definitions should be derivable.

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.