The Drupal 8 Plugin system is largely in a state that it could be committed to core: #1497366: Introduce Plugin System to Core however, there are some concerns that the plugin system may not be particularly obvious from a DX use case. Those of use who have developed the plugin system thus far feel quite the opposite, and so we have tried to put together a simple use case to demonstrate the use of plugins. We need some people who are willing to dig in here a bit and give some serious feedback. This issue is the first of two.

Within this issue we want feedback on building individual plugins.

Documentation of the plugins system can be found here: http://drupal.org/node/1637614 it is not complete, but should give a good idea of the innards of the system.

The plugin system can be checked out as a full Drupal 8 install by executing the following command:

git clone --recursive --branch plugins-next http://git.drupal.org/sandbox/eclipsegc/1441840.git drupal
git checkout 3bc376e5a2f8f6a376560f48745ab6d95a5c2834

The plugins example module for this can be placed in sites/all/modules
git clone --recursive --branch info_hooks http://git.drupal.org/sandbox/eclipsegc/1663586.git msg_plugin

You will find 3 modules in this repository.

  1. msg_plugin: the base module which defines the plugin type, and a couple of plugins.
  2. msg_plugin_extras: a module that ONLY implements plugins (it has 2 plugins within it and the appropriate hook to inform the system about them)
  3. msg_plugin_custom: a module which implements a derivatives base plugin.

Looking at the msg_plugin_extras module should give a VERY good idea of what is actually required to write new plugins for this module. The plugins are intentionally simplistic. A simple "this makes sense" or "this doesn't make sense" is probably all we really need here. As many people giving feedback as possible would be nice. The task is simple, and there are serious questions on this topic right now, so PLEASE take some time to review this.

As a side note: the PSR-0 directory structure is needlessly complex. This is on purpose because the mechanism for discovering these plugins is currently a hook, but we would like to do this discovery via annotations on the plugin classes themselves, which would actually allow us to remove the hooks for the modules entirely. This directory structure is representative of the annotation based discovery. If you'd like to play with that solution as well, then you can look at these branches on the same repositories.

Drupal 8 with Plugins & Annotations:
git clone --recursive --branch plugins-annotations http://git.drupal.org/sandbox/eclipsegc/1441840.git drupal

Plugin Example Module with Annotated Plugins instead of Hooks:
git clone --recursive --branch 8.x-1.x http://git.drupal.org/sandbox/eclipsegc/1663586.git msg_plugin

We need feedback on plugin type building on this issue, if you'd like to give feedback on plugin building, please check #1676202: DX: D8 Plugin Type writing

Eclipse

Files: 

Comments

Tor Arne Thune’s picture

Apparently there's no remote branch called info_hooks.
warning: Remote branch info_hooks not found in upstream origin, using HEAD instead

EclipseGc’s picture

check again, I copied and pasted exactly what is in the post and it works for me.

Tor Arne Thune’s picture

Issue summary: View changes

Correcting links.

Tor Arne Thune’s picture

EDIT: My mistake, sorry. I was pulling from the same sandbox as for the plugin system.

Adagio’s picture

I'm on the 8.x-1.x branch.

Love annotations, love directory structure making it easy to see both plugin types and implementations.

As a best practice I think I'd prefer if there was a directory reserved for plugin implementations. Imagening many folders for different plugins, the "Type" and "Derivative" etc folders would kinda be in the middle of those which feels a bit unstructured.

I'd expect plugin type authors to provide an abstract class for plugin implementations for structure.

As a plugin implementer, I would prefer to implement derivatives as static methods on the plugin. Code closeness. Maybe allow both by have a default "derivator" which invokes static methods on "derivative = true" annotation for cases of reuse.

Would it make sense to bake in some common versioning from the start?

EclipseGc’s picture

