Issue #1778942 by yched, tstoeckler, andypost: Fixed Discovery::getDefinition() / getDefinitions() : inconsistent return values for 'no result.

git commit -am Issue

core/lib/Drupal/Component/Plugin/Discovery/DiscoveryInterface.php

@@ -19,8 +19,8 @@ interface DiscoveryInterface {
    * @param string $plugin_id
    *   A plugin id.
    *
-   * @return array
-   *   A plugin definition.
+   * @return array|null
+   *   A plugin definition, or NULL if no definition was found for $plugin_id.
    */
   public function getDefinition($plugin_id);
 
@@ -28,7 +28,8 @@ interface DiscoveryInterface {
    * Gets the definition of all plugins for this type.
    *
    * @return array
-   *   An array of plugin definitions.
+   *   An array of plugin definitions (empty array if no definitions were
+   *   found).
    */
   public function getDefinitions();
 

core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php

@@ -33,7 +33,7 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
    */
   public function getDefinition($plugin_id) {
     $plugins = $this->getDefinitions();
-    return isset($plugins[$plugin_id]) ? $plugins[$plugin_id] : array();
+    return isset($plugins[$plugin_id]) ? $plugins[$plugin_id] : NULL;
   }
 
   /**

core/lib/Drupal/Core/Plugin/Discovery/CacheDecorator.php

@@ -65,7 +65,7 @@ class CacheDecorator implements DiscoveryInterface {
    */
   public function getDefinition($plugin_id) {
     $definitions = $this->getDefinitions();
-    return isset($definitions[$plugin_id]) ? $definitions[$plugin_id] : array();
+    return isset($definitions[$plugin_id]) ? $definitions[$plugin_id] : NULL;
   }
 
   /**

core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.php

@@ -37,7 +37,7 @@ class HookDiscovery implements DiscoveryInterface {
    */
   public function getDefinition($plugin_id) {
     $plugins = $this->getDefinitions();
-    return isset($plugins[$plugin_id]) ? $plugins[$plugin_id] : array();
+    return isset($plugins[$plugin_id]) ? $plugins[$plugin_id] : NULL;
   }
 
   /**

core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.php

@@ -61,19 +61,6 @@ class WidgetPluginManager extends PluginManagerBase {
     cache($this->cache_bin)->delete($this->cache_id);
   }
 
-  /**
-   * Overrides Drupal\Component\Plugin\PluginManagerBase::getDefinition().
-   *
-   * @todo Remove when http://drupal.org/node/1778942 is fixed.
-   */
-  public function getDefinition($plugin_id) {
-    $definition = $this->discovery->getDefinition($plugin_id);
-    if (!empty($definition)) {
-      $this->processDefinition($definition, $plugin_id);
-      return $definition;;
-    }
-  }
-
   /**
    * Overrides Drupal\Component\Plugin\PluginManagerBase::getInstance().
    */

core/modules/system/lib/Drupal/system/Tests/Plugin/Discovery/AnnotatedClassDiscoveryTest.php

@@ -0,0 +1,51 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\system\Tests\Plugin\Discovery\AnnotatedClassDiscoveryTest.
+ */
+
+namespace Drupal\system\Tests\Plugin\Discovery;
+
+use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
+
+/**
+ * Tests that plugins with annotated classes are correctly discovered.
+ */
+class AnnotatedClassDiscoveryTest extends DiscoveryTestBase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Annotated class discovery',
+      'description' => 'Tests that plugins are correctly discovered using annotated classes.',
+      'group' => 'Plugin API',
+    );
+  }
+
+  public function setUp() {
+    parent::setUp();
+    $this->expectedDefinitions = array(
+      'apple' => array(
+        'id' => 'apple',
+        'label' => 'Apple',
+        'color' => 'green',
+        'class' => 'Drupal\plugin_test\Plugin\plugin_test\fruit\Apple',
+      ),
+      'cherry' => array(
+        'id' => 'cherry',
+        'label' => 'Cherry',
+        'color' => 'red',
+        'class' => 'Drupal\plugin_test\Plugin\plugin_test\fruit\Cherry',
+      ),
+      'orange' => array(
+        'id' => 'orange',
+        'label' => 'Orange',
+        'color' => 'orange',
+        'class' => 'Drupal\plugin_test\Plugin\plugin_test\fruit\Orange',
+      ),
+    );
+    $this->discovery = new AnnotatedClassDiscovery('plugin_test', 'fruit');
+    $this->emptyDiscovery = new AnnotatedClassDiscovery('non_existing_module', 'non_existing_plugin_type');
+  }
+}
+

