Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | plugins_service_container-10.patch | 52.79 KB | effulgentsia |
#8 | plugins_service_container-8.patch | 51.93 KB | effulgentsia |
#7 | plugins_service_container.patch | 48.12 KB | effulgentsia |
#5 | plugins_service_container.patch | 48.81 KB | effulgentsia |
#2 | drupal_service_container.patch | 17.21 KB | effulgentsia |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedHere'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.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedComment #4
effulgentsia CreditAttribution: effulgentsia commentedThis should be
setFactoryService('plugin.type.%plugin.scope%.%plugin.type%')
. The next patch I post here will have that fix.Comment #5
effulgentsia CreditAttribution: effulgentsia commentedHere 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).
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThis is ready for review now. It will certainly fail bot, but here's hoping bot won't even try on a sandbox project.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedThis patch adds a plugin() function wrapper to drupal_container()->blah()->blah()->blah().
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedThis 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:
Comment #9
neclimdulI'm still digesting this and it seems clever and all but I'm having a hard time figuring out why you wrote this.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedThis has a few more minor changes.
I'll try to find you on IRC today so we can chat about it.
Comment #11
RobLoachThis 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).
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.
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.
Comment #12
RobLoachDidn't mean to change status.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedI 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.
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.
Sweet. Thanks for the tip.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedWell, 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.
Comment #15
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedI 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.