We actually worked through that scenario (it's the solution I prefer) but ctools has had complaints about this topic before, and while you're correct from a closeness standpoint, cleanliness wise the separate class works a little better. That being said, a different Derivative Discovery Decorator class could be written to implement what you want. I expect this will be a common use case during the D8 cycle, and that we'll formalize this a second (and final) time in D9.

With regard to the plugin directories, we need the two directory structure under a statically set directory (in this case we've chosen "Plugins"). This is to prevent modules with the same plugin name from colliding and picking up each other plugins. As an example, you can likely expect core to provide a cache plugin of some sort, similarly you can expect views to provide a cache plugin. Plugins\core\cache and Plugins\views\cache respectively separate these plugins by the type that implemented them. In this way they don't ever conflict. Hopefully that makes sense. I'm very much a fan of the plugin annotations as well, and I really hope we get that into core. I think it could do some really cool things for us.

Thanks for reviewing this.

Eclipse

EclipseGc’s picture

Issue summary: View changes

Correcting links again.

alexweber’s picture

As someone who has been following all of this stuff from a distance I admit that the whole plugin concept is not that trivial to wrap one's head around but the documentation around it is pretty good and its starting to make more sense.

I'm a big fan of PSR-0 and then new namespace/folder organization, but one thing that I really can't figure out is the seemingly interchangeable upper/lower case notation being used in namespace declarations. As far as I can tell it doesn't really matter to PHP but it should be in our coding standards to make sure the code looks homogenous.

Personally I love annotations and, regardless of whether they are implemented, the whole discovery system will be good to reduce the sheer number of hooks processed on every request.

This is a big change but a good one IMO and, from a module developer's standpoint, I think everything proposed makes sense.

Good work! :)

Adagio’s picture

Yea I definitely like namespacing to module/type making that data easily available. What I meant wrt the directories is that I'd prefer to have a folder designated exclusively to namespaced plugin implementations. So stuff like Type, Derivative etc doesn't intermingle, making both things harder to get a handle on (envisioning tons of plugin implementations and possibly other related types of classes). Possible alternatives include:

1) - Common root, subdirectory for implementations
Plugins/Implementations/module/access_controller/Role.php
Plugins/Type/AccessController.php

2) - Different roots for implementations and "system classes"
PluginImpl/module/access_controller/Role.php
Plugin/Type/AccessController.php

EclipseGc’s picture

Apologies for the handful of issues with getting this code working. Patches are still being committed against the plugins-next branch and the example plugin module should be up to date at this point and working properly.

EclipseGc’s picture

Issue summary: View changes

adding a link to a related issue.

xjm’s picture

So, I come from a functional programming, cowboy-coding, "self-taught hack" PHP background, and I've never once coded anything using CTools. I have only a vague idea what a plugin is. I'm probably pretty representative of a fair chunk of Drupal's contrib and freelance developers in that regard--the ones who have the most Drupal baggage and the least familiarity with the way the "rest of the world" supposedly does things. Pretty much a Drupala Rasa when it comes to this system. So let's see how it takes me to understand it! I decided to more or less document a stream-of-consciousness for it.

Chapter 1: Didn't RTFM, but stuff is fairly clear

First I just did what I'd do whenever testing out any module. Installed it, tried most of the features. Enabled the submodules and tried their features too. Simple enough. So how does it work? How would I extend it to add my own messages?

  1. Opened and skimmed msg_plugin.info, msg_plugin.install, msg_plugin.module.
    • Nothing too unfamiliar here. Mostly basic module hooks.
    • The hook_init() creates a new Messenger object to implement the module's functionality, totally readable,
      $messgenger_factory->createInstance()->message().
    • There's an info hook that has a title, description, and fully namespaced classname for the roses and violets messages; seems straightforward enough.
    • Similarly in the configuration form, it creates a new object to get the definitions of available messages.
      $messenger->getDefinitions(). The configuration option is saved in a plain old config variable.
  2. The only thing that isn't clear to me is why the createInstance() method is taking an empty array in addition to the machine name of the configured message, so I look it up.
    • First I check in the included Messenger class; I know where to look for it from the use statement in msg_plugin.module and because I know about PSR-0.
    • Only thing in Messenger.php is the super-simple constructor, so I guess the method's on the base class or something.
    • Some weirdly named classes used here--Reflections? DerivativeDiscoveryDecorators sounds like Dr. Seuss. "Daisy's Derivative Discovery Decorators, D, D, D." Whatever. Looks like this tells Drupal what info hook to use to get info about the available messages. I don't care about that at the moment, though; I'm looking for something else.
    • At this point I'd go to api.d.o, 'cause obviously this method is coming from something in core. I even set up a site with API module to emulate this but it's not parsing the stuff for me. Did someone not write docblocks? Documentation gate, boys and girls. ;)
    • Lacking that, though, the four use statements in Messenger.php tell me where to look in core, so I grep. Again, with api.d.o, this wouldn't be necessary.
    • I find the method right away in PluginType.php, and it's declared as:
      public function createInstance($plugin_id, array $configuration = array())
    • So looks like the second argument is some optional configuration, and the module's author is being needlessly verbose in passing array() when calling the method. To test my hypothesis, I delete that argument from inside the hook_init() and see if stuff still works. And lo, it does.
  3. So next I'm going to look at the classes for the two messages from the info hook. Again I just browse the PSR-0 path based on the namespaces given in that hook. And wow, couldn't be any simpler, just has a message() method that returns that string, as used in the hook_init(). Makes total sense.
  4. Still not concerned with the Dr. Seuss internals just yet. I look in the submodules next.
  5. First I look at the extras. Simple as heck; just an info hook declaring two more possible messages, with the info hook and class paths I expect from the main module. I spent more time typing this sentence than scanning that. I add my own static message in my own module in a matter of minutes.
  6. Next the custom derivative thingy. I pop open the module's files again.
    • The .install creates a table to store the custom messages I was adding, naturally. Nothing unfamiliar there.
    • Module file is pretty standard: a menu item and accompanying form to create the custom messages; another implementation of that info hook. The only thing different is that the info hook now has a 'derivative' key in addition to the title, class, and description keys that I recognize from the other two info hooks. It looks like another class with namespace. I'm curious how that works, so I (again) tab down through the lib directory to peek inside those classes.
    • Custom::message() is different from the static ones in the obvious way; it just queries the database for the message to use. Duh, of course.
    • The only line that's not immediately obvious is:
      list($plugin, $mid) = explode(':', $this->getPluginId());
      So it's parsing the message ID out of the plugin ID, which is namespaced with a colon. Okay, but not sure where the namespacing by the colon is coming from. Is that in the .module anywhere? Nope. The parent module? Nope. Time to look that method up on the core API, too.
    • I dig around inside the core plugin directory a bit and still don't see anything in the constructors or getID() methods that explains how and when the namespace is set for the plugin ID, so time to consult the docs.

