Spin-off from #1512948-3: Convert one simple core subsystem to use plugin system as part of the initial patch. This is still a work in progress, but wanted to post my thoughts here so people can review it and tell me how wrong it is. I think it might be a nice way though to simplify parts of the plugin system. I'll post more here when I can demonstrate that simplification.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

This is a patch that needs to be applied on top of D8 HEAD plus #1497230-70: Use Dependency Injection to handle object definitions . It is not rolled against any of the work in this sandbox. I'm just using this project for the issue, since it's kind of related to plugins. Future patches will incorporate the plugins work happening in this project.

effulgentsia’s picture

Here's a bit more. It includes what I found as the essential functionality of the plugin system from the plugins-kernel branch. As per #1, this is rolled against D8 HEAD plus the dependency injection patch.

This doesn't implement "derivative plugins" yet, because I'm not sure yet if that belongs in a base class, or if that should be handled via subclasses. Note that because of this patch's use of Symfony's service container, subclasses to handle stuff like derivative plugins can be easily added (I think), so I'm tempted to leave that off of this initial patch, until we're ready to submit something that needs it (Blocks, Fields) to the core issue queue.

This code is still largely untested. My next step is to try to rework image effects to use it, and see if any of this actually works.

effulgentsia’s picture

Title: Extend Symfony's service container and use that instead of plugins for simple swappable systems » Extend Symfony's service container and use that for simple swappable systems and plugin access and discovery
effulgentsia’s picture

