Provide a trait for categorizing plugin managers and use it for conditions and actions

+++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
@@ -0,0 +1,113 @@
+  /**
+   * Processes a definition to ensure there is a category.
+   *
+   * If the definition lacks a category, it defaults to the providing module.
+   *
+   * @param array $definition
+   *   The plugin definition.
+   */
+  protected function processDefinitionCategory(&$definition) {
+    // Ensure that every block has a category.
+    if (empty($definition['category'])) {
+      $definition['category'] = $this->getModuleName($definition['provider']);
+    }
+  }

We are now introducing even more code that does not work with object plugin definitions.

This looks almost ready!

1. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,112 @@
   +trait CategorizingPluginManagerTrait {
   

   Looks like there is no test coverage for this trait? Or is that already covered with block plugin test cases? Could you add a comment where this is covered?

2. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,112 @@
   +  /**
   +   * Processes a definition to ensure there is a category.
   +   *
   +   * If the definition lacks a category, it defaults to the providing module.
   +   *
   +   * @param array $definition
   +   *   The plugin definition.
   +   */
   +  protected function processDefinitionCategory(&$definition) {
   +    // Ensure that every block has a category.
   +    if (empty($definition['category'])) {
   +      $definition['category'] = $this->getModuleName($definition['provider']);
   +    }
   +  }
   

   So the assumption is that only modules can provide plugins? The plugin definition uses "provider" to name the extension providing the plugin - I'm not sure if themes or install profiles even can provide plugins.

2. More importantly, the comments still talk about block, should be plugin.

A install profile should be in the module list too, and the worst thing that would happen to a module is that it would get the machine name in category if it doesn't specify it manually.

The new test case looks good, thanks! Only minor nitpicks left:

1. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +     * @var \Drupal\Component\Plugin\CategorizingPluginManagerInterface | \PHPUnit_Framework_MockObject_MockObject
   

   there should not be a a space before and after the | separator.

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +          'vegetables'
   

   missing trailing comma.

3. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +      $this->assertSame(array_keys($sorted), ['apple', 'mango', 'cucumber']);
   

   Inconsistent usage of [] and array() in this test file. Please use one or the other everywhere.

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +    /**
   +     * Implements getDefinitions() for the trait to provide some test definitions.
   +     *
   +     * @return array
   +     */
   +    public function getDefinitions() {
   

   no needs to document @return here. Just use {@inheritdoc} and add the additional comment under it.

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +     * Implements getDefinitions() for the trait to provide some test definitions.
   

   Exceeds 80 chars.

6. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +    /**
   +     * {@inheritdoc}
   +     */
   +    public function processDefinition(&$definition, $plugin_id) {
   +      parent::processDefinition($definition, $plugin_id);
   +      $this->processDefinitionCategory($definition);
   +    }
   

   why does the test class have to override this method? Please add a comment.

Ah, I see.

So this looks good to me now.

+++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
@@ -0,0 +1,114 @@
+      $grouped_definitions[$definition['category']][$id] = $definition;

An object is not casted to a string when used as an array key.

Simple fix attached since I already wrote it when implementing this new trait for field type plugins.

An object is not casted to a string when used as an array key.

$definition['category'] can be an object ? Sounds weird - when does this happen ?

Also, not convinced by "use module name as fallback when no description is provided".
That's taken from the exiisting code for Blocks, and is possibly fine for the Blocks UI use case, but doesn't feel like the generic behavior we want for all cases ?

$definition['category'] can be an object ? Sounds weird - when does this happen ?

It's always an object, just like 'label' and 'description' and all the other translatable keys from a plugin annotation, they're instances of \Drupal\Core\StringTranslation\TranslationWrapper.

Also, not convinced by "use module name as fallback when no description is provided".

That's just a protected helper method, it's not the default, as can be seen in the patch from #2373491: Categorize field type plugins.

1. +++ b/core/lib/Drupal/Component/Plugin/CategorizingPluginManagerInterface.php
   @@ -0,0 +1,50 @@
   +   * Gets sorted definitions.
   ...
   +   *   (optional) The definitions to sort. If omitted, all definitions are used.
   ...
   +   * Gets sorted definitions grouped by category.
   ...
   +   * whereas definitions are sorted by label.
   ...
   +   *   (optional) The definitions to group. If omitted, all definitions are
   

   s/definitions/plugin definitions/

2. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
   @@ -23,17 +23,13 @@
   +  use CategorizingPluginManagerTrait {
   +    getSortedDefinitions as traitGetSortedDefinitions;
   +    getGroupedDefinitions as traitGetGroupedDefinitions;
   +  }
   

   Wow, never seen this before.

   At first: WTF.

   Having read the other hunks in this file: makes sense.

3. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,114 @@
   +  }
   

   Still only supports modules?

4. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,114 @@
   +   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getCategories().
   ...
   +   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getSortedDefinitions().
   ...
   +   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getGroupedDefinitions().
   

   {@inheritdoc}

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,151 @@
   +    }
   +  }
   ...
   +  }
   +}
   ...
   +  }
   +}
   

   Nitpicky nitpick: missing trailing newline.

