Write a hook based plugin discovery implementation.

Here ya go. This is rebased against plugins-kernel with #1529162-2: Decouple plugin type discovery from plugin discovery applied, so make sure to apply that patch first if you want to commit or review in context.

Here's a reroll for plugins-next. I have no objection at all with you committing your version instead of this, but I think we're all settled on discovery, so I don't think there's a reason for this issue to stay postponed.

Incidentally, I did a search on HEAD for how many hook_*_info() hooks we have in core, and it's a couple dozen: more than I realized. So once a plugin system is in core, we'll have an opportunity to use it a lot: tokens, filters, language types, language negotiation, openid, archiver, updater, and more.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.phpundefined
@@ -0,0 +1,54 @@
+  protected $plugins;

I think "plugin" currently designates "an instantiated plugin object. This var stores $definitions (method name is getPluginDefinition(), for that matter).

+++ b/core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.phpundefined
@@ -0,0 +1,54 @@
+      if (isset($this->cache_key) && $cache = cache()->get($this->cache_key)) {

This assumes all cache entries will live in the same cache bin (the main 'cache' bin, currently) ? "All info hooks in the same bin for all plugin types" might be good enough, but then should this be 'cache', or some dedicated 'cache_plugin' ?

Related : how can outside code trigger a cache refresh ? Persistent cache is easy (bin is hardcoded and cid is known by the plugin type), but the HookDiscovery class also acts as a static cache. In the current architecture, clearing that one means means ditching the PluginType object stored in drupal_object(), which is not doable right now.
Or : PluginTypeInterface needs a resetDiscovery() method ?

+++ b/core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.phpundefined
@@ -0,0 +1,54 @@
+        $plugins = module_invoke_all($this->hook);
+        if (isset($this->before_alter)) {
+          $plugins = call_user_func($this->before_alter, $plugins);
+        }

I like the before_alter callback, since I'll need to fill-in missing properties on each definition.

One thing that the current code doesn't allow, is tracking which module provides which definition (I need this for 'field type' plugins). This information is lost after module_invoke_all(), this would rather require something like :

foreach (module_implements($this->hook) as $module) {
  $plugins = (array) module_invoke($module, $this->hook);
  foreach ($plugins as $plugin_id => $definition) {
    $definition['module'] = $module;
    $this->plugins[$plugin_id] = $definition;
  }
}

+1 for the $this->defaults code that went got added in plugins-next.

I guess this makes the 'before_alter' property moot.

Updated patch on top of #1540206: Allow self inspection on instantiated plugin objects (the patch in there fixes processPluginDefinition()).

Fixes most my remarks in #4 :
- Adds $definition['module'] = $module; in HookDiscovery::getPluginDefinitions()
- Adds $plugin_id as an argument for processPluginDefinition() (to allow $definition['name'] = $plugin_id)
- Removes the 'before_alter' callback; moves the ksort on image effects, does it on the calling side in the image effects UI
- Adds a HookDiscovery::clear() to flush persistent and static caches.
Right now as a method on the Discovery class only and without a proxy method on the PluginType, it's not accessible from the outside. Not sure what to do with that yet. Adding a clearPluginDefinitions() method on the PluginType makes no sense for the current other discovery classes, that do not cache anything - although they should ?

+++ b/core/modules/image/image.admin.inc
@@ -94,8 +94,10 @@ function image_style_form($form, &$form_state, $style) {
+  $image_effects = image_effect_definitions();
+  ksort($image_effects);

Maybe we should move the ksort() into image_effect_definitions()? In a follow-up, we'll need to figure out if we want HookDiscovery to support a sort() callback, or a more generic beforeAlter() or afterAlter() callback, since processDefinition() runs on a single definition and therefore can't sort definitions. But that follow-up can happen after an initial core patch lands.

+++ b/core/modules/image/image.module
@@ -845,41 +855,11 @@ function image_style_path($style_name, $uri) {
+  return drupal_object('Drupal\image\Effect')->getPluginDefinitions();

There's no drupal_object() any more.

Adding a clearPluginDefinitions() method on the PluginType makes no sense for the current other discovery classes, that do not cache anything - although they should ?

I think for now, we can leave it a per-discovery-class and per-type-class decision for if and how to deal with internal cache clearing. Perhaps in a follow-up we can discuss whether to add an interface for or extend existing interfaces to standardize cache clearing. In any case, good call on adding HookDiscovery::clear().

The ksort IMO has no reason whatsoever to be part of image_effect_definitions(). It's only used for the order of options in the UI select dropdown (arguably, IMO - I don't see how sorting on machine name helps UX). So I'd say it belongs the the corresponding UI code.
I don't think that we should support the idea that the list of implementations of a given plugin type has one good natural order. Ordering, if relevant, is ad-hoc IMO, not inherent to the list itself.
Not sure we have other cases for full-list-only massaging of the definitions ?

for now, we can leave it a per-discovery-class and per-type-class decision for if and how to deal with internal cache clearing
Agreed, but currently there is no way to call HookDiscovery::clear() from the outside. PluginType doesn't let you access the discovery directly, and clear() is not part of PluginTypeInterface.

The ksort IMO has no reason whatsoever to be part of image_effect_definitions()

Yep. Good reasoning.

I don't think that we should support the idea that the list of implementations of a given plugin type has one good natural order.

Yep. I also agree. Or in any case, the plugin type can provide sort function(s) as appropriate. So, we can have a Drupal\image\Effect::sort() function or Drupal\image\Effect::sortByName()/Drupal\image\Effect::sortBySomethingElse() functions. We don't need to add this to this patch, just agreeing that it's not the plugin system's job to deal with sorting.

there is no way to call HookDiscovery::clear() from the outside

What I mean by per-type-class decision is Drupal\image\Effect can provide a clear() method that calls $this->discovery->clear(). If the "outside" wants to clear image effect caches, it knows it's working with a Drupal\image\Effect object, not just a PluginTypeInterface, so it can call that method. Eventually, I suspect we'll want to add something to PluginType and/or PluginTypeInterface, but I'd rather not derail on that, and instead come back to it once we have some core subsystems converted so we can see where there's redundancies to optimize.

Drupal\image\Effect can provide a clear() method that calls $this->discovery->clear(). If the "outside" wants to clear image effect caches, it knows it's working with a Drupal\image\Effect object, not just a PluginTypeInterface, so it can call that method.

Works for me - this might mean that some surrounding code gets type-hinted as Drupal\image\Effect rather than PluginInterface, though, which violates some standards.
Although those standards are probably not strict ones; at some point, you have to know you work with a specific plugin type - and the indirection between [$scope, $type] and PluginTypeClass was abandoned, so a plugin type *is* a given class.

So, fine :-)

Then it means every plugin type that uses HookDiscovery must implement a clear() method on its own ?

On it.

This decouples from #1540206: Allow self inspection on instantiated plugin objects. yched's points in both these issues are valid, but we can start with this, and improve in the other issue. I don't have image style tests passing though, so still troubleshooting that.

Yay. "Image styles and effects UI configuration" tests pass.

Do we need to use the DI Container? Meh, it works just fine and avoids a global pretty effectively.

If you prefer this one, commit this one. If you prefer #19, commit that one. Your choice.

I forgot to unassign myself from this when I posted a working patch in #16. Better late than never.

we can replace it with the "cache any discovery" system we've discussed some other time

Inspired by #1613638: Use decoration instead of a base class for derivative discovery, here's an implementation of cache as decorator. Leaving at RTBC, because if you don't like this, you can commit #19 or #20 instead.