So I totally understand how to add my own static message; it's completely mindless. However, I need to read a little more to understand how the derivative thingies work, so I'm going to read the docs. No spoilers, please. Going to RTFM, and then write my own custom plugin-thinger. :)

Edit: I think, if I exclude the time I spent writing the post above and being distracted by other things, the exploration above took me probably an hour at most. Edit 2: Less, actually, I think.

xjm’s picture

Chapter 2: Reading the docs like a grad student

  1. So, the docs (which I skimmed) didn't say anything about the namespacing.
  2. On a hunch I grepped the whole core Plugin directory for ':', and found methods that handle the namespacing in Daisy's Derivative Discovery Decorators.
  3. None of the docs for getPluginId() say anything about it.
  4. Now, I guess since the constructor apparently allows developers to set whatever discovery method they want, I certainly shouldn't expect docs about this namespacing stuff in the plugin interface or abstract; but I went on a wild goose chase through like six different files and couldn't decide where it should be documented.
  5. At a minimum, DDD::encodePluginId(), DDD::decodePluginId(), and DDD::getDerivatives() need some more explicit docs and some @see action from... somewhere. You folks who understand the guts decide where.
  6. I'm wondering if the custom plugin could've used DDD::decodePluginId() instead of the explode() business. The method is protected and I don't quite grok the whole inheritance structure and stuff.
  7. However, I think there should be public getters for the components of the Plugin ID. E.g.:
      $this->getPluginID()->parentIDPartThingamajig();
      $this->getPluginID()->childIDPartThingamajig();
      

    I don't know what to call them, but I assume someone else does. :)

  8. Aside: As far as I recall, the entity API uses simple method names for object property thingies without the get prefix, like ->id() and ->label() and ->value(), which is more intuitive to me. I know this is one of those holy wars things (emacs pinkie salute!) and that there's disagreement on that point for entities as well, but wouldn't it be good to be consistent with other parts of core? Or am I totally mistaken about that? Or is there some subtle difference with the added abstraction here that means the get prefix distinguishes these methods from those in some way?

My distraction over the ID namespacing bit is resolved, and since I'm a lazy developer, I'm probably going to try to copy from the custom derivative message thing without reading the docs carefully or necessarily understanding the big picture. I'll consult them if I get stuck, probably, but when I skimmed them they seemed more relevant for creating a base plugin thing, not for extending it.

Time spent: about 20 mins. (not including the writeup).

xjm’s picture

$this->getPluginID()->parentIDPartThingamajig();
$this->getPluginID()->childIDPartThingamajig();

So I realized during my nap this doesn't work since getPluginId() returns a string (also, I hate the casing in the actual method name). But, you get the idea.

