Introduce a service that returns filtered plugin definitions

+++ b/core/lib/Drupal/Core/Plugin/PluginDefinitionRepository.php
@@ -0,0 +1,60 @@
+  public function get($type, $consumer, DiscoveryInterface $discovery, array $filters = [], array $context = []) {
+    $hook[] = "plugin_filter_{$type}";
+    $hook[] = "plugin_filter_{$type}__{$consumer}";

Tim and I discussed a bunch of this code earlier and we has a use_case kind of string instead of a $type/consumer. I still think that's probably a good approach and reduces the number of parameters.

That said I 10000000% support the addition of this to core. We've needed plugin filtering for a long time, and while I've attempted to do this at times in other code, this is a much better total approach.

Eclipse

Tim and I discussed this, and he convinced me that my objection isn't warranted. On top of that, the test looks good but there are a lot of docblocks that need addressing yet.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextFilter.php
   @@ -0,0 +1,34 @@
   + * @todo.
   

   Let's do

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextFilter.php
   @@ -0,0 +1,34 @@
   +  public static function getFilter(array $contexts) {
   ...
   +  public function filter(array $definitions) {
   
   +++ b/core/lib/Drupal/Core/Plugin/PluginDefinitionRepository.php
   @@ -0,0 +1,60 @@
   +/**
   + * @todo.
   + */
   

   Need docs

3. +++ b/core/lib/Drupal/Core/Plugin/PluginDefinitionRepository.php
   @@ -0,0 +1,60 @@
   +class PluginDefinitionRepository {
   

   This isn't really a repository. It's more of a "Filterer" but I'm not sure that's a good name either. I know I used "Mutator" for anything that changes the list of definitions in my plugin rewrite. I'm not certain that's good either. PluginDefinitionFilterCollector? I dunno

4. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,45 @@
   +/**
   + * @addtogroup hooks
   + * @{
   + */
   

   This doesn't look right, but I've not messed with an api file in a while.

I addressed #6. I ended up renaming "PluginDefinitionRepository" to "DiscoveryFilterer", since to me that accurately described what was going on. What do people think of that?

Here's a tagged services approach.
Any preferences for this over hooks?

I think to answer this we need to think of the 80% use case - what's the most common operation for filtering plugin definitions? In my experience I've maintained a hard-coded list of definitions I want to hide (unset from the list), but others may have more complex filtering needs. Can we think of examples of complex filtering use cases that would do better as services?

From a DX-perspective I think this is better than a hook.

• It's object-oriented, well organised code v.s. a 900+ line .module file)
• You can write unit tests easier
• It's more reusable
• You can override existing discovery filters
• You can give filters priority, this is useful if you would like to keep definitions organised in a certain way and you need to ensure other filters are run first, though the name filterer can be a bit misleading, if you are allowed to move the definitions around (or even add new ones)

I have to admit I like the hook version as well, looking from a themer experience. Themes can implement this in the theme_name.theme and optionally maybe use theme settings to configure the visibility without having to create a module for it. But I'm fine with whatever solution goes in, as long something goes in :)

1. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,66 @@
   +   *   provided by the code requesting the filtered definitins.
   

   Spelling definitins

2. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,66 @@
   +    $hook[] = "plugin_filter_{$type}";
   +    $hook[] = "plugin_filter_{$type}__{$consumer}";
   +    $this->moduleHandler->alter($hook, $filters, $context);
   

   s/$hook/$hooks

3. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,66 @@
   +      $definitions = call_user_func($filter, $definitions);
   

   Should also be sending the $context to the callables?

   Then a module could use the same callable but have logic based on the context.

4. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,49 @@
   + * @param Callable[] $filters
   + *   An array of callables to filter the definitions.
   

   I am not real clear why the hook was written this way.

   It seems weird to me provide a hook so that modules can provide an "array of callables". If we are going to use hooks why not call the hook after the definitions are collected and then the hooks themselves would alter the list?

   Is the pattern in the current patch used in core elsewhere I could look at?

5. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,49 @@
   + *   requesting the filtered definitins.
   

   Spelling definitins

6. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,49 @@
   + *   requesting the filtered definitins.
   

   Spelling definitins

7. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,49 @@
   +  $filters[] = function ($definitions) {
   +    // Explicitly remove the "Help" blocks from the list.
   +    unset($definitions['help_block']);
   +    return $definitions;
   

   Since *.api.php files are good way to show examples of how hooks could be used should we use a different example here from above?

8. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -42,7 +54,8 @@ public function __construct(BlockManagerInterface $block_manager) {
   @@ -63,8 +76,10 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   
   @@ -63,8 +76,10 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
        $build['#type'] = 'container';
        $build['#attributes']['class'][] = 'block-categories';
    
   -    $definitions = $this->blockManager->getDefinitionsForContexts($this->getAvailableContexts($section_storage));
   -    foreach ($this->blockManager->getGroupedDefinitions($definitions) as $category => $blocks) {
   +    $filters[] = ContextFilter::getFilter($this->getAvailableContexts($section_storage));
   +    $definitions = $this->discoveryFilterer->get('block', 'layout_builder', $this->blockManager, $filters, ['section_storage' => $section_storage]);
   

   Is there reason we aren't passing $delta and $region in $context?

   It seems this would give more flexibility to contrib especially to not allow certain blocks in certain regions. Not sure about delta.

9. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAPITest.php
   @@ -0,0 +1,75 @@
   +class LayoutBuilderAPITest extends BrowserTestBase {
   

   We should also have generic test for the DiscoveryFilterer class working with the hooks.

Here are some observations.

1. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,90 @@
   +    $hooks[] = "plugin_filter_{$type}__{$consumer}";
   

   I don't think we use __ pattern in core anywhere.

2. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,55 @@
   + * Alter the filtering of plugin definitions for a specific type.
   

   Let's explain TYPE a little with example.

3. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,55 @@
   + * Alter the filtering of plugin definitions for a specific type and consumer.
   

   Let's explain TYPE and CONSUMER a little with example.

The attached explains the hooks' usage of TYPE & CONSUMER with examples.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextAwarePluginManagerInterface.php
   @@ -20,6 +20,8 @@
   +   * @deprecated Use \Drupal\Core\Plugin\DiscoveryFilterer instead.
       */
      public function getDefinitionsForContexts(array $contexts = []);
   

   Should we also deprecate \Drupal\Core\Plugin\Context\ContextAwarePluginManagerTrait?

2. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,90 @@
   + * This allows modules to alter plugin definitions, which is useful for tasks
   

   Should we say "allows modules and themes"? Not sure if this is just assumed.

3. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,90 @@
   +   * Gets the plugin definitions for a given type and sorts and filters them.
   

   I am not sure where the definitions are getting sorted.

   Neither \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions nor \Drupal\Core\Plugin\Context\ContextHandlerInterface::filterPluginDefinitionsByContexts mention sorting in the docs.

4. +++ b/core/lib/Drupal/Core/Plugin/DiscoveryFilterer.php
   @@ -0,0 +1,90 @@
   +   *   (optional) An array of contexts.
   

   We should add something here about if the contexts are provide they will used to filter.

5. +++ b/core/lib/Drupal/Core/Plugin/plugin.api.php
   @@ -0,0 +1,68 @@
   +function hook_plugin_filter_TYPE_alter(array &$definitions, array $extra, $consumer) {
   +  // Remove the "Help" block from the Block UI list.
   +  if ($consumer == 'block_ui') {
   +    unset($definitions['help_block']);
   +  }
   

   I like that change in hook allows the example hooks to be much simpler.

6. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   @@ -102,7 +114,7 @@ public function listBlocks(Request $request, $theme) {
   -    $definitions = $this->blockManager->getDefinitionsForContexts($this->contextRepository->getAvailableContexts());
   +    $definitions = $this->discoveryFilterer->get('block', 'block_ui', $this->blockManager, $this->contextRepository->getAvailableContexts(), ['theme' => $theme]);
   

   Further down in this method we have
   $region = $request->query->get('region');

   It seems like it would make sense to move up so that $region if available could be added to $extra

   This would allow a module disallow certain block within certain regions on the main block admin page.

7. +++ b/core/modules/layout_builder/src/Controller/ChooseSectionController.php
   @@ -43,7 +54,8 @@ public function __construct(LayoutPluginManagerInterface $layout_manager) {
   @@ -62,7 +74,8 @@ public function build(SectionStorageInterface $section_storage, $delta) {
   
   @@ -62,7 +74,8 @@ public function build(SectionStorageInterface $section_storage, $delta) {
        $output['#title'] = $this->t('Choose a layout');
    
        $items = [];
   -    foreach ($this->layoutManager->getDefinitions() as $plugin_id => $definition) {
   +    $definitions = $this->discoveryFilterer->get('layout', 'layout_builder', $this->layoutManager, [], ['section_storage' => $section_storage]);
   

   I was going to suggest that we pass $delta in $extra here with idea that it would allow say restricting a "header" layout to only be added at the top of the page.

   But then I realized you can still reorder the sections(correct?) once they are placed so would not be effective.

   So unless anyone else can think of another reason for passing $delta it seems find the way it is.

8. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -0,0 +1,29 @@
   +  foreach ($definitions as $plugin_id => $definition) {
   +    // Field block IDs are in the form 'field_block:{entity}:{bundle}:{name}',
   +    // for example 'field_block:node:article:revision_timestamp'.
   +    preg_match('/field_block:.*:.*:(.*)/', $plugin_id, $parts);
   +    if (isset($parts[1]) && in_array($parts[1], $disallowed_fields, TRUE)) {
   +      // Unset any field blocks that match our predefined list.
   +      unset($definitions[$plugin_id]);
   +    }
   +  }
   

   I know this is just test code but it seems overly complicated for the test.

   Test modules also can serve as ways to learn the system and this seem to bring unnecessary complexity that makes it harder to under what is possible.

   It could be simplified as

     $disallowed_plugins = [
       'field_block:node:bundle_with_section_field:sticky',
       'help_block',
     ];
   
     foreach ($disallowed_plugins as $disallowed_plugin) {
       unset($definitions[$disallowed_plugin]);
     }
   

Looks good to me!

We should change record about the new hooks.

@tim.plunkett thanks for the CR
Assuming the tests pass I am +1 for RTBC

Assigning to @EclipseGc as he said he was going to take a look.

uhhh yes! Tim and I walked through this together, and plugin filtering is a thing we've needed since like... day 2 of D8, so yeah this is good to have in general and gives us some really amazing possibilities for Layout Builder as the patch is right now.

RTBC

Eclipse

CR needs to mention about deprecated methods and trait
That could be done in follow-up but we have policy for that https://www.drupal.org/core/deprecation

Maybe this could be follow ups but...

1. I am wondering if we want to add a @see to \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions() that points to \Drupal\Core\Plugin\PluginDefinitionFilterer::get() that indicates that if you displaying the plugins to the UI you should usually use PluginDefinitionFilterer::get()

   Otherwise I wondering how coders would find this new functionality.

   If am a developer and a write plugin types that I am going expose in a form and if the plugin type should be filtered by context I will find the new functionality because \Drupal\Core\Plugin\Context\ContextAwarePluginManagerInterface::getDefinitionsForContexts has been deprecated and points to PluginDefinitionFiltererInterface

   On the other hand if I am writing a plugin type that is not filtered by context, like image effects, then I would use \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions to retrieve the definitions when I want to display a list of plugins.

   For instance in \Drupal\image\Form\ImageStyleEditForm::form has

   $effects = $this->imageEffectManager->getDefinitions();
       uasort($effects, function ($a, $b) {
         return Unicode::strcasecmp($a['label'], $b['label']);
       });
       foreach ($effects as $effect => $definition) {
         $new_effect_options[$effect] = $definition['label'];
       }
   

   If a module wanted to limited the list available plugins here, for instance to only only instance of scaling plugin per image style, then they would have use a form alter. But if the code used PluginDefinitionFilterer::get() then the new hooks could be used to filter in this situation(provided enough metadata was passed in $extra.

2. Should we have a follow up to determine where core should replace calls to \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions() with \Drupal\Core\Plugin\PluginDefinitionFilterer::get()
   One example would be the ImageStyleEditForm example above.

1. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
   @@ -37,10 +37,17 @@ class BlockManager extends DefaultPluginManager implements BlockManagerInterface
   +  protected function getType() {
   +    return 'block';
   
   +++ b/core/lib/Drupal/Core/Condition/ConditionManager.php
   @@ -43,6 +44,13 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +  protected function getType() {
   +    return 'condition';
   
   +++ b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
   @@ -42,8 +45,16 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +  protected function getType() {
   +    return 'layout';
   

   why not to use class variable?

2. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -0,0 +1,65 @@
   +    return \Drupal::service('module_handler');
   ...
   +    return \Drupal::service('theme.manager');
   

   why not to add protected vars for static cache?

Looking really good! I like the changes seems like it is obvious on how to use the new logic.

Setting to "Needs work" but only for comment changes I mention below. But also could probably be talked out of them if others think they aren't needed.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -17,7 +17,9 @@ class ContextHandler implements ContextHandlerInterface {
   -      if (!isset($plugin_definition['context'])) {
   +      // @todo Support object-based plugin definitions in
   +      //   https://www.drupal.org/project/drupal/issues/2961822.
   +      if (!is_array($plugin_definition) || !isset($plugin_definition['context'])) {
   

   I was confused as why this patch need this change.

   Seems that changes to \Drupal\layout_builder\Controller\ChooseSectionController::build() made this necessary.

   But isn't strictly related to the new interface because we would have run into the same problem even without the patch if LayoutPluginManagerInterface extended ContextAwarePluginManagerInterface.

   Correct?

2. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerInterface.php
   @@ -0,0 +1,29 @@
   +  /**
   +   * Gets the plugin definitions for a given type and consumer and filters them.
   +   *
   +   * @param string $consumer
   +   *   A string identifying the consumer of these plugin definitions.
   +   * @param \Drupal\Component\Plugin\Context\ContextInterface[]|null $contexts
   +   *   (optional) Either an array of contexts to use for filtering, or NULL to
   +   *   not filter by contexts.
   +   * @param mixed[] $extra
   +   *   (optional) An associative array containing additional information
   +   *   provided by the code requesting the filtered definitions.
   +   *
   +   * @return \Drupal\Component\Plugin\Definition\PluginDefinitionInterface[]|array[]
   +   *   An array of plugin definitions that are filtered.
   +   */
   +  public function getFilteredDefinitions($consumer, $contexts = NULL, array $extra = []);
   

   Should be put @see here or anywhere in this interface about the new hooks? Others reading this you might assume the filter is only context filter and not take into consideration the hooks.

   Looking back at #21.2 it looks like we had a comment of "which is useful for tasks" has to why you would want to get filtered plugins. which I am not sure is anywhere in the new version.

3. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerInterface.php
   @@ -0,0 +1,29 @@
   +   * @param string $consumer
   +   *   A string identifying the consumer of these plugin definitions.
   

   Should we say "usually a module or theme" or is it always? I guess I profile could implement this too? An extension short name?

This looks good to me now. I am +1 for RTBC.

Re #31

CR needs to mention about deprecated methods and trait

The patch no longer deprecates these.

@EclipseGc RTBC'ed in #28 but the change is very different now so maybe he should take another look.

This looks really good actually. My only misgiving is that we're hard-coding consumer ids, which will make it difficult to build new UIs later for limiting this stuff in a UI, but I'm unconvinced that's a real use case given the nuance in this code, and also seems like the sort of thing we could change at some later date.

I'm rtbcing this.

Eclipse

Great

+++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
@@ -0,0 +1,29 @@
+function layout_builder_test_plugin_filter_block__layout_builder_alter(array &$definitions) {

Nit should this function have the argument array $extra even if it isn't used?

1. +++ b/core/lib/Drupal/Core/Block/BlockManagerInterface.php
   @@ -4,10 +4,11 @@
   +interface BlockManagerInterface extends ContextAwarePluginManagerInterface, CategorizingPluginManagerInterface, FilteredPluginManagerInterface {
   

   this is a BC break, should we instead just have BlockManager implement it direct? Then there is no BC break.

   I know its unlikely that someone has an implementation of a block manager that doesn't extend the existing one, but it is allowed, so unless I'm mistaken, I think we have to honour it.

2. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -0,0 +1,65 @@
   +    return \Drupal::service('module_handler');
   

   Because most plugin managers will extend from DefaultPluginManager, should we check for presence of $this->moduleHandler first here and avoid the call to the container?

Agree with #47.1 that it seem like we are able add methods via adding the interface.

#47.2 also seems like good fix.

I'll discuss the impact of the new method, in particular whether it impact ability to backport, with the other framework managers

Consensus was this isn't eligible for backport, because of the new methods etc.

Adding review credits

Committed e91d969 and pushed to 8.6.x

Published change record