Drupal\Component\Plugin\Derivative\DeriverBase is designed to error out.

+++ b/core/tests/Drupal/Tests/Component/Plugin/Derivative/DeriverBaseTest.php
@@ -0,0 +1,66 @@
+/**
+ * @coversDefaultClass Drupal\Component\Plugin\Derivative\DeriverBase
+ */
+class DeriverBaseTest extends UnitTestCase {

DOn't we still need a @group?

Might one potential fix would be for getDerivativeDefinitions() to actually update $this->derivatives, and then ensure the $derivative_id exists?

What about doing it like this? Less confusing logic, with a single point of failure (getDefinitions()).

This actually makes more sense, test-wise.

This fix makes sense to me. @dawehner, @Mile23?

Thanks for the reviews, and to @Mile23 for catching this bug in the first place!

Still missing an @group.

Also this will result in a performance hit for things like SelectionBase::getDerivativeDefinition() - since that will now always call SelectionBase::getDerivativeDefinitions().

  /**
   * {@inheritdoc}
   */
  public function getDerivativeDefinitions($base_plugin_definition) {
    foreach ($this->entityManager->getDefinitions() as $entity_type_id => $entity_type) {
      $this->derivatives[$entity_type_id] = $base_plugin_definition;
      $this->derivatives[$entity_type_id]['entity_types'] = array($entity_type_id);
      $this->derivatives[$entity_type_id]['label'] = t('@entity_type selection', array('@entity_type' => $entity_type->getLabel()));
      $this->derivatives[$entity_type_id]['base_plugin_label'] = (string) $base_plugin_definition['label'];
    }
    return parent::getDerivativeDefinitions($base_plugin_definition);
  }

Needs work for #12

Theoretically that's a performance hit, but that's why most, if not all, plugin managers cache the processed plugin definitions. Even if a piece of code calls these deriver methods more than once, it's always been the deriver's responsibility to provide internal caching. Should we add that to SelectionBase?

If you look closely, there's something else wrong here. DeriverBase::getDerivativeDefinition() statically caches derivatives, not per base plugin ID. This means that if you'd use the derivers as DeriverInterface permits by calling getDerivativeDefinition() for different base plugins, you'd keep getting the same results as the derivative definitions of the first base plugin are cached and returned on subsequent calls. The only reason we've never had this problem in core, is that DerivativeDiscoveryDecorator creates a new deriver every time it needs to use one.

This patch fixes the other bug as well, and makes sure that all overrides of DeriverBase::getDefinitions() use static caching.

Re-roll.

I have rerolled the patch for 8.2.x version.

Any Update on this issue ?

Patch no longer applies, mostly due to #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase

Re-roll (based the patch in #22 due to the test failures of the patch in #29).

Well that was an abject failure! Let's try that again...

I agree with the problem - the current DeriverBase is definitely flawed.

But I think one of our observations is wrong.

If you look closely, there's something else wrong here. DeriverBase::getDerivativeDefinition() statically caches derivatives, not per base plugin ID. This means that if you'd use the derivers as DeriverInterface permits by calling getDerivativeDefinition() for different base plugins, you'd keep getting the same results as the derivative definitions of the first base plugin are cached and returned on subsequent calls. The only reason we've never had this problem in core, is that DerivativeDiscoveryDecorator creates a new deriver every time it needs to use one.

I would say this is wrong with the current way that deriver classes are used.

Deriver classes get the base plugin id injected in the constructor or the ::create() method.
See ContainerDerivativeDiscoveryDecorator::getDeriver().

This means one deriver object is only ever used for the same base plugin definition.

But now the $base_plugin_definition is passed as a parameter to the ->getDerivativeDefinitions() method, which falsely suggests that the same deriver object could be used for different base plugin definitions.

Technically, even with the proposed patch, this is still not fully supported.
In theory, you can poison the cache if you have two different base definitions with the same id.
This should normally not happen, but this is relying a lot on the context in which this is called.

A cleaner solution would be to inject the base definition in the constructor and ::create(), alongside the base plugin id, and not pass it to the ->getDerivativeDefinitions() and ->getDerivativeDefinition() method.

Ofc changing this would cause BC issues.

-----

The sections below may become obsolete if we apply changes as proposed above.

---------

Originally, before writing the section above, I was going to say the proposed solution is BC-breaking.
There are subclasses in contrib that write directly to $this->derivatives.
Now we are changing the structure of the array, which breaks these subclasses.

In practice, this is not really as big a problem as I originally thought.
The new version of ->getDerivativeDefinition() no longer looks into $this->derivatives. So the subclass can basically do what it wants with this property.

It only becomes BC-breaking if we start using the deriver in a way where the poisoned cache actually matters.
I don't currently see that we plan to do that.

A problem is that we don't really push contrib modules to upgrade their subclasses, unless developers "read the manual", which most of them won't.
To be more recognizable, we could deprecate this base class and create a new one with different name.
(at the cost of wasting a nice class name)

BUT
All I wrote in this section kind of becomes obsolete with the earlier section.

-------

Now some other feedback.

    return isset($definitions[$derivative_id]) ? $definitions[$derivative_id] : NULL;

Can we use ?? null coalesce operator?

    return $definitions[$derivative_id] ?? NULL;

   public function getDerivativeDefinitions($base_plugin_definition) {
     return isset($this->derivatives[$base_plugin_definition['id']]) ? $this->derivatives[$base_plugin_definition['id']] : [];
   }

I don't think this provides a nice DX for the subclass.
The subclass still needs to write to the protected property, and then call the parent method. Not convenient.
What about this instead?

   public function getDerivativeDefinitions($base_plugin_definition) {
     return $this->derivatives[$base_plugin_definition['id']] ??= $this->doGetDerivativeDefinitions($base_plugin_definition);
   }
   abstract protected function doGetDerivativeDefinitions($base_plugin_definition): array;

Actually, I wrote a new version of this class long ago, and shared it here :)
#2727011-34: [policy, no patch] Private vs protected, and the role of inheritance

I finally found a good example in the Drupal codebase!

Drupal\Component\Plugin\Derivative\DeriverBase.

An alternative implementation of this class would look like this:

abstract class DeriverBase implements DeriverInterface {

  /**
   * Base plugin definition.
   *
   * Subclasses don't need access to this.
   *
   * @var array|null
   */
  private $basePluginDefinition;

  /**
   * Derived plugin definitions.
   *
   * Subclasses don't need access to this.
   *
   * @var array[]|null
   */
  private $definitions;

  /**
   * {@inheritdoc}
   */
  final public function getDerivativeDefinition($derivative_id, $base_plugin_definition): ?array {
    $definitions = $this->getDerivativeDefinitions($base_plugin_definition);
    return $definitions[$derivative_id] ?? NULL;
  }

  /**
   * {@inheritdoc}
   */
  final public function getDerivativeDefinitions($base_plugin_definition): array {

    if ($this->definitions === NULL
      // In the unexpected case where a deriver is called with a different base
      // plugin definition than in a previous call, all derivatives need to be
      // reset.
      || $this->basePluginDefinition !== $base_plugin_definition
    ) {
      $this->definitions = $this->buildDerivativeDefinitions($base_plugin_definition);
      $this->basePluginDefinition = $base_plugin_definition;
    }

    return $this->definitions;
  }

  /**
   * Builds derivative definitions for a given base plugin definition.
   *
   * @param array $base_plugin_definition
   *   The definition array of the base plugin.
   *
   * @return array
   *   An array of full derivative definitions, keyed by derivative id.
   */
  abstract protected function buildDerivativeDefinitions(array $base_plugin_definition): array;

}

With a base class like this, subclasses don't ever need to touch the properties of the base class.

I have used this base class successfully in a project.