effulgentsia’s picture

FileSize
4.92 KB

Thanks for the reviews so far! Keep them coming.

As I wrote in #1676202-3: DX: D8 Plugin Type writing, the example module in the issue summary is a great way to onboard to the proposed plugin system, but in case it also helps to see a before/after of how this will affect writing plugins for modules that convert their existing plugin-like systems to the proposed plugin system, here's the relevant portion of #1497366-176: Introduce Plugin System to Core that relates to the feedback being requested on this issue. Feel free to post reviews here of either the example module, or the aggregator module changes in this patch, or both. Thanks!

xjm’s picture

I was advised not to look at that patch in #12 until I'd read the docs and explored the examples. ;)

Garrett Albright’s picture

Could someone explain to me, a developer with a fair amount of module development experience but hasn't really been following D8 development all that closely (or worked with Ctools much), what in broad terms a "plugin" is and how it differs from a module? What are some cases where I'd want to write one instead of another? I scanned through the documentation, but still didn't feel like I was getting the picture.

The D8 plugin system is designed to provide an easy to follow set of guidelines and reusable code components which will allow developers to expose pluggable components within their code and enable management of those components through the user interface more readily when the situation requires it.

So they're like basic modules intended to be used with other modules/plugins and with a whole bunch of pre-built Form API stuff to integrate with it in the back end? Yeah, I don't get it.

