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
StatusFileSize
new5.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

StatusFileSize
new1.14 KB
new5.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
StatusFileSize
new1.59 KB
new4.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
StatusFileSize
new0 bytes
new4.16 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

You're totally right.

damiankloip’s picture

StatusFileSize
new1.45 KB
new4.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
StatusFileSize
new964 bytes
new4.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
StatusFileSize
new502 bytes
new4.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.