@a 'fliptable' mateescu : oh, right - thanks, don't mind me :-)

#29.4: {@inheritfoc} does not fit here because a trait cannot implement an interface, so it does not inherit anything. So the direct reference is fine.

So looks RTBC again.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

1. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
   @@ -56,61 +52,27 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +    // Do not display the 'broken' plugin in the UI.
   +    unset($definitions['broken']);
   +    return $definitions;
   

   It is odd that the BlockManager has to take care of that. A concept of a broken plugin is probably a problem in a lot of places. Do we have an issue to solve just that? (I know its out of scope for this issue)

2. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,114 @@
   +  /**
   +   * Gets the name of the module.
   +   *
   +   * @param string $module
   +   *   The machine name of a module.
   +   *
   +   * @return string
   +   *   The human-readable module name if it exists, otherwise the
   +   *   machine-readable module name.
   +   */
   +  protected function getModuleName($module) {
   +    // Gather module data.
   +    if (!isset($this->moduleData)) {
   +      $this->moduleData = system_get_info('module');
   +    }
   +    // If the module exists, return its human-readable name.
   +    if (isset($this->moduleData[$module])) {
   +      return $this->t($this->moduleData[$module]['name']);
   +    }
   +    // Otherwise, return the machine name.
   +    return $module;
   +  }
   

   Isn't that exactly what ModuleHandlerInterface::getName() is supposed to do? ... In case there are modules without a label, we should fix it in there.

3. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,114 @@
   +  /**
   +   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getCategories().
   +   */
   ...
   +  /**
   +   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getSortedDefinitions().
   +   */
   ...
   +  /**
   +   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getGroupedDefinitions().
   +   */
   

   Let's not criple in these old documentation lines and use {@inheritdoc} instead.

4. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -0,0 +1,114 @@
   +    $categories = array_unique(array_values(array_map(function ($definition) {
   

   Maybe this line is worth to have a quick one line documentation.

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,156 @@
   +namespace Drupal\Tests\Core\Plugin {
   

   ... Afaik always did not used the intentation, but gladly we don't need it.

6. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -0,0 +1,156 @@
   +  class CategorizingPluginManagerImpl extends DefaultPluginManager implements CategorizingPluginManagerInterface {
   

   Is Impl. as a name a thing?

1. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -43,31 +36,43 @@ protected function processDefinitionCategory(&$definition) {
   +    // If the class has an injected module handler, use it. Otherwise fall back
   +    // to fetch it from the service container.
   +    if (isset($this->moduleHandler)) {
   +      return $this->moduleHandler;
   +    }
   +    return \Drupal::moduleHandler();
   

   Why not follow our normal pattern?

   if (!$this->moduleHandler) {
     $this->moduleHandler = \Drupal::moduleHandler();
   }
   return $this->moduleHandler;
   

   Then no need for a comment.

2. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
   @@ -56,61 +52,27 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +    // Do not display the 'broken' plugin in the UI.
   +    unset($definitions['broken']);
   ...
        // Do not display the 'broken' plugin in the UI.
   -    unset($definitions['broken']);
   +    unset($definitions[$this->t('Block')]['broken']);
   

   Now we have this twice, once with using a t() call as the array key? :( :( :(

+++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
@@ -0,0 +1,120 @@
+  /**
+   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getCategories().
+   */
...
+  /**
+   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getSortedDefinitions().
+   */
...
+  /**
+   * Implements \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getGroupedDefinitions().
+   */

Can we just fix that quickly?

I'm stupid.

Unblocking contrib is prioritised for beta and having less duplicate code reduces fragility. Committed d3e3d70 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.