(This isn't entirely a selfish request - this sort of information should probably go into the documentation as well.)

EclipseGc’s picture

@Garrett,

No, they're more like info hooks that automatically remove the lion share of the code from the executable path. Usually when an info hook exists it is so that we can do something like:


$form_options = array();

foreach (module_implements('my_hook_info') as $module) {
  foreach (module_invoke($module, 'my_hook_info') as $id => $info) {
    $form_options[$id] = $info['title'];
  }
}

$form['my_hook'] = array(
  '#type' => 'select',
  '#title' => t('Some title'),
  '#options' => $form_options,
  '#default_value' => variable_get('my_var', 'some default'),
);

Then at some later portion of code, you'll re-invoke all of this all over again, and pass that variable to get just the info for the selected item, and then do something bigger with that information (like firing a callback or something).

In drupal at the moment, all of this code is generally in the executable path, or the developer has to go to great effort the write wrapping code around all of that that will optionally load code from some file to execute a callback. Plugins within D8 leverage PSR-0 class namespaces (there's a link in the top of the docs I wrote: http://drupal.org/node/1637614 about PSR-0 stuff) to move all of these "callbacks" outside the executable path. This means:

  1. Better code organization
  2. Leaner Drupal (probably)
  3. Standard solution (once a developer learns it, looking at someone else's implementation should be relatively trivial)

Plugins can be used in places other than info hooks, but they're the easiest analogy to discuss right now. Hopefully this answers your questions.

Eclipse

xjm’s picture

Chapter 3: The derivative things

I took another look at msg_plugin_custom with the goal of creating my own derivative thing (in this case, to set my own dynamic messages).

  1. I look again at Drupal\msg_plugin_custom\Plugins\msg_plugin\messenger.
    • Other than the lack of code documentation, this makes complete sense now that I know what the deal is with the namespacing of the plugin ID.
    • I am momentarily frustrated again by the fact that I had to spend twenty minutes sorting out why I needed to parse the plugin ID to get the plugin ID from DDD::getPluginId(), when the variable $plugin (looking at DDD:encodePluginID() I guess it's called the "base plugin ID") is never going to be used nor vary within my module. But, presumably, this is a DX issue that can be fixed before this goes in or as a followup.
  2. I look at Drupal\msg_plugin_custom\Plugins\Derivatives\Custom. One thing I wonder about right away is why there are two different classes and consequently two different files. Like, why can't it just be:
    namespace Drupal\msg_plugin_custom\Plugins\msg_plugin\messenger\Custom;
    
    class Custom extends PluginAbstract implements DerivativeInterface {
    
      public function message() {
        // ...
      }
    
      public function getDerivativeDefinition() {
        // ...
      }
    
      public function getDerivativeDefinitions() {
        // ...
      }
      
    }
    

    But whatever, I don't actually need to understand that right now.

  3. Aside, If this is going to be used as an examples module kinda thing, I'd really recommend adding proper docblocks for the classes, with extra information about what they're for, and adding additional details to the docblocks for the methods explaining what they do and what their purpose is. Not going to give EclipseGc a hard time about that yet, though, since this is way more than nothing. :)
  4. I blink at getDerivativeDefinition() once or twice.
    • Oh, it's a single-value wrapper for the other method in here. And what, the interface makes you declare both probably. Okay.
    • I can't immediately think of a usecase where the single-value wrapper would ever do anything other than exactly what this one does--call the multiple value one, and return the derivative thing corresponding to the requested derivative ID. So it's weird that there isn't a base class to do that for me, but it's no big deal because I can just copy and paste.
    • I compulsively move it after the other method in the code source because that seems more logical.
  5. I look at getDerivateDefinitions().
    • So it queries the database to get all the data on the custom messages the user has defined, sets the title and description for each, and then merges in the base plugin definition.
    • Straightfoward enough, except I wonder who's passing it that base plugin definition. This'd be where I'd consult api.d.o again.
    • I grep the core Plugins directory for it again, and see it gets called in DDD. Naturally, I guess. There's a protected getDerivatives() that uses it. "This should be called by the class extending this in DiscoveryInterface::getDefinitions()." Uh, okay. I don't really care to explore this further; I assume that stuff that's happening automagically based on how the Messenger plugin type class is declared. The keys (title, description, etc.) are the same as in the info hook, and probably defined in HookDiscovery. It's 7:30a, so I don't bother to confirm if that's actually the case.

Next I'll come up with a use-case where I'd make my own derivatives and code that up.

merlinofchaos’s picture

I can't immediately think of a usecase where the single-value wrapper would ever do anything other than exactly what this one does--call the multiple value one, and return the derivative thing corresponding to the requested derivative ID. So it's weird that there isn't a base class to do that for me, but it's no big deal because I can just copy and paste.

You never want to do this, for performance reasons, because that woudl cause you to needlessly load N-1 derivatives where N is the total number of derivatives you have and 1 is the number of derivatives you actually want. This isn't much of an issue when you have only 3, but what if you have 100? What's the largest number of views you've ever seen on a site?

merlinofchaos’s picture

Note: the singular functions would be unnecessary if we forced the full list of plugins to always be cached, which is something we have to consider in any case. This would be nice for DX, but forcing caches is also not nice for other DX, so it's a trade-off.

neclimdul’s picture

You guys are great. I have nothing to add other then keep the thoughts coming.

EclipseGc’s picture

Apologies for being lazy on this. I tried to do something I thought would be intuitive and it was the exact opposite I see now. I will try to change this code specifically to be a bit more straight forward today if possible. In my defense, I was pretty pressed for time ;-)

Eclipse

EclipseGc’s picture

Issue tags: +Blocks-Layouts

adding scotch tag

Sylvain Lecoy’s picture

This seems simple enough and usable from a dev point of view.

However, I don't see the use of any interface in your plugins, how do you plan to mock/stub them for testing ? Is that just a miss for the demo purpose ?

Also, the Message class seems first to be a concrete factory (at least it is said l.10 of msg_plugin.module), ok then, but the naming is a bit confusing, and the implementation too, why it does not contains a ->message method then ? If I'm looking at the inherited class, I notice that the Message class is actually not a factory, but extends a sort of Manager, which is a mix and match of a Factory, Mapper, and Discovery. It does not make sense.

The Manager is for me a typical Mediator. Again, the naming does not make sense (from a DX perspective). From an architectural point of view, why do these interfaces need to collaborates together ? I can see in the implementation (PluginManagerBase) that there is no interaction at all.

So let's have a look at the mediated objects:

Discovery

I first look at the interface, here I should found every information that I need. It contains two methods, simple enough to understand at first sight. At least if I knew what is a plugin definition. It is an array like hook_info ? It is a class mapping ? There is no link for documentation about a plugin definition in the code, so let open the implementations to find more about it. The Static implementation gives no clues about it. The Decorator one neither.

The Decorator has a wierd implementation, the purpose of a decorator pattern is to add (extends) the functionalities of an object at runtime, that is, the decorator adds functionalities. Here it seems acting like a proxy, delegating new functionalities to the object decorated, which is not the intent of the Decorator pattern. From a DX perspective, it is again something hard to follow and understand, I should have renamed it Proxy, or made the methods added to the decorated object public. The naming does not make sense.

Factory

This is an expected interface of a concrete factory, specifying the id of the plugin to instanciate via a discovery mechanism a plugin. It does make sense. By looking at how the factory uses the discovery sub-component, it looks like the discovery is a actually a configuration of classes. The factory is creating plugins from a dynamic configuration of classes (called discovery in the DefaultFactory), meaning that the adding of new classes in the container (new plugins) are fully supported because the discovery object will reference them. That make sense (not the naming however).

Mapper

Can't see in your example how the mapper is set, or used ? What is the relationship between the factory and the mapper ? Seems like createInstance and getInstance is related, but how ?

EclipseGc’s picture

Mappers are generally used for doing something like shortcutting to a particular configuration for a particular plugin. So you might pass a CMI config name here which is in turn loaded, and broken into "plugin_id" and "configuration" to then be passed through createInstance. It's not actually useful to all plugin types, and we didn't have a need for one for the aggregator fetcher we already converted, so a Mapper was not delivered with core. I have one I am working on for the blocks migration.

The Plugin Manager implements discovery, factory and mapper interfaces because we did not want to have to do an extra level of calls $manager->factory->createInstance() vs $manager->createInstance().

Discovery is not really a difficult concept, but you're not alone in not getting it at first glance. The easiest way to think of it is as a replacement for info hooks. It's going to map VERY closely to that. The only real difference is that with the Factory classes that were committed to core, you're expected to add a class element to the plugin definition array so that we can instantiate the appropriate plugin.

Hopefully this helps some.

Eclipse

Sylvain Lecoy’s picture

Thanks for the explaination, so here are my concerns about naming:

  • Rename the Decorator to Proxy, the DerivativeDiscoveryDecorator is not the implementation of the decorator pattern at all. It should be DerivativeDiscoveryProxy.
  • If Messenger is a plugin type, at least it should implements an interface, or declares an interface, that plugins should implements. In the example with the ->message() function.
  • The Plugin Manager is a sort of Facade then. Why not calling it PluginFacade instead of PluginManager.

About the architecture, I just saw that the plugin system has been commited to drupal and discussion has been long, so maybe its not time to discuss about it. I did not followed the development so I come with no background at all, neither on ctools plugin system. I think its clean, readable and usable, when I'll give it a try by implementing some plugins for some of my projects maybe I'll be able to give you some in-depth review.

About the renaming, I think you should definitely think about the three points I mentionned, not naming correctly software artifacts is for me a critical point. Programmers expect something that is well known to work like it says to, and when they look at the implementation, it is something completely different. If you invent some design patterns its good, but if you are naming a class that is supposed to implements a pattern which has been documented and specified for almost 20 years in architectures books, and doing something different, I think its not a good practise.

EclipseGc’s picture

Rename the Decorator to Proxy, the DerivativeDiscoveryDecorator is not the implementation of the decorator pattern at all. It should be DerivativeDiscoveryProxy.

I've gone a read a bit on Proxies and Decorators today just to make up my own opinion on this topic (as I was not the person who named them such). Mostly I have to rely on what I find on the web as I'm not a pattern aficionado so without further ado:

http://powerdream5.wordpress.com/2007/11/17/the-differences-between-deco...
http://stackoverflow.com/questions/350404/how-do-the-proxy-decorator-ada...
http://social.msdn.microsoft.com/Forums/en-US/architecturegeneral/thread...
http://www.coderanch.com/t/98988/patterns/Proxy-Vs-Decorative-pattern

All of these seem to support our use of the word Decorator here, so I think I have to disagree. What they define as a Decorator is indeed what we defined as a Decorator

If Messenger is a plugin type, at least it should implements an interface, or declares an interface, that plugins should implements. In the example with the ->message() function.

The msg_plugin module is 1000% just an example, and as such I went for the absolute simplest implementation I could think of. Interfaces are great (obviously we used them all over the place in the actual plugin system) but it would complicate the learning of actually implementing a plugin type and/or creating plugins. As such I really consider it a "best practice" and chose not to include it in the example because what I wanted was a bite sized chunk that someone who doesn't already know OOP super well could grasp and utilize. Within the confines of most core plugin system I expect there to be interfaces and abstracts, and other such things that you expect in a heavily OO driven environment, but I did not want to confuse people who were already going to look at the namespace, use and class statements with trepidation.

The Plugin Manager is a sort of Facade then. Why not calling it PluginFacade instead of PluginManager.

This particular conversation delayed the plugin system going into core by weeks, and I personally really like Manager (disclosure I suggested it to Dries, I think others may have suggested it earlier, but it fit well). I will admit that I personally am not that knowledgable about recognized programming patterns, their names, etc... however I can read a wikipedia page, and their description of a Facade does not match what we call a plugin manager. There is really no simplification of another interface involved here, it's just a group of interfaces together with the abstract class proxying to their methods, which likely makes this a better candidate for being called a proxy than what we currently call decorators. The problem here is that the Proxy pattern intends that we add functionality (which we aren't doing) and a Facade intends that we simplify functionality, which we are also not doing. Likewise from my reading a Proxy seems to be optional, while a Facade generally is not. In our case, since our abstract is literally calling through to the appropriate class' method of the same name, a plugin system could feasibly exist which did not utilize a manager at all. They are purely a sane convenience (and will likely end up in the Dependency Injection Container if all goes well with future development).

In truth, from my reading the Plugin Manager looks closest to a Bridge pattern, though in weeks of discussion on the topic, no one (many of whom are familiar with patterns) suggested this.

Eclipse

Sylvain Lecoy’s picture

I have to admit that the line between Proxies and Decorators is gray like they say :)

