Most DerivativeInterface implementations share common code(getDerivativeDefinition() and data (protected $derivatives).

Refactoring them to an abstract base class would help us remove some duplicated code and reduce the apparent complexity of plugin derivative creators by making them only have to write one method (getDerivativeDefinitions()).

The already copy/pasted implementations are present in:

  • BlockPluginUI
  • CustomBlock
  • DefaultWizardDeriver
  • LanguageBlock
  • Layout
  • SelectionBase
  • SystemMenuBlock

Alternate implementations are present in AggregatorCategoryBlock and AggregatorFeedBlock (more inline code), EntityDerivative (slightly different), and MockLayoutBlockDeriver and MockMenuBlockDeriver (always invoke getDerivativeDefinitions(), do not use $this->derivatives caching).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fgm’s picture

Title: Refactore DerivativeInterface implementations to remove some code » Simplify DerivativeInterface implementations by refactoring duplicated code to a base class
Status: Active » Needs review
FileSize
14.13 KB

Trying out patch.

fgm’s picture

Slightly reworded for better clarity.

dawehner’s picture

Just some small points.

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeBase.phpundefined
@@ -0,0 +1,37 @@
+ * Definition of Drupal\Component\Plugin\Derivative\DerivativeBase.

This should be "Contains \".

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeBase.phpundefined
@@ -0,0 +1,37 @@
+   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinition().

Let's use {@inheritdoc} instead

aspilicious’s picture

Do we always need to say "implements derivativeInterface" if the base class already does it?

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeBase.phpundefined
@@ -0,0 +1,37 @@
+    $this->getDerivativeDefinitions($base_plugin_definition);

Lets add an empty implementation of this function in the base class

I started the same issue in the queue I'll close that one.

fgm’s picture

More simplifications, as suggested in #3 and #4.

The patches show two versions:

  • the first is the shortest and probably fastest, in which DerivativeBase::getDerivativeDefinitions() is an abstract function, and child classes do just what they want.
  • the second, applied on top of the first, lets the child classes do what they want in the protected $this->derivatives, then defer to the parent implementation to return. It is likely to be a little bit slower.

Status: Needs review » Needs work

The last submitted patch, 0002-Issue-2026769-by-fgm-Provide-a-base-implementation-f.patch, failed testing.

dawehner’s picture

Micro-performance doesn't really matter, as plugin instances will be cached all the time.

aspilicious’s picture

Status: Needs work » Needs review

I would go with the first one. Needs review for that one.

fgm’s picture

I would favour the first version too, but for the record, here is the second one in a single patch which the bot can test.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

#5:1 looks good to me.

Eclipse

tim.plunkett’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed
 9 files changed, 129 insertions(+), 227 deletions(-)

Very nice. Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.