A part of #1966246: [meta] Introduce specific annotations for each plugin type

CommentFileSizeAuthor
#18 1990156-18.patch4.38 KBdamiankloip
#18 interdiff-1990156-18.txt502 bytesdamiankloip
#16 1990156-16.patch4.54 KBdamiankloip
#16 interdiff-1990156-16.txt964 bytesdamiankloip
#14 1990156-14.patch4.35 KBdamiankloip
#14 interdiff-1990156-14.txt1.45 KBdamiankloip
#13 1990156-13.patch4.16 KBdamiankloip
#13 interdiff-1990156-13.txt0 bytesdamiankloip
#10 1990156-10.patch4.72 KBdamiankloip
#10 interdiff-1990156-10.txt1.59 KBdamiankloip
#8 1990156-8.patch5.09 KBdamiankloip
#8 interdiff-1990156-8.txt1.14 KBdamiankloip
#7 1990156-7.patch5.09 KBdamiankloip
db.condition-annotation.patch4.21 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: Unassigned » damiankloip
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

db.condition-annotation.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, db.condition-annotation.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

db.condition-annotation.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

db.condition-annotation.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, db.condition-annotation.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
1990156-7.patch5.09 KB

Rerolled. If we want to do this, we can't really call the PluginManagerBase constructor like we currently are. So I think we really need to just override the whole thing:

--- a/core/lib/Drupal/Core/Condition/ConditionManager.php
+++ b/core/lib/Drupal/Core/Condition/ConditionManager.php
@@ -12,7 +12,13 @@
 use Drupal\Core\Executable\ExecutableInterface;
 use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Language\LanguageManager;
+use Drupal\Core\Language\Language;
 use Drupal\Core\Plugin\DefaultPluginManager;
+use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
+use Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator;
+use Drupal\Core\Plugin\Discovery\AlterDecorator;
+use Drupal\Core\Plugin\Discovery\CacheDecorator;
+use Drupal\Component\Plugin\Factory\DefaultFactory;
 
 /**
  * A plugin manager for condition plugins.
@@ -33,7 +39,16 @@ class ConditionManager extends DefaultPluginManager implements ExecutableManager
    *   The module handler to invoke the alter hook with.
    */
   public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler) {
-    parent::__construct('Condition', $namespaces);
+    $annotation_namespaces = array(
+      'Drupal\Core\Condition\Annotation' => DRUPAL_ROOT . '/core/lib',
+    );
+    $this->discovery = new AnnotatedClassDiscovery('Condition', $namespaces, $annotation_namespaces, 'Drupal\Core\Condition\Annotation\Condition');
+    $this->discovery = new DerivativeDiscoveryDecorator($this->discovery);
+    $this->discovery = new AlterDecorator($this->discovery, 'condition_info');
+    $this->discovery = new CacheDecorator($this->discovery, 'condition:' . $language_manager->getLanguage(Language::TYPE_INTERFACE)->langcode);
+
+    $this->factory = new DefaultFactory($this);
+
     $this->alterInfo($module_handler, 'condition_info');
     $this->setCacheBackend($cache_backend, $language_manager, 'condition');
   }
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-1990156-8.txt1.14 KB
1990156-8.patch5.09 KB

Meh, sorry. We need to also use ContainerFactory and NOT DefaultFactory.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Condition/ConditionManager.phpundefined
@@ -33,7 +39,16 @@ class ConditionManager extends DefaultPluginManager implements ExecutableManager
+    $annotation_namespaces = array(
+      'Drupal\Core\Condition\Annotation' => DRUPAL_ROOT . '/core/lib',
+    );
+    $this->discovery = new AnnotatedClassDiscovery('Condition', $namespaces, $annotation_namespaces, 'Drupal\Core\Condition\Annotation\Condition');
+    $this->discovery = new DerivativeDiscoveryDecorator($this->discovery);

All these parameters could be solved by calling parent::__construct with these parameters, so just the alterdecorator is needed. The containerfactory also exists on the parent.

+++ b/core/lib/Drupal/Core/Condition/ConditionManager.phpundefined
@@ -33,7 +39,16 @@ class ConditionManager extends DefaultPluginManager implements ExecutableManager
+    $this->discovery = new CacheDecorator($this->discovery, 'condition:' . $language_manager->getLanguage(Language::TYPE_INTERFACE)->langcode);

The defaultPluginManager already cares about caching, so this should not be added here.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-1990156-10.txt1.59 KB
1990156-10.patch4.72 KB

thank you, so we can just do this...

We don't need to implement an alter decorator either, as this is also taken care of on the parent.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Perfect!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Condition/ConditionManager.phpundefined
@@ -12,7 +12,13 @@
+use Drupal\Core\Language\Language;
 use Drupal\Core\Plugin\DefaultPluginManager;
+use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
+use Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator;
+use Drupal\Core\Plugin\Discovery\AlterDecorator;
+use Drupal\Core\Plugin\Discovery\CacheDecorator;
+use Drupal\Core\Plugin\Factory\ContainerFactory;

How come all the new use statements? They don't appear to be... used :)

+++ b/core/lib/Drupal/Core/Condition/ConditionManager.phpundefined
@@ -33,9 +39,13 @@ class ConditionManager extends DefaultPluginManager implements ExecutableManager
+    parent::__construct('Condition', $namespaces, $annotation_namespaces,'Drupal\Core\Condition\Annotation\Condition');

Missing a space... $annotation_namespaces,'Drupal\Core\Condition\Annotation\Condition'

Given #2022087: Add module owner to plugin definition in AnnotatedClassDiscovery should we clean up the module key here too or is that out-of-scope... actually I guess it is.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-1990156-13.txt0 bytes
1990156-13.patch4.16 KB

You're totally right.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-1990156-14.txt1.45 KB
1990156-14.patch4.35 KB

Spoke to alexpott on IRC, Let's also take care of the module key in the annotations too.

Status: Needs review » Needs work

The last submitted patch, 1990156-14.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-1990156-16.txt964 bytes
1990156-16.patch4.54 KB

That classic comma in an annotation again :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/core/modules/language/lib/Drupal/language/Plugin/Condition/Language.phpundefined
@@ -26,7 +25,7 @@
+classLanguage extends ConditionPluginBase {

Let me think for second .. . this can't work, even in PHP.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-1990156-18.txt502 bytes
1990156-18.patch4.38 KB

Haha, no, certainly not!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Perfect, wow this needed quite a big amount of comments.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 377521e and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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