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

Created on 21 November 2014, over 9 years ago
Updated 29 August 2023, 10 months ago

Problem/Motivation

More unit testing goodness for #2052109: [meta] Expand phpunit tests for \Drupal\Component\Plugin classes β†’ .

This time we're testing Drupal\Component\Plugin\Derivative\DeriverBase:: getDerivativeDefinition(), which it turns out is buggy.

  public function getDerivativeDefinition($derivative_id, $base_plugin_definition) {
    if (!empty($this->derivatives) && !empty($this->derivatives[$derivative_id])) {
      return $this->derivatives[$derivative_id];
    }
    $this->getDerivativeDefinitions($base_plugin_definition);
    return $this->derivatives[$derivative_id];
  }

This method dutifully checks if the array elements exist on $derivatives, but then fails to check them again after calling getDerivativeDefinitions(). Furthermore, it's likely that the return value of getDerivativeDefinitions() should be stored in $derivatves, but the method doesn't do this.

Also, the behavior of DeriverInterface can't really be deduced from this base class. That is, don't look to this class for clues about how derivers should work, because it is inconsistent.

Proposed resolution

  • Analyze the need for storing the results of getDerivativeDefinitions().
  • Add a test which covers these cases.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

<!--Uncomment the relevant rows for the issue. -->
πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
PluginΒ  β†’

Last updated about 14 hours ago

Created by

πŸ‡ΊπŸ‡ΈUnited States Mile23 Seattle, WA

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡©πŸ‡ͺGermany donquixote

    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;
    
  • πŸ‡©πŸ‡ͺGermany donquixote

    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.

Production build 0.69.0 2024