Problem/Motivation

We currently have 3 discovery mechanisms and 4 decorators in core. All but one decorators implement __call() in the same way, and all but one discovery mechanisms and decorators implement getDefinition() in the same way. Both of these implementations can be considered generic implementations that most discovery mechanisms and decorators in contrib will likely implement as well.

Proposed resolution

Create a DiscoveryBase which implements getDefinition() and a DiscoveryDecoratorBase which extends DiscoveryBase and implements __call(). In core alone this patch has a negative diffstat.

Remaining tasks

Review patch.

User interface changes

None.

API changes

None. There is no "We have a new Plugin API" change notice. If we ever create one, we should mention the new base classes.

CommentFileSizeAuthor
#11 1809200-11.discovery-base.patch15.39 KBtstoeckler
#6 1809200-5-plugins-discovery-base.patch11.41 KBtstoeckler
#6 1809200-1-5-interdiff-do-not-test.patch3.59 KBtstoeckler
#1 1809200-1-plugin-discovery-base.patch9.72 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
1809200-1-plugin-discovery-base.patch9.72 KB

Here's the patch.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Looks good to me and seems to make sense.

There doesn't seem to be a difference in terms of PHP class discovery/loading either; the interface is replaced with a base class, so just a different lookup.

I only have a stupid nitpick:

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
@@ -147,11 +147,4 @@ protected function getDerivativeFetcher($base_plugin_id, array $base_definition)
   }
-

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -94,5 +86,4 @@ public function getDefinitions() {
   }
-
 }

Apparently, there's supposed to be a blank line before the closing curly brace of a class definition.

Status: Needs review » Needs work

The last submitted patch, 1809200-1-plugin-discovery-base.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Re #2: I think that *should* be a standard, but it currently isn't. See http://drupal.org/node/608152

Since it's completely unrelated, I'll take it out of the patch, though. Better patch coming up in a minute.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Postponed

It breaks a lot of things, so better leave this postponed til feature freez

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Postponed » Needs review
FileSize
1809200-1-5-interdiff-do-not-test.patch3.59 KB
1809200-5-plugins-discovery-base.patch11.41 KB

Wow, I managed to massively mess that up...

This one should work.

I also found LegacyDiscoveryDecorator in field.module.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Postponed

I agree with @andypost, this is a nice clean-up that should probably wait, it will break a lot of the issues that are using this code.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Hmm... I guess there's nothing that inherently speaks against doing this after feature-freeze, but can you explain what exactly would be touched by this? Any code sub-classing any of the existing classes is not affected. I.e. only patches that actually change/refactor the current discovery mechanisms and decorators would need to be rerolled. I'm not aware of any of those, so. Maybe I'm missing something.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Postponed » Needs review

Oh, hmm, DiscoveryInterface is still able to be used in typehinting...
I take it back! Carry on.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

After writing my own DiscoveryDecorator tonight, this makes things much much clearer.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
1809200-11.discovery-base.patch15.39 KB

This needed quite a re-roll.

Forgot to add the diffstat to the patch, but by now it's +71 -138, so not bad I guess. :-)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Closed (duplicate)

I guess they ignored this issue and did #2026769: Simplify DerivativeInterface implementations by refactoring duplicated code to a base class instead.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue summary: View changes

Updated issue summary.