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

Comments

damiankloip’s picture

damiankloip commented
Assigned: Unassigned » damiankloip
dawehner’s picture

dawehner
Primary language German
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
commented
Status: Needs work » Needs review

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

dawehner’s picture

dawehner
Primary language German
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 commented
Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 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 commented
StatusFileSize
new interdiff-1990156-8.txt1.14 KB
new 1990156-8.patch5.09 KB

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

dawehner’s picture

dawehner
Primary language German
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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1990156-10.txt1.59 KB
new 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
commented
Status: Needs review » Reviewed & tested by the community

Perfect!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1990156-13.txt0 bytes
new 1990156-13.patch4.16 KB

You're totally right.

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff-1990156-14.txt1.45 KB
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1990156-16.txt964 bytes
new 1990156-16.patch4.54 KB

That classic comma in an annotation again :)

dawehner’s picture

dawehner
Primary language German
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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1990156-18.txt502 bytes
new 1990156-18.patch4.38 KB

Haha, no, certainly not!

dawehner’s picture

dawehner
Primary language German
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 🇪🇺🌍
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.