But the thing is, a decorator should add functionalities to the decorated object, adding methods. The fact that all methods are protected shows that no new functionalities are added by the decorator. Plus, the lines belows are pure delegation:

Delegation.

<?php
  /**
   * Passes through all unknown calls onto the decorated object.
   */
  public function __call($method, $args) {
    return call_user_func_array(array($this->decorated, $method), $args);
  }
?>

Decoration.

<?php
  /**
   * New method!
   */
  public function newFunctionality($arg) {
    // Do something with arg.
    $test = $this->decorated->getDefinition($arg);
    return is_array($test) ? $test : array($test);
  }
?>

In the pages you mention, they speak of proxy like it is a ProtectionProxy or AccessProxy, from the GoF definition, Proxy Pattern is described with the following intent: "Provide a surrogate or placeholder for another object to control access to it.".
Decorator Pattern is described with the following intent: "Attach additional responsibilities to an object dynamically. Decorators provide a flexible alternative to subclassing for extending functionality.[GoF,p175]".

  • In the wordpress one: look the UML diagrams, the Proxy and RealSubject has the same method: DoAction(), while the Decorator and ConcreteDecorator share the same operation() method, but the ConcreteDecorator adds additionalBehavior(). It is written in a UML doc "operation() is delegated to parent before/after additionalBehavior()".
  • In the stackoverflow one: Proxy could be used when you want to [...] control access to the object. Decorator is also called "Smart Proxy." This is used when you want to add functionality to an object [...].

