? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
25 May 2017

Summary of Changes

It is always a challenge when you build your first plugin, to meet the class name requirements of Joomla. With the move to namespaces, and in general to become more simple and clear, we should also simplify the plugins. IMO it should be possible to define the class name of the plugin in the manifest file. Like that the plugin developer doesn't have to take care how the class name must be. It can be freely defined.

The manifest file will then look like (removed some tags for simplicity):

<?xml version="1.0" encoding="utf-8" ?>
<extension type="plugin" version="3.7.0" group="fields" method="upgrade">
	<name>plg_fields_calendar</name>
	<author>Joomla! Project</author>
	<version>3.7.0</version>
	<files>
		<filename plugin="Joomla\Plugin\Fields\Calendar\CalendarField">CalendarField.php</filename>
		<folder>params</folder>
		<folder>tmpl</folder>
	</files>
	<languages>
		<language tag="en-GB">en-GB.plg_fields_calendar.ini</language>
		<language tag="en-GB">en-GB.plg_fields_calendar.sys.ini</language>
	</languages>
</extension>

Internally this pr reuses the filename attribute in the manifest cache to define the class name of the plugin. As example the calendar fields plugin got namespaced and the class name is defined as Joomla\Plugin\Fields\Calendar\CalendarField in the manifest cache.

Honestly I don't know what the purpose of the filename attribute in the manifest cache is, so I reused this one. This can also be changed if required.

The code in the PluginHelper class to load the plugins is now simpler as the amount of combined ifs got reduced.

Testing Instructions

  • Go to Content -> Articles -> Fields
  • Click on "New"

Expected result

The list of types should include the calendar type.

Actual result

The list of types should include the calendar type.

Documentation Changes Required

It needs to be documented that plugin classes can be defined in the plugin manifest file.

avatar laoneo laoneo - open - 25 May 2017
avatar laoneo laoneo - change - 25 May 2017
Status New Pending
avatar laoneo laoneo - change - 25 May 2017
Title
Define the calendar plugin class in the manifest file
[4.0] Define the calendar plugin class in the manifest file
avatar laoneo laoneo - edited - 25 May 2017
avatar joomla-cms-bot joomla-cms-bot - change - 25 May 2017
Category Libraries Front End Plugins
avatar wilsonge
wilsonge - comment - 25 May 2017

This feels wrong to me. This is the plugins name - not the class name (although the two are related). For example this is used by the installer to load the correct language file.

https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/installer/adapter/plugin.php#L252-L280

If you allow total freedom you're going to run into class naming conflicts. Even when we do namespacing we're still setting a clear standard on how we expect naming, just based on how the autoloader works, and so I don't see how this is going to make a big difference being a convention that is set in the XML file vs in the PHP file.

avatar laoneo
laoneo - comment - 25 May 2017

Why this approach makes sens is again to prevent class loading by the PluginHelper class. We can change the attribute for sure from filename to something else. This here doesn't matter about autoloading in the classloader. Where else would you define then the plugin class the PluginHelper can load for the plugin? It can't (and we also should come away from that) compile the classname by itself because of the vendor part. So somewhere you need to define the class to load, for plugins I don't really see another place than the manifest file.

avatar Bakual
Bakual - comment - 25 May 2017

What is the advantage of being "free" with the class name? Personally I don't see one.

I'd say for a lot of plugins, namespacing them and registering an autoloader (or own namespace) is maybe a bit to much. Like modules, often enough they are very simple extensions consisting of one or two classes and maybe a layout file.
We need a very simple and especially efficient approach here.

But then I don't know how the new event system works. Maybe that changes the game here anyway.

avatar laoneo laoneo - change - 25 May 2017
Title
[4.0] Define the calendar plugin class in the manifest file
[4.0] [RFC] Define the calendar plugin class in the manifest file
avatar laoneo laoneo - edited - 25 May 2017
avatar mbabker
mbabker - comment - 25 May 2017

I'd say for a lot of plugins, namespacing them and registering an autoloader (or own namespace) is maybe a bit to much

Part of the goal should be the only reason you have to manually load a class is if it has some awkward convention that doesn't fit in with the rest of the CMS. So regardless of how many PHP classes may exist somewhere, they should all eventually fit into an autoloading mechanism. That's not to endorse or shoot down this PR, but in general the aim should be to have almost zero JLoader::register() calls.

But then I don't know how the new event system works. Maybe that changes the game here anyway.

The new event system really doesn't affect how plugins get loaded or registered, at least in 4.0. Right now the manner of automatically registering all public functions starting with on as an event listener is a deprecated behavior for removal in 5.0, but that just affects more how you construct your plugin internals (with https://github.com/joomla/joomla-cms/blob/4.0-dev/plugins/quickicon/joomlaupdate/joomlaupdate.php being what that end result looks like) than how plugins are installed and loaded into the application.

avatar Bakual
Bakual - comment - 25 May 2017

Isn't the main plugin file (or the module one, probably same thing applies there) similar to a "bootstrap" or "dispatcher" class? Eg a non-namespaced class which then sets up the needed stuff for the extension.
For plugins and modules we don't need to load its classes from outside (like models, tables and helpers for components), all we need here is a way to load the main class. So we have a bit of a different requirement here.

Imho, the simplest thing is to have a naming convention how the main plugin (and module) file is named and let that one then deal with its namespace and autoloading (if it even has more than one class). That's actually what we have today already and can reuse. I don't really see an advantage yet in changing that part.