core/modules/system/lib/Drupal/system/Tests/Plugin/Discovery/DiscoveryTestBase.php

@@ -0,0 +1,62 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\system\Tests\Plugin\Discovery\DiscoveryTestBase.
+ */
+
+namespace Drupal\system\Tests\Plugin\Discovery;
+
+use Drupal\simpletest\UnitTestBase;
+
+/**
+ * Tests that plugins are correctly discovered.
+ */
+class DiscoveryTestBase extends UnitTestBase {
+
+  /**
+   * The discovery component to test.
+   *
+   * @var \Drupal\Component\Plugin\Discovery\DiscoveryInterface
+   */
+  protected $discovery;
+
+  /**
+   * The plugin definitions the discovery component is expected to discover.
+   *
+   * @var array
+   */
+  protected $expectedDefinitions;
+
+  /**
+   * An empty discovery component.
+   *
+   * This will be tested to ensure that the case where no plugin information is
+   * found, is handled correctly.
+   *
+   * @var \Drupal\Component\Plugin\Discovery\DiscoveryInterface
+   */
+  protected $emptyDiscovery;
+
+  /**
+   * Tests getDefinitions() and getDefinition().
+   */
+  function testDiscoveryInterface() {
+    // Ensure that getDefinitions() returns the expected definitions.
+    // For the arrays to be identical (instead of only equal), they must be
+    // sorted equally, which seems unneccessary here.
+    $this->assertEqual($this->discovery->getDefinitions(), $this->expectedDefinitions);
+
+    // Ensure that getDefinition() returns the expected definition.
+    foreach ($this->expectedDefinitions as $id => $definition) {
+      $this->assertIdentical($this->discovery->getDefinition($id), $definition);
+    }
+
+    // Ensure that an empty array is returned if no plugin definitions are found.
+    $this->assertIdentical($this->emptyDiscovery->getDefinitions(), array(), 'array() returned if no plugin definitions are found.');
+
+    // Ensure that NULL is returned as the definition of a non-existing plugin.
+    $this->assertIdentical($this->emptyDiscovery->getDefinition('non_existing'), NULL, 'NULL returned as the definition of a non-existing plugin.');
+  }
+}
+

core/modules/system/lib/Drupal/system/Tests/Plugin/Discovery/StaticDiscoveryTest.php

@@ -0,0 +1,57 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\system\Tests\Plugin\Discovery\StaticDiscoveryTest.
+ */
+
+namespace Drupal\system\Tests\Plugin\Discovery;
+
+use Drupal\Component\Plugin\Discovery\StaticDiscovery;
+
+/**
+ * Tests that plugins are correctly discovered.
+ */
+class StaticDiscoveryTest extends DiscoveryTestBase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Static discovery',
+      'description' => 'Tests that plugins using static discovery are correctly discovered.',
+      'group' => 'Plugin API',
+    );
+  }
+
+  public function setUp() {
+    parent::setUp();
+    $this->expectedDefinitions = array(
+      'apple' => array(
+        'label' => 'Apple',
+        'color' => 'green',
+      ),
+      'cherry' => array(
+        'label' => 'Cherry',
+        'color' => 'red',
+      ),
+      'orange' => array(
+        'label' => 'Orange',
+        'color' => 'orange',
+      ),
+    );
+    // Instead of registering the empty discovery component first and then
+    // setting the plugin definitions, we set them first and then delete them
+    // again. This implicitly tests StaticDiscovery::deleteDefinition() (in
+    // addition to StaticDiscovery::setDefinition() which we need to use
+    // anyway).
+    $discovery = new StaticDiscovery();
+    foreach ($this->expectedDefinitions as $plugin_id => $definition) {
+      $discovery->setDefinition($plugin_id, $definition);
+    }
+    $this->discovery = clone $discovery;
+    foreach ($this->expectedDefinitions as $plugin_id => $definition) {
+      $discovery->deleteDefinition($plugin_id);
+    }
+    $this->emptyDiscovery = $discovery;
+  }
+}
+