+++ b/core/includes/bootstrap.inc
@@ -2152,6 +2152,22 @@ function _drupal_bootstrap_configuration() {
+    ->setFactoryService('plugin.plugin.%plugin.scope%.%plugin.type%')

This should be setFactoryService('plugin.type.%plugin.scope%.%plugin.type%'). The next patch I post here will have that fix.

effulgentsia’s picture

Status: Needs review » Needs work
FileSize
48.81 KB

Here it is with image effects converted (with some @todos still, but I think what's here is enough to demonstrate the plugin system at work).

effulgentsia’s picture

Status: Needs work » Needs review

This is ready for review now. It will certainly fail bot, but here's hoping bot won't even try on a sandbox project.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
48.12 KB

This patch adds a plugin() function wrapper to drupal_container()->blah()->blah()->blah().

effulgentsia’s picture

This restores the decoupling of Discovery from other classes, as is in the plugins-kernel branch.

This now shares a lot of architectural similarities with that branch. The key differences with that branch are:

  • The use of Symfony's container for making all plugin system classes swappable.
  • The change from a single PluginKernel class to PluginType and Plugin classes.
  • The removal of "mappers", since I think Symfony's container makes them unnecessary.
  • The removal of "derivative plugins", since I think that should be deferred to follow-up work as per #2.
  • The inclusion of cache system (using Symfony's container, not plugins) and image effects system (using plugins) conversions to demonstrate the canonical use-cases of each system.
neclimdul’s picture

I'm still digesting this and it seems clever and all but I'm having a hard time figuring out why you wrote this.

effulgentsia’s picture

This has a few more minor changes.

I'm having a hard time figuring out why you wrote this

I'll try to find you on IRC today so we can chat about it.

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2152,6 +2153,14 @@ function _drupal_bootstrap_configuration() {
+  // Register fallback implementations for the cache and plugin systems.
+  drupal_container()->registerFallback('cache.{cache.bin}', 'Drupal\Core\Cache\DatabaseBackend')
+    ->setArguments(array('%cache.bin%'));
+  drupal_container()->registerFallback('plugin.discovery.{plugin.scope}.{plugin.type}', 'Drupal\Core\Plugin\DefinitionDiscovery\DefinitionDiscoveryFromConfig')
+    ->setArguments(array('plugin.type.%plugin.scope%.%plugin.type%', 'plugin.plugin.%plugin.scope%.%plugin.type%'));
+  drupal_container()->registerFallback('plugin.type.{plugin.scope}.{plugin.type}', 'Drupal\Component\Plugin\PluginType')
+    ->setArguments(array('%plugin.scope%', '%plugin.type%', new Reference('plugin.discovery.%plugin.scope%.%plugin.type%')));

This might be the wrong place to stick the registration of the plugins themselves. We're doing the same thing in install.core.inc too. Not sure why. Once we have the Kernel, we could have an event dispatcher do it for us, so I suppose for now this is where we'd want it. Would really like Composer's autoload.php to handle that for us, but that's over at #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo).

+++ b/core/lib/Drupal/Component/DependencyInjection/ContainerBuilder.phpundefined
@@ -0,0 +1,128 @@
+  protected function findFallback($id) {
+    // @todo Optimize this by structuring $this->fallbacks to be more
+    //   efficiently searched.
+    // @todo Add best fit logic if there are multiple matches. For example, if
+    //   $id = 'a.b.c' and there are two fallbacks: 'a.{foo}.{bar}' and
+    //   'a.b.{bar}', the latter (more specific) one should be returned.
+    foreach ($this->fallbacks as $fallback => $definition) {
+      $regex = '/' . preg_replace('/\\\{.*?\\\}/', '\\w+', preg_quote($fallback, '/')) . '/';
+      if (preg_match($regex, $id)) {
+        return $fallback;
+      }
+    }

I understand there is a @todo to clean this up a bit, but if we opt to avoiding the use of regex here, we will gain some performance benefit as it wouldn't have to re-compile it each call. String function calls are in some cases faster than regex. Can be easier to debug too.

+++ b/core/lib/Drupal/Core/Config/DrupalVerifiedStorageSQL.phpundefined
@@ -48,6 +48,6 @@ class DrupalVerifiedStorageSQL extends DrupalConfigVerifiedStorage {
   static public function getNamesWithPrefix($prefix = '') {
-    return db_query('SELECT name FROM {config} WHERE name LIKE :name', array(':name' => db_like($prefix) . '%'))->fetchCol();
+    return db_query('SELECT name FROM {config} WHERE name LIKE :name ORDER BY name', array(':name' => db_like($prefix) . '%'))->fetchCol();

Is this ORDER BY suppose to be here?

Note that the ContainerBuilder also comes with alias and extension support, which make the ContainerBuilder more of a hub of awesome. There's also the Variable class, which could act as a plugin class map too, in place of the Definition.

drupal_container()->register('cache.bin', 'Variable')->setArguments(array('Drupal\Cache\MyCache'));
$class = drupal_container()->get('cache.bin');
return new $class();
RobLoach’s picture

Didn't mean to change status.

effulgentsia’s picture

Title: Extend Symfony's service container and use that for simple swappable systems and plugin access and discovery » [Meta] Extend Symfony's service container and use that for simple swappable systems and plugin access and discovery
Assigned: Unassigned » effulgentsia
Status: Needs work » Needs review

I discussed this patch with neclimdul and yched yesterday, and in addition to them giving some good feedback which I need to write up at some point, we agreed that this needs to be broken up into independent issues against either Symfony, core, or plugins-kernel as appropriate. I'll do that as I have a chance, and marking this a meta issue in the meantime. Still leaving as "needs review" in case anyone wants to provide more feedback.

Is this ORDER BY suppose to be here?

I believe it makes sense for config to return its objects in a predictable order, but I'll file that as a separate issue against the config system. Image module currently implements its own sort by name, which I wanted to remove, which is why I put it into this patch.

There's also the Variable class

Sweet. Thanks for the tip.

effulgentsia’s picture

Well, lsmith77 and stof from Symfony didn't like the idea of runtime fallback resolution (https://github.com/symfony/symfony/issues/3875), because Symfony's container is geared towards being compilable, so I have some more learning and thinking to do on that. I don't yet know if we're planning on Container compilation in Drupal, but something worth keeping in mind.

Crell’s picture

I would like to compile it if at all possible for performance. Otherwise the setup costs could be quite unwieldy. I just don't know how we're going to do that yet. :-)

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs review » Closed (works as designed)

I don't think there's anything left to do here. Based on feedback from merlinofchaos, the plugins-next branch now uses the approach of plugin types as non-swappable classes, and http://drupal.org/project/issues/1441840?component=Plugins contains the remaining open issues for getting the plugin system finished (that list might not be complete, but I think it's close).

I'm still curious about how we should deal with drupal_container() compilation, but we can tackle that in the core issue queue as Crell, I, or anyone else has ideas worth posting. In any case, I agree that the ability to compile is desirable, so this issue's suggestion of extending ContainerBuilder with runtime fallback logic isn't right.

We still have the problem of needing to figure out swappable cache mapping (which possibly extends to swappable plugin mappers in general), so if you're interested in that, please give feedback in #1538798: Make mappers runtime swappable.