I agree with you that there is no complex access control, but the "DDD" class is clearly a surrogate. Actually, its like drupal menu system, sometimes the 'access callback' is just TRUE, that is a sort of access control. The fact that you given permission for all unknown methods to be delegated to the proxied object, is an access control. Plus given the fact that the DDD class does not add any new functionalities, the naming "Proxy" better reflect the class than "Decorator".

Now there is something about "a proxy manage creation of the encapsulated object, whereas you pass an existing object to a decorator". I am not 100% sure that this is correct, however I remember from my classroom time that a decorator never creates an instance on his own, but a proxy does not necessary manage entierely the lifecycle of an object. For some case it is good that a proxy manage the creation and deletion (a protection proxy, or a firewell, for instance), but in practise you never want to do that. When you want to test your proxy class, you need to inject a Stub or Mocked object, that mean in a way, the proxy does not fully manage the creation of the dependencies, or your proxy class is untestable !

Here is how the java.io is decorated:

You could have named it Decorator only if you added a public method, or made the function getDerivativeFetcher() public for instance (augmenting the API), in this case yes it should have been a Decorator. But because you are controlling the access to this new functionality (with the protected keyword), this is definitely a Proxy.

Glad you agree with the interfaces, and yes for the demo purpose it is better not to add complexity.

Appart from the Decorator, I really feel confortable with the plugin API. Examples and use case will help to streamline it. :)

Sylvain Lecoy’s picture

FileSize
64.21 KB

I don't know if you come from a Java background but "Eclipse" looks like you are familiar with this language so this could help you figure out why the DDD is a Proxy and not a Decorator.

In short: Decorators augments the interface. Proxies controls access to a surrogate.

EclipseGc’s picture

OK, pretty sure I 100% grok what you're going after right now, I do have a bit of a problem with it though and my problem is pretty simple.

1.) The simple fact of the matter is that these would be decorators if their methods were publicly accessible.
2.) Nothing prevents someone from adding a class there that does meet the strict definition of a decorator.

If then we get really serious about making sure these things are named properly, we end up with situations where there's something like