core/modules/system/lib/Drupal/system/Tests/Plugin/DiscoveryTest.php

@@ -1,38 +0,0 @@
-<?php
-
-/**
- * @file
- * Definition of Drupal\system\Tests\Plugin\DiscoveryTest.
- */
-
-namespace Drupal\system\Tests\Plugin;
-
-/**
- * Tests that plugins are correctly discovered.
- */
-class DiscoveryTest extends PluginTestBase {
-
-  public static function getInfo() {
-    return array(
-      'name' => 'Discovery',
-      'description' => 'Tests that plugins are correctly discovered.',
-      'group' => 'Plugin API',
-    );
-  }
-
-  /**
-   * Tests getDefinitions() and getDefinition().
-   */
-  function testDiscoveryInterface() {
-    // Ensure that getDefinitions() returns the expected definitions.
-    $this->assertIdentical($this->testPluginManager->getDefinitions(), $this->testPluginExpectedDefinitions);
-
-    // Ensure that getDefinition() returns the expected definition.
-    foreach ($this->testPluginExpectedDefinitions as $id => $definition) {
-      $this->assertIdentical($this->testPluginManager->getDefinition($id), $definition);
-    }
-
-    // Ensure that NULL is returned as the definition of a non-existing plugin.
-    $this->assertIdentical($this->testPluginManager->getDefinition('non_existing'), NULL, 'NULL returned as the definition of a non-existing base plugin.');
-  }
-}

core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/fruit/Apple.php

@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\plugin_test\Plugin\plugin_test\fruit\Apple.
+ */
+
+namespace Drupal\plugin_test\Plugin\plugin_test\fruit;
+
+use Drupal\Core\Annotation\Plugin;
+
+/**
+ * @Plugin(
+ *   id = "apple",
+ *   label = "Apple",
+ *   color = "green"
+ * )
+ */
+class Apple {}
+

core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/fruit/Cherry.php

@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\plugin_test\Plugin\plugin_test\fruit\Cherry.
+ */
+
+namespace Drupal\plugin_test\Plugin\plugin_test\fruit;
+
+use Drupal\Core\Annotation\Plugin;
+
+/**
+ * @Plugin(
+ *   id = "cherry",
+ *   label = "Cherry",
+ *   color = "red"
+ * )
+ */
+class Cherry {}
+

core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/fruit/Orange.php

@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\plugin_test\Plugin\plugin_test\fruit\Orange.
+ */
+
+namespace Drupal\plugin_test\Plugin\plugin_test\fruit;
+
+use Drupal\Core\Annotation\Plugin;
+
+/**
+ * @Plugin(
+ *   id = "orange",
+ *   label = "Orange",
+ *   color = "orange"
+ * )
+ */
+class Orange {}
+

core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/fruit/README.txt

@@ -0,0 +1,3 @@
+The classes in this directory act as a mock plugin type to test annotated class
+discovery. See the corresponding test file:
+/core/modules/system/lib/Drupal/system/Tests/Plugin/Discovery/AnnotatedClassDiscoveryTest.php