avatar mbabker
mbabker - comment - 25 May 2017

Isn't the main plugin file (or the module one, probably same thing applies there) similar to a "bootstrap" or "dispatcher" class?

For plugins, no. The "main" file is the actual event listener that gets registered to the event dispatcher. No bootstrap or anything else involved with those.

For plugins and modules we don't need to load its classes from outside (like models, tables and helpers for components), all we need here is a way to load the main class. So we have a bit of a different requirement here.

Plugins are supposed to be services and there is a part of the application responsible for loading those in. So at a minimum you have to have the code we have now where at least JPluginHelper knows how to import things. But, if you wanted to create a structure where you have an abstract middle layer in your plugins (like the Smart Search stuff), it becomes a little more important for there to be an autoloader concept (right now that middle layer exists in a component, could just as easily be a library class, but what if that middle layer is in another plugin?). In the case of modules, imagine the concept of them is totally revisited and an application request becomes a chained series to a bunch of MVC style things (component tasks, modules, etc.), if we ever got to that point (and I honestly would say "why haven't we had that discussion yet?" to it) then we need to be able to reach a new class of extensions in a way other than importing a "front controller" (for lack of better terms for that procedural entry point file).

Imho, the simplest thing is to have a naming convention how the main plugin (and module) file is named and let that one then deal with its namespace and autoloading (if it even has more than one class). That's actually what we have today already and can reuse. I don't really see an advantage yet in changing that part.

The two extension types are different things, let's not mix them too much.

Right now a minimal plugin is the XML manifest and the JPlugin subclass. Plugins don't have a bootstrap, entry file, whatever you want to call it. So the only way plugins can interface with the system is through plugin events. They also have to take care of manual registering all the things, there is no class name convention or auto loading available for plugins except for that main class because JPluginHelper has a hardcoded filesystem plus class name convention.

Modules are just a procedural file that may be triggered X number of times per request. They have no structure, even though most modules these days contain at least a helper PHP class, some "fancier" modules may include more. Without switching modules to a class oriented object and going for a HMVC style architecture, there is nothing to change in those right now.

avatar laoneo
laoneo - comment - 25 May 2017

Michael hit the nail. The thing is that the plugin class is an all in one thing. Basically it does the job of the dispatcher and the execution part. Gold solution would probably be to have here a dispatcher as well which registers the plugin events. Then it would lead to that a plugin can even have multiple classes whixh are listening to various events.
Target of this pr should be to find an agreement how to simplify the plugin part. I barely remember but writing my first plugin was frustrating. Till I finally figured out why my functions didn't get called. This pr here introduces basically a dispatcher. In combination with the example from Michael we have a clearer plugin system. It also opens new ways to hook into the event system, but thats then something for J5.

avatar mbabker
mbabker - comment - 25 May 2017

Well, now that's getting a little complex. Plugins are designed primarily as a single event subscriber. Some of them have just ballooned into having a lot of custom functionality which calls for having more classes to separate things in a proper way. So I don't think we need a dispatcher to push subscribers into a dispatcher.

I think we do need to be able to handle autoloading conventions within plugins (and modules, though those are admittedly less important for me as long as they are procedural code). I don't think we need a plugin dispatcher (in the same thinking as a component dispatcher), or a separate procedural file that is used to load plugin services and whatnot (that could honestly be accomplished by having a JPlugin::boot() method which is called after the plugin is constructed, similar to how AbstractApplication::__construct() calls initialise(), this way the plugin doesn't have to run a bunch of procedural code outside the class when the file is first loaded or have a constructor that is running a lot of logic not related to instantiating the class).

avatar laoneo
laoneo - comment - 25 May 2017

What would bd then the classname of the plugin? Still PlgFieldsCalendar?

avatar mbabker
mbabker - comment - 25 May 2017

Without namespace support, there's no reason to change the existing class name convention. And to be honest, without using a new directory for plugins or ignoring PSR-(0|4) we can't namespace the existing plugin classes in their existing location. So maybe right now this is something best left in the "idea for a day where we can consider it" pile, because just shooting from the hip I fear we've got bigger issues to address than whether a plugin class name is defined in the XML manifest or follows a convention as constructed in JPluginHelper.

And I don't think there's necessarily a need for plugins to support several subscriber classes in one package. You could still do it now anyway by manually registering the class to the event dispatcher and it would still be dependent on whether the extension package (as tracked by the #__extensions table) is enabled or not.

avatar Bakual
Bakual - comment - 26 May 2017

What would be the advantage of namespacing the (main) plugin class? It's not like you're going to call it from several places indivually. The only place it's called is from the event dispatcher class and there you can as well just load it like we do today, you don't gain anything. It's probably faster anyway the way it is now.
Of course it should be (and probably already is) possible to use namepsace and autoloading for the more complex plugins. But that's plugin internals then, nothing we need to bother about.

I barely remember but writing my first plugin was frustrating. Till I finally figured out why my functions didn't get called.

Honestly, that could be solved by improiving the tutorial in how to write a plugin. I don't think it's a code issue itself.

avatar laoneo
laoneo - comment - 27 May 2017

I'm going to close this one. Just wanted to get opinions how we do think about plugins in general. If we want to leave the current scheme then we leave it.

avatar laoneo laoneo - change - 27 May 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-05-27 16:13:56
Closed_By laoneo
Labels Added: ?
avatar laoneo laoneo - close - 27 May 2017

Add a Comment

Login with GitHub to post a comment