Decouple plugin type discovery from plugin discovery

Yay, thanks eff !

+++ b/core/includes/bootstrap.incundefined
@@ -3479,32 +3479,27 @@ function _drupal_shutdown_function() {
-  if (empty($kernels[$scope][$type])) {
-    if (!isset($discovery)) {
-      $discovery = new ConfigDiscovery($scope, $type);
-    }
-
-    $kernels[$scope][$type] = new PluginKernel($discovery);
+  if (!isset($kernels[$scope][$type])) {
+    $type_class = "Drupal\\$scope\\Plugin\\Type\\$type";
+    $kernels[$scope][$type] = new PluginKernel(new $type_class());

I was thinking of registering plugin types through an HTTPKernel event at bootstrap, but that is not possible before the Kernel patch lands. Relying on a hardcoded "Drupal\\$scope\\Plugin\\Type\\$type" namespace pattern is another, possibly better option, but this means :
- $type should be a capitalized string.
- $scope == module, if we want the current registered autoload paths to fire - or the namespace needs to be registered first; where ? event at bootstrap ;-) ? chicken and egg...
That's a restriction, dunno if it's an acceptable one (if yes, should we keep $scope as a variable name ?)

+++ b/core/modules/simpletest/tests/plugins_test.incundefined
@@ -0,0 +1,54 @@
+class plugin implements PluginTypeInterface {

Minor : should be "class pluginType implements ..." ?

+++ b/core/includes/bootstrap.incundefined
@@ -3479,32 +3479,36 @@ function _drupal_shutdown_function() {
+    $scope_namespace = ($scope == 'core') ? 'Core' : $scope;
+    $type_classname = str_replace(' ', '', ucwords(str_replace('_', ' ', $type)));
+    $class = "Drupal\\$scope_namespace\\Plugin\\Type\\$type_classname";

This is some really weird PSR-0 magic... I'm not comfortable with forcing PSR-0 structures and doing magic like this with it. I don't really see the point of doing it anyways, I really think plugin types should provide their own singleton if they have any specialized logic anyways. e.g. cache()

+++ b/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.phpundefined
@@ -18,12 +18,11 @@ use Drupal\Component\Plugin\Derivative\DerivativeInterface;
+  public function __construct(DiscoveryInterface $discovery, $class_key) {
     $this->discovery = $discovery;

This class_key bit is kinda weird. Its leaking implementation details in a weird way.

+++ b/core/lib/Drupal/Component/Plugin/Factory/FactoryInterface.phpundefined
@@ -13,14 +13,6 @@ use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
-   * Construct a plugin factory.
-   *
-   * @param DiscoveryInterface $discovery
-   *   A discovery object for finding out implementation information.
-   */
-  public function __construct(DiscoveryInterface $discovery);
-

This was by design to enforce consistent factory instantiation by the kernel.

+++ b/core/modules/simpletest/tests/plugins.testundefined
@@ -18,36 +15,23 @@ class PluginBaseTestCase extends DrupalUnitTestCase {
-    // Setup a plugin type to test.
-    $discovery = new StaticDiscovery();
-    // Initialize the plugin singleton with a discovery class for our test type.
-    plugin('simpletest', 'plugin', $discovery);
-
-    $discovery->setPluginTypeDefinition(array(
-      'class' => 'class',
-      'factory' => 'Drupal\\Component\\Plugin\\Factory\\DefaultFactory'
-    ));
-    $discovery->setPluginDefinition('coolaid', array(
-      'string' => 'oh yeah',
-      'class' => 'PluginBaseTestPluginClass',

This is subtly different then something that was novel about the previous implementation. Its possible its still doable but it was also proving that with a method like this you could interact with the discovery after the fact and register additional plugins.

+++ b/core/modules/simpletest/tests/plugins.testundefined
@@ -18,36 +15,23 @@ class PluginBaseTestCase extends DrupalUnitTestCase {
+    // @todo After moving the classes of plugin_test.inc to PSR-0 locations,
+    //   remove this line.

This is possible with a test module but requires a full WebTest. To keep the unit test you can call the classloader singleton and register a namespace root. Just de-register it on tearDown.

It feels like we've moved away from the generic enough for anything kernel to specialized logic in the plugin type and that seems like a lose. It does have some useful idea though so I'm going to play with the concept. We should get you commit access so you can make a branch for me to pull some of those comment and other cleanups that aren't related.

I don't know what to think...

#5 committed and pushed. lets see where this goes.

Fixed in plugins-next. Since we've moved to follow up issues marking fixed.

Side note on drupal_object() : problem with such a generic function is that we cannot type hint its return value. Given that this is now the entry point to the plugin system for any external client code willing to instantiate a plugin, not being able rely on IDEs autocompletion for the (longish) method names is unfortunate.

I also vaguely suspect that this very genericity will raise endless bikeshed discussions when a core patch is up for review. Could we aim lower and simply go with a plugin_type($class_name) function, that we can properly hint with PluginTypeInterface ?

Also, after porting field API branch to the current plugins code, I do find that

$formatter = drupal_object('Drupal\field\Plugin\FormatterPluginType')->getPluginInstance($options);

is not too legible :-(.

More seriously, It basically prevents refatcors. If you find you want to rename the plugin type class (or refactor the namespace hierarchy), you'd need to change all the places in code that interact with this plugin type, and some of them might be in 3rd party modules, which you can't know anyway.
This is the same reason why some people (including me) winced when EclipseGC mentioned on IRC he considered storing class names in config data.

So, yay for moving plugin types out of the Discovery class, but I do miss the plugin($scope, $type)->getPluginInstance() indirection...

Well the class name can't be type hinted but we can do tests. We can also call getInstance in the function so that's good. We can't pass arbitrary information to the type constructor though. that means if your type goes though the plugin_type method you can't say pass a DI object into it.

Yeah, a mapping is tedious to collect, but it's a condition for loose coupling. Also, it's really lightweight, since the 'discovery', 'factory', 'mapper' properties have been moved to the runtime plugin type class.

@effulgentsia : BTW, #1538798: Make mappers runtime swappable is a non-issue with a plugin type registry - just pull 'mapper' back into the registry.

How would we register types? Honestly, the fact that they are not registered right now I see as a feature, I've never really been a fan of a singleton every plugin type goes through. I see plugins more as a set of tools for building systems so just passing a class around within that system or providing your own singleton seems reasonable.

How would we register types?

As I proposed earlier in this thread, one possibility is event / listeners early at bootstrap, like other early / critical systems are apparently going to do. Plugins (types) are sufficiently low-level and ubiquitous to deserve such special casing. And it allows for mapper swappability.

Passing classes around is a no-go IMO for the refactor reasons explained above. And leaving it up to each subsystem (that is, each subsystem whose maintainer figured out by himself that letting 3rd party code reference class names directly is wrong) provide some disparate variants around :

function mysubsystem_plugin() {
  return drupal_object('The\Path\To\My\PluginType');
}

// and / or 
function mysubsystem_plugins() {
  return drupal_object('The\Path\To\My\PluginType')->getPluginDefinitions();
}

// and / or
function mysubsystem_get_plugin($options) {
  return drupal_object('The\Path\To\My\PluginType')->getPluginInstance($options);
}

sounds unappealing IMO.

I'd much rather have all code (both internal to a subsystem and 3rd party) standardize on calling plugin('scope', 'type')->theMethod() directly.

I'd much rather have all code (both internal to a subsystem and 3rd party) standardize on calling plugin('scope', 'type')->theMethod() directly.

IMO, there's no really good reason for this, and it places artificial limitations on plugins forcing them to all act the same way when different kinds of plugins, in the real world, often need to behave very differently. This is one of the little things I have always been against other people pushing onto the plugin system, and I remain against it. In fact, I would go so far as to consider the plugin() function to be unnecessary cruft.

I'm struggling with how to respond since I'm apparently not making my point.

Something I can throw out I guess is I would remove drupal_object()/plugin() or any generalized entry point. I had it originally because people wanted it but I planned on proving in our use cases its useless. Since plugin() is gone and drupal_object() is really weird I figure we just get rid of it. As far as the code, there would just be something like this:

// my system needs a singleton so here it is!
function mysubsystem($options) {
  $foo = drupal_static(__FUNCTION__);
  if (/*cachelogic*/) {
    $foo[$cid] = new MyPluginType($injection)->getPluginInstance($options);
  }

  return $foo[$cid];
}

The large amount of hand-waving and incomplete code there is indicative of the fact that all this logic is very specific to the system and can't/shouldn't be generalized.

Would it make sense to start without any plugin(), plugin_type(), or drupal_object() function, use something like the code above for Field API, and then see what core looks like after we have a bunch of plugin types, and decide then if we want to add some global plugin system function?

OK, works for me, I guess.

Not forcing the use of a standard entry point on all plugin types sounds legitimate, but having a standard that should fit 80% is still worthy IMO - even the standard is "expose yourself to drupal_container()", which is fine by me.

Do we need the wrapper to do the tests? Seems like we can just new then keep the unit testing.

A unit test can/should call new directly, and inject whatever mocked objects are appropriate.

Integration tests can/should use the higher-level front-end routines, to test that they all work correctly.

+++ b/core/modules/system/tests/plugins.test
@@ -1,63 +1,41 @@
+  protected $test_plugin_type;

This should be camelCase.

Otherwise I'm all down for getting rid of drupal_object(), as it's a horrid idea. :-) I haven't been following recent developments closely enough to feel comfortable RTBCing this, but I saw nothing in #32 specifically that made me freak out.

I left the abstract test in because I was using that for the derivative test I'd been working on and I'd hoped that sort of logic would/could make it into simpletest someday and the tests where failing with PDOExceptions with DrupalUnitTestCase. Also, the one-off loader means that namespace won't be loadable in other tests which is possibly desirable. I liked your docs though so I took them. :)

Also made the camelCase change.

I think the is done now!