Feedback from pounard

sub. Thanks for reporting it here.

Another note, what is the benefit from having the PluginControllerSingleSlotInterface interface? Does the specific business code should define by itself if it's using a singleton or I misunderstood what it's being used for?

Discovery is basically a registry (more than that, but a module could just provide a single registry), this pattern name should probably resides somewhere, it's much more clearer. Besides, a module could basically define a plugin type and provide its own registry that does not provide any discover facilities (why not?). In this particular case, a provider would probably not be needed.

What's missing in this code is the factory side of this. All this code only register and provide lookup for finding plugins, but maybe the most important part is missing, how do I get instances?
EDIT this paragraph only: May be the controller should be the factory? While it's unclear in the current design, the more important method is the one that gives me my instance.
Probably a higher level of Controller (the factory then) should be public API instead of the procedural code, eg: get_plugins_factory($plugin_type)->getInstance($partiular_type = NULL); // If NULL give default or first one, else attempt to find the one asked, in both case interogate all "controllers" for the given $plugin_type).

Why does the procedural API need to have the $owner parameter? It's good for low level administration tasks, but not for end-developer that doesn't care from where its plugins come from, he just need to have working instances from a certain type?

Cache lookup in the PluginDiscoveryIncludeBase class provide a single cache entry for each owner/type couple. It might be smarter to use one and only one cache entry, growing each time yo request a new object. If an object does not exists, just mark it with something like $plugins[$owner][$type] = FALSE in order to avoid the next lookup if it does not exists. By doing this, more cache_set() will be done, but in the long term, only one cache entry will be kept therefore you'll probably do a lot less I/O on a production system.
It depends on the size of the cache, but if it's less than a few hundred of thousands bytes, don't hesitate to cache one instead of multiple (each cache_get() lead to a costly I/O operation over a remote server, think about it).

BTW you probably should cache a more advanced array than the file list. By caching the file list you make the rest of the lookup algorithm being run each time someone request a plugin while a fully cached plugin map would probably avoid some foreach() here and there for a minimal cost in memory. It happens that the parseInclude() method can sometime be a really heavy operation, so its result should be cached, not the raw file list.

EDIT: Is there a particular reason of separating themes from modules? Basically, for core, themes are in the system table pretty much as the modules are. I known that module_implements() does not provide theme implementations (while drupal_alter() does) this inconsistency should probably get rid of in the lower layers than this particular plugin handling code, please keep it stupid simple.
EDIT this paragraph only: Besides, themes are purely for presentation, why on earth would they provide business plugins? This mixes up the presentation layer and the business layer, while the whole "butler" initiative is also about request routing (therefore totally removing presentation from business, or upside down) whatever is the reason, themes should probably not be able to provide plugins else it will probably lead to some aberrations like someone not able to switch theme back because the one he used provided a business plugin he needs.

Re-EDIT: Most of the default metadata you set in the info array is useless if I read your code right, maybe you should get rid of most of it (such as cache handling) and let specific controllers handle that themselves (default controller might handle both cached and not cached version, and just override a boolean property in custom implementations using it a base class would do the trick) then all the cache handling metadata won't be useful anymore, it will avoid to have arrays growing. The more metadata you will code instead of describing the more your code will be fast.
Sample: AbstractRawPluginControler implementts PluginController and class AbstractCachedPluginController extends AbstractRawPluginControler, and , then specific implementation will choose the cached or not, while the PHP compiler will generate bytecode for this metadata and run fast instead of you having to check array keys and stuff.

Not all plugins are 'business'. In the real world, there are plugins for layouts and styles and themes are very much interested in being able to provide these.

Themes can't respond to hooks, so they need a separate method. The most straightforward way we could come up with was to completely separate it.

As for Discover -- that's actually more or less what 'core' does. It provides a registry that doesn't actually discover anything. But I think discovery is a better term for it than registry.

Not all plugins are 'business'. In the real world, there are plugins for layouts and styles and themes are very much interested in being able to provide these.

