User tests: Successful: Unsuccessful:
We need more and more direct plugin instances to work with, especially in media manager. So it would be nice to have a plugin factory for it. This pr introduces a plugin factory and convertes the Authentication workflow to use the plugins from the factory.
Additionally it deprecated the PluginHelper::importPlugin
and PluginHelper::import
functions.
Log in on the back end.
It works.
It works.
The new authentication interface needs to be documented and the plugin factory.
Category | ⇒ | Installation Libraries Front End Plugins |
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
As long as we don't have such a service extension end point we have to go with plugins. We are already using plugins for many different use cases and one of them is services. As long as we can't come up with a better solution this is the best way we can have for now.
We need to come up with a better solution because right now this is going down the same path of why you say having Factory::getContainer()
is a bad idea. Architecturally, it is wrong to use event subscribers/listeners as general purpose service classes.
If you want to have a service with user configurable settings (params) a plugin is the only way for now or do you have another idea, without making a new extension type. Perhaps we should get rid of the idea that every plugin needs to be a subscriber.
You would need a new extension type if you're looking for something that isn't part of the event system. Plugins are deeply rooted as part of the event system so I think it would be a bad idea to try and repurpose them in a general catch all way, or even make non-event based plugins (using the same extension type). Not saying I know what the right answer looks like in the long run, but I do know my gut says that this approach doesn't feel right.
If there is nobody which volunteers to do all of that, it will be a massive new feature because you need to write a whole component for it. Then this is the best solution for now to improve the current behavior to not create plugin instances by our own.
Thinking more about that, the only doable solution which can be realized before Joomla 6 is to break up the current plugin belts and allow a plugin to be more than only en event subscriber. So a plugin can be a service OR an event subscriber for now. We can go with the idea further and deprecate libraries and say that a plugin can also be a library plugin. Perhaps in the future we will also have a plugin of the type override which serves layout overrides. A plugin can be everything which should be globally accessible in the code and needs a UI with params to configure. If we would have that before, then the custom fields types would be much easier to implement.
The benefit of plugins is that they have a UI where the end user can define some params. So instead of setting up for every use case, where the current event driven plugin doesn't fit, a new extension type with a full blown component. We should search for a more lightweight approach.
The end user doesn't care if a plugin is a service or an event subscriber, she/he just wants that it solves the particular problem. People which are new to Joomla get confused by our different extensions (component, module, plugin, library, template), so we should not add more. The just don't know what to use.
We don't need to make everything a plugin. We have a 10 year history of plugins being directly attached to the event system, please let's not change that for the sake of things. It's easy to add new extension types, the hard part is naming. Otherwise you end up like another CMS where everything is a plugin, but the problem with that approach in Joomla is systematically not everything applies in a global scope like their extensions do.
It's easy to add new extension types
That's absolute not true and you know that, adding a new extension type with a UI, installer process and everything else is a big load of work.
And its just not end user friendly to add another extension type. It would be much easier if everything in Joomla would be an extension (plugin) which can then internally do different things. Easier for the developer and end user. But this is not the target of this pr here. Adding more types of extensions is just not practicable.
Just because it is here since ages, it doesn't mean it can not change.
This is the second time I've had this conversation about re-scoping or renaming extension types. Doing that basically wipes out 10+ years of documentation and how users are actually using the platform. Do you honestly believe that if we renamed modules to widgets and made everything plugins in 4.0 it would be better for our user base?
For better or worse we don't have a WordPress architecture where everything is a plugin. Our extension types have different scopes. Architecturally, this is correct, even if it isn't communicated to the non-technical users very efficiently. I wouldn't just go about wiping out the entire project's history just because.
Plugins are event subscribers. Nothing more, nothing less. Please let's just leave it that way.
re-scoping or renaming extension types
We are extending the plugin extension type nothing more and nothing less. No documentation needs to be rewritten, perhaps extended but nothing becomes obsolete with the event subscriber stuff. You are not understanding what I want to say. I know it would opening the plugin for more use cases.
You just can't say this is wrong without coming up with a solution which is realistic to implement with the resources we have. We all know what would be the technical correct solution but this one is not doable because of lack of resources. Also it would confuse the end user more than it helps, to identify if it needs a service or a plugin for his problem.
That's re-scoping by definition. You are changing the scope of what a plugin extension is if you make it so that it is not an event dispatcher. Which inherently changes how the plugin must be interacted with through the Plugin Manager UI (at a minimum the ordering field which controls the position it is loaded into the dispatcher at does not apply for a 'non-subscriber plugin').
Plugins can already be used for the purposes you're aiming for. Having a UI to manage a configuration for something in the system that may not directly be used as a component or module. The plugins also have the benefit of being able to load things up correctly, because files and libraries extensions don't have a way to register their assets (so inherently they'd need a plugin to to at least load their stuff).
If you're looking to provide a service library, a plugin is the way to go. The plugin gives the UI to manage its configuration, and you can use an early system hook to load it and make it available globally. What does making a plugin a non-event subscriber actually solve in the long run when you would still need an event subscribing plugin to get the thing loaded to begin with?
You are changing the scope of what a plugin extension is if you make it so that it is not an event dispatcher
What I think can be a simple solution is that according to the interface the plugin implements it can be either way a service or an event subscriber. I'm not saying here that it should not be an event subscriber anymore. All the existing plugins would stay as they are.
How would you solve then the authentication issue we have right now in core, where we need services instead of plugins?
You are still trying to change what JPlugin
is and mandate that its implementations are not event subscribers. JEvent
/JEventDispatcher
/JPlugin
are all part of the event system, just because the first two have been phased out in 4.0 doesn't mean the third one is fair game for re-use in any way someone desires. That is my issue. For the life of Joomla, a plugin extension and any class inheriting from JPlugin
has been an event subscriber. Making it possible for a plugin extension to not be an event subscriber causes a new nightmare of issues from both the perspective of the API and the UI (UI concerns are primarily that ordering field, then you have to make a distinction in the UI of an event versus non-event plugin; at the API level you have to deal with the fact that the plugin system right now will load any class in the plugin group into the event dispatcher and that the only code you can guarantee to run in normal system operation is the plugin's constructor unless you are direct calling a method from outside, which goes against a publisher/subscriber or observer type pattern, so now the plugin helper has to know how to differentiate these classes).
Using the code that we have available in the Joomla repos right now, I would make the authentication plugins proper plugins, integrate the Authentication Framework package, and the authentication plugins would be required to use one of the plugin events to register their authentication strategy to the Joomla\Authentication\Authentication
class. So you have pre and post authenticate plugin events, the authentication plugins register the strategy at any event up to the pre-authenticate event, then Authentication::authenticate()
is called to actually process the authentication instead of the system we have now where the actual plugin class instances are retrieved and used. Yes, it makes writing authentication plugins a bit more difficult for developers (it basically requires they have two PHP classes in the plugin instead of one), but at the same time that would eliminate the practice we have now of using event subscribers in a non-event driven way.
In a properly service driven platform, plugins and components would be able to register services into our DI container or the various service classes/registries/factories with relative ease and therefore things like JHtml helpers, form fields, authentication strategies, custom logger types, console commands, cache/session handlers, or really any part of the system that has an extension point could easily have those custom extensions added without requiring developers to include the component manually or manually register the service at different points or other manual scan and include behaviors that we have in the system now. The closest thing we have to that is a system plugin (because we still have this grouping notation to not load all subscribers on every request) subscribed to onAfterInitialise
.
You're looking for a way to register service classes that are not event subscribers and because the plugin classes already have a way that you can kind of do this you're proposing a change that implicitly supports making plugins non-subscriber driven. Which to me is changing the scope of a plugin. In the case of authentication plugins specifically, you are more openly making it clear that those plugins are not simple event subscribers or observers and are general service classes, breaking SRP (yes I know we are already doing this but in my eyes this PR instead of fixing the broken logic is taking a stance that we are embracing it).
You are also mixing logic in this PR that really should be separate things and not be an all-in-one package:
AuthenticationPluginInterface
- This part and the surrounding details of it is really the heart of my issue because this is the one that is really making a statement of "our plugins are not event subscribers and can be direct used as general service classes"PluginFactoryInterface
- Generally, this part seems OK, especially with your desire to eliminate everything with a static keywordAs I have pointed out in my last couple of comments, there are API and UI concerns that would need to be addressed to embrace this approach of being able to build plugins that are not event subscribers.
If there is really a desire from our community to have non-event subscribing plugins, this needs a proper roadmap. This PR to me is a half baked implementation and needs a lot more work to be a viable candidate for inclusion in core.
Not sure if I got anything right, but calling a plugin method directly seems to me wrong. For the authentication (and maybe other parts also) it might be the problem that we have implemented this as a plugin but it should be a service (just thinking out loud)
Think Symfony's container building process for a minute. You use a compiler pass to take services with a given tag and those get added to the appropriate "parent" service (i.e. Twig extensions). Since we have a runtime oriented container versus compiled container, the closest comparison we'd have to that is using plugins to register to a service during the request (presumably at onAfterInitialise
for something global or onAfterRouting
if you're going for something component specific).
Going back to my example with the Framework Authentication package. We'd have a service in the container for the Joomla\Authentication\Authentication
class, and plugins which provide authentication strategies (as they're called in that class, essentially though our authentication plugins as a whole are the comparable thing in the CMS now) would get the Authentication
class from the container and call its addStrategy()
method. Instead of direct calling plugin methods then, the authentication class calls a registered service (any class, including an anonymous one (a PHP 7 feature), implementing the interface). So instead of Joomla\Authentication\Authentication
fetching all of the authentication plugins and calling the onUserAuthenticate
method on them, it has an internal registry of strategies/providers/whatever that it loops over to process the authentication request. That's the same approach I would take to dealing with the authentication plugin issue. The plugin remains correct as an event listener, and the plugin provides an implementation/handler/whatever for a service and still has a UI that users can configure things through.
Before I started this pr, I was thinking to go the way with an AuthenticationManager with AuthServices where a plugin fills the manager with these services (probably something similar like the fw package). But then I thought it would be too complicated for the average developer for a task like authentication.
I mean it is not that I don't find it awkward to call event subscriber plugins directly but we have that with editors too. We are facing a similar problem in the media manager, and I'm pretty sure there will be more in the future. So we need to solve that in some way. Hopefully someone can come up with a global solution here.
Anyway I made a commit with the idea to have multiple plugin types and not only subscribers. Closing it as I think this will end nowhere.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-09-14 06:36:07 |
Closed_By | ⇒ | laoneo |
But then I thought it would be too complicated for the average developer for a task like authentication.
What skill level are you taking as "average"? Understanding what a PHP class is or how to create a class implementing an interface or having an instruction for Factory::getContainer()->get(Authentication::class)->addStrategy(new MyStrategy);
should be pretty basic stuff, especially if you are writing an authentication service as you have to be aware of a lot more when interfacing with that part of any platform (securely reading input data, correct password validation, if talking to a third party service handling that communication correctly, possibly making changes to the session). For existing plugins, it would literally be moving the onUserAuthenticate
logic into a new class and making minor tweaks based on whatever the interface is of the new classes that'll be required.
I do get what issue you are trying to solve. I don't think changing how plugins work fixes it. I do think there are ways you can fix some of them using the existing plugin system and its conventions, the authentication one explained somewhat fairly, but that also requires breaking changes as it relates to how authentication plugins are written (moreso because of the methodology shift than removal of an old API).
Event subscribers/listeners are not supposed to be service classes that get directly called by other objects, except for whatever is registering them to the event dispatcher (because something has to handle the object's instantiation) and the dispatcher itself. If an event listener is doing something that needs to be accessible outside the event system, that logic should be extracted to another service class and that class is called.
I don't know what the long term goal here is but I'm not entirely comfortable here with what it is you're aiming for.