Files: 
CommentFileSizeAuthor
#18 1990156-18.patch4.38 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,181 pass(es). View
#18 interdiff-1990156-18.txt502 bytesdamiankloip
#16 1990156-16.patch4.54 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Plugin/Condition/Language.php. View
#16 interdiff-1990156-16.txt964 bytesdamiankloip
#14 1990156-14.patch4.35 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,000 pass(es), 0 fail(s), and 1 exception(s). View
#14 interdiff-1990156-14.txt1.45 KBdamiankloip
#13 1990156-13.patch4.16 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#13 interdiff-1990156-13.txt0 bytesdamiankloip
#10 1990156-10.patch4.72 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,100 pass(es). View
#10 interdiff-1990156-10.txt1.59 KBdamiankloip
#8 1990156-8.patch5.09 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,867 pass(es). View
#8 interdiff-1990156-8.txt1.14 KBdamiankloip
#7 1990156-7.patch5.09 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
db.condition-annotation.patch4.21 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch db.condition-annotation.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

damiankloip’s picture

Assigned:Unassigned» damiankloip
dawehner’s picture

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

Status:Needs work» Needs review

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

dawehner’s picture

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

Assigned:damiankloip» Unassigned
Status:Needs work» Needs review
FileSize
5.09 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

FileSize
1.14 KB
5.09 KB
PASSED: [[SimpleTest]]: [MySQL] 57,867 pass(es). View

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

dawehner’s picture

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

Status:Needs work» Needs review
FileSize
1.59 KB
4.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,100 pass(es). View

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

Status:Needs review» Reviewed & tested by the community

Perfect!

alexpott’s picture

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

Status:Needs work» Needs review
FileSize
0 bytes
4.16 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

You're totally right.

damiankloip’s picture

FileSize
1.45 KB
4.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,000 pass(es), 0 fail(s), and 1 exception(s). View

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

Status:Needs work» Needs review
FileSize
964 bytes
4.54 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Plugin/Condition/Language.php. View

That classic comma in an annotation again :)

dawehner’s picture

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

Status:Needs work» Needs review
FileSize
502 bytes
4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,181 pass(es). View

Haha, no, certainly not!

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

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.