$this->discovery = new MyDecorator(new MyProxy(new MyDiscovery()));

And I question whether that is a good thing. It also significantly complicates explaining what's happening on that line if I suddenly have to begin delineating between something that it looks like can actually be argued about by people who know. I'm just trying to be practical here.

Finally, I understand why you say it's not a Decorator, I'm still not sure I agree that it's a Proxy from my reading but this sort of feels like the elimination of one option leaves us with a default, which I'm not sure I like. In any event, given how fine a line this is and the fact that both decorators and proxies are likely to exist on that line, do you honestly feel like this is an issue worth revisiting? I really don't personally.

Eclipse

1.) On a personal note, the definition of: "This is used when you want to add functionality to an object, but not by extending that object's type." really speaks to me still as what we're doing with things we call Decorators (from the definition on the stack overflow link) because, "functionality" has never been "new public methods" in my mind. If you look at that explanation and immediately dismiss it as wrong, then fine, I'm willing to be wrong here, but this goes back to just how fine a line this is and the definitions we assign to various words.

2.) No, my name predates the IDE pretty significantly.

Sylvain Lecoy’s picture

If you ask me personally, I would have renamed it (otherwise I would not have written so much text just for the sake of it).

But you redirected me to some interesting discussion, and thing evolves, maybe the API in its final state will looks completely differently. Just wanted to say that there was three things that confused me when I looked at the implementation, that's it. Maybe it will not confuse other people, that's just personnal !

About your point from the "new public methods" thing. I made some interesting research about it; Historically, decorators was used for graphical interfaces. For instance if you have a TextView object that displays text in a window. TextView has no scroll bars by default, because you might not always need them. When we do, we can use a ScrollDecorator to add them. Suppose we also want to add a thick black border around the TextView. We can use a BorderDecorator to add this as well. We simply compose the decorators with the TextView to produce a bordered, scrollable text view.

Decorator subclasses are free to add operations for specific functionality. For example, ScrollDecorator's ScrollTo operation lets other objects scroll the interface if they know there happens to be a ScrollDecorator object in the interface. (Design Patterns [176] - 1994). It is said that subclasses are free to add, meaning that its not an obligation. The collaboration specifications are: "Decorator forwards requests to its Component object. It may optionally perform additional operations before and after forwarding the request." and are not violated by your implementation.

But the Decorator pattern is not limited to graphical user interface, streams are a fundamental abstraction in most I/O facilities. For use cases in the java.io new methods are not necessary added, and you are right that new functionalities is not necessary new public methods.

To sum-up, things that did not make sense for me at first sight are now clear:
1) I was confused by my own lack of understanding the pattern, thank you.
2) You told me that the use of interface in a demo was not necessary. 100% agree.
3) The Manager is a legacy object that will be replaced by DI if I understood you well, no big deal to think about a convenient name, Manager is fine.

EclipseGc’s picture

The only clarification I would make is that the Manager is not likely to go away, just likely to be pushed into the DIC so that contrib could actually replace the manager if there was ever such a desire. Otherwise I think we're 100% on the same page here.

Eclipse

effulgentsia’s picture

The Manager is a legacy object that will be replaced by DI if I understood you well

I don't think so. What may change is just how the manager object is gotten, so for example, in aggregator_refresh(), $fetcher_manager = new FetcherManager(); may eventually become $fetcher_manager = drupal_container()->get('plugin.manager.aggregator.fetcher'), or if aggregator_refresh() moves from a procedural function to an object method, then $this->fetcherManager could be injected into an $aggregator object. But I don't see the concept of a manager object going away.

I agree there's some similarities between the manager object and a facade pattern depending on how you interpret the facade definition of "Provides a simplified interface". In the case of manager, it's not implementing a reduced interface, it's combining all the interfaces into a single entry point object. I think this is a type of simplification, but I don't know that it fits the strict definition of "facade".

I haven't read this whole thread in detail yet, but thank you for commenting on it from a classic OO perspective. I agree with trying to see where we're using already named patterns and following their naming conventions as much as possible. And where we're not using established patterns, asking ourselves whether we should be or whether we really do have a novel thing we're expressing.

sun’s picture

Issue tags: +Plugin system
sun’s picture

Issue summary: View changes

adding a specific sha1 to guarantee compatibility.

sun’s picture

Component: other » plugin system
Status: Active » Fixed

Marking these as fixed. We're still iterating over many aspects and are improving them. Check the plugin system component for related issues.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

link filter to issue