I agree with this, but, in Panels for example, I can still show any Panels using any theme, therefore adding the theme plugin dependency over the Panel (which is itself not theme related at all, except by the fact that it provides templates and such) could lead to serious administration/management issues.
In this case, I think such module should choose, either it provide theme related only stuff, either it doesn't, but actual model mixes things up a bit.

As for Discover -- that's actually more or less what 'core' does. It provides a registry that doesn't actually discover anything. But I think discovery is a better term for it than registry.

I tend to disagree, because it keep the plugins list and is able to restore and lookup in it, so it acts like a registry. But this is discussable.

Themes can't respond to hooks, so they need a separate method. The most straightforward way we could come up with was to completely separate it.

And I'm quite happy that themes cannot respond to hooks, hooks are mostly business stuff related while theme are basically only skins (and they probably should remain no more than only skins in order to provide a full separation between data and presentation).

Thanks for your reply.

Whaw, you did even more writings than me!

As for the procedural accessors, if you mean hooks loading there isn't really any way around that since each hook can define quite a few plugins. So we'd need to whole registry to build the registry which kinda goes against the whole point

Why not? This would work quite flawlessly (I myself did this exact same kind of code and this also this kind).

Also a non-cached hook based plugin definition system was a /requirement/ of plugins in core because we have to define very early Drupal systems like cache using it(though I think include based plugins provide a better method for site specific implementations that's a separate argument that'll come up later).

Having some non cached plugins controllers seems totally right to me. But if the framework is not fully bootstrapped (remember than when you init the cache backends, you still have no variables) just an info plugin based controller probably won't be sufficient (both hooks and variables depends on cache). Cache plugin should be something that is handled in another way because of the early bootstrap phase they happen to init (only file configuration file is available from here).

As far as the OO can solve everything bit, I'm somewhat tired of explaining. Interfaces will not efficiently define metadata for admin interfaces or any arbitrary metadata

Totally understand that, I dont fight metadata, I fight too many of it: each piece of metadata you'll come up with will remain in memory for ever! When I saw that you did wrote something like 10 different metadata keys without even any existing implementation I thought it was a bit overloaded, you should add them while you need them (cache is something you do not mandatory need as metadata, for example, it's a choice to make).

we already have a flexible class registry autoloader that solves this problem.

Which sometimes literally explodes your site :) (this was a flame, but it happened to me, this autoloader depends on database, but database partially depends on autoloader, what if I enable a logger at earlybootstrap? Pouf, not a flame).
But having an autoloader is indeed not questionable, let's pass this question :)

As far as the panels bit

Panels bit was only theme related in my answer to Earl.

because CTools and Crell have different ideas about what a factory looks like

Then, they should agree sometime. What developers will use most are the factories.

A bit of background, CTools plugins as they are in contrib have always been about the discovery of plugin metadata. That's all its plugins are is metadata.

CTools plugins litterally implements an OO layer over procedural code. By switching to pure OO code this is the kind of thing you can erase from our memories now once for all (I don't say it for metadata).

We don't want a one of "here's some DI, give me an object" system, we want a definition management system that drives that object retrieval system

You probably want it anyway. But I understood well that is not this code's aim, this is the lower layer, won't argue it again about the existing code.

I've ripped out all the caching and algorithm optimization ctools had(many exactly like the ones you've mentioned) in favor of writing the most dumbed down straight forward implementations.

You did include cache, so it's not straight forward at all. You should have kept the original or remove all bits of it.

My plan here being that I hoped good patterns would emerge, code would fall into more logical layers, and at the end of the day when we're really happy with it

Then remove all of it.

That's a little naive and and optimistic

Absolutely not, this is the good way to go, design first, optimize later. Just remember that the design itself should be optimized while binary cache is micro optimization for the API level.

Opened these:
#1152476: Caching
#1152478: Factories

Thanks for your time and anwsers.

Subscribing. I didn't know this code existed, and have been thinking a lot about storage of plugin configuration, pre-database settings etc., have not reviewed any code here yet and only scanned the discussion but appears that I've finally found where this discussion is actually happening.