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.
- msg_plugin: the base module which defines the plugin type, and a couple of plugins.
- 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)
- 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
Comment | File | Size | Author |
---|---|---|---|
#27 | Decorators.png | 64.21 KB | Sylvain Lecoy |
#12 | plugins-aggregator.patch | 4.92 KB | effulgentsia |
Comments
Comment #1
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedApparently there's no remote branch called info_hooks.
warning: Remote branch info_hooks not found in upstream origin, using HEAD instead
Comment #2
EclipseGc CreditAttribution: EclipseGc commentedcheck again, I copied and pasted exactly what is in the post and it works for me.
Comment #2.0
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedCorrecting links.
Comment #3
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedEDIT: My mistake, sorry. I was pulling from the same sandbox as for the plugin system.
Comment #4
Adagio CreditAttribution: Adagio commentedI'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?
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedWe 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
Comment #5.0
EclipseGc CreditAttribution: EclipseGc commentedCorrecting links again.
Comment #6
alexweber CreditAttribution: alexweber commentedAs 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! :)
Comment #7
Adagio CreditAttribution: Adagio commentedYea 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
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedApologies 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.
Comment #8.0
EclipseGc CreditAttribution: EclipseGc commentedadding a link to a related issue.
Comment #9
xjmSo, 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?
msg_plugin.info
,msg_plugin.install
,msg_plugin.module
.hook_init()
creates a newMessenger
object to implement the module's functionality, totally readable,$messgenger_factory->createInstance()->message()
.roses
andviolets
messages; seems straightforward enough.$messenger->getDefinitions()
. The configuration option is saved in a plain old config variable.createInstance()
method is taking an empty array in addition to the machine name of the configured message, so I look it up.Messenger
class; I know where to look for it from theuse
statement inmsg_plugin.module
and because I know about PSR-0.Messenger.php
is the super-simple constructor, so I guess the method's on the base class or something.use
statements inMessenger.php
tell me where to look in core, so I grep. Again, with api.d.o, this wouldn't be necessary.PluginType.php
, and it's declared as:public function createInstance($plugin_id, array $configuration = array())
array()
when calling the method. To test my hypothesis, I delete that argument from inside thehook_init()
and see if stuff still works. And lo, it does.message()
method that returns that string, as used in thehook_init()
. Makes total sense..install
creates a table to store the custom messages I was adding, naturally. Nothing unfamiliar there.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.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.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.
Comment #10
xjmChapter 2: Reading the docs like a grad student
':'
, and found methods that handle the namespacing in Daisy's Derivative Discovery Decorators.getPluginId()
say anything about it.DDD::encodePluginId()
,DDD::decodePluginId()
, andDDD::getDerivatives()
need some more explicit docs and some@see
action from... somewhere. You folks who understand the guts decide where.DDD::decodePluginId()
instead of theexplode()
business. The method is protected and I don't quite grok the whole inheritance structure and stuff.I don't know what to call them, but I assume someone else does. :)
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 theget
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).
Comment #11
xjmSo 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.Comment #12
effulgentsia CreditAttribution: effulgentsia commentedThanks 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!
Comment #13
xjmI was advised not to look at that patch in #12 until I'd read the docs and explored the examples. ;)
Comment #14
Garrett Albright CreditAttribution: Garrett Albright commentedCould 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.
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.)
Comment #15
EclipseGc CreditAttribution: EclipseGc commented@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:
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:
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
Comment #16
xjmChapter 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).Drupal\msg_plugin_custom\Plugins\msg_plugin\messenger
.DDD::getPluginId()
, when the variable$plugin
(looking atDDD: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.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:But whatever, I don't actually need to understand that right now.
getDerivativeDefinition()
once or twice.getDerivateDefinitions()
.DDD
. Naturally, I guess. There's a protectedgetDerivatives()
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 theMessenger
plugin type class is declared. The keys (title, description, etc.) are the same as in the info hook, and probably defined inHookDiscovery
. 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.
Comment #17
merlinofchaos CreditAttribution: merlinofchaos commentedYou 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?
Comment #18
merlinofchaos CreditAttribution: merlinofchaos commentedNote: 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.
Comment #19
neclimdulYou guys are great. I have nothing to add other then keep the thoughts coming.
Comment #20
EclipseGc CreditAttribution: EclipseGc commentedApologies 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
Comment #21
EclipseGc CreditAttribution: EclipseGc commentedadding scotch tag
Comment #22
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThis 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 ?
Comment #23
EclipseGc CreditAttribution: EclipseGc commentedMappers 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
Comment #24
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThanks for the explaination, so here are my concerns about naming:
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.
Comment #25
EclipseGc CreditAttribution: EclipseGc commentedI'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
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.
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
Comment #26
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI 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.
Decoration.
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]".
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. :)
Comment #27
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI 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.
Comment #28
EclipseGc CreditAttribution: EclipseGc commentedOK, 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
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.
Comment #29
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedIf 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.
Comment #30
EclipseGc CreditAttribution: EclipseGc commentedThe 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
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #32
sunComment #32.0
sunadding a specific sha1 to guarantee compatibility.
Comment #33
sunMarking these as fixed. We're still iterating over many aspects and are improving them. Check the plugin system component for related issues.
Comment #34.0
(not verified) CreditAttribution: commentedlink filter to issue