@anibalsanchez In the testing instructions you write that we shall check that the page is not cached when

Page is not excluded in the plugin configuration

Can it be that this is a copy paste from the description of the case when the page shall be cached and that the „not“ in that sentence is wrong here for the not cached case?

@anibalsanchez What I mean In my previous comment is following, see the red mark:

Thanks @richard67 I've updated the testing instruction to clarify the inconsistency. Pls, check it again.

Thanks @richard67 I've updated the testing instruction to clarify the inconsistency. Pls, check it again.

@anibalsanchez Looks ok now. Thanks.

For me there is too much static stuff in here. I would just return the events in getSubscribedEvents and then do the checks if cache is enabled in the respective function once called.

Thanks for all your feedback.

Until now, people have been happy with the current plugin and the legacy events implementation. I didn't want to change how the plugin itself has been working, and the original idea has been only to use the SubscriberInterface interface. However, I'm happy to go for a full review and clean the plugin code in a way that, in the future, the same practice can be applied to the rest of the plugins.

Before investing more time in the PR, can we confirm that it will be merged if we continue improving the code? Maybe @bembelimen can give us a hint?

Me personally would like to get rid of all old events in 5.0 and replace them with the new subscriber interface version. Since we don't have a fallback from events written with the subscriber interface to the old "onXXX" functions we would need to introduce new event names.

Even if we don't remove the old event system in 5.0 we should rewrite all events to the subscriber interface including proper parameter validation.

Also I would be more strict with core plugins and mark them as final. May be @nikosdion can have a look at the events we created in #35396 and give some feedback if it's the supposed way you had in mind.

@HLeithner Let me address everything.

Renaming Events: NO!

You do NOT have to rename the events themselves. You just have to remove the legacy plugin support from CMSPlugin added by me and Michael. Its one and only job is to trawl through the public methods, find which ones start with on and register them as event handlers through a legacy wrapper.

When you remove the legacy wrapper old plugins (replying on the legacy wrapper) will stop working; they will load but do nothing at all. New style plugins, implementing the SubscriberInterface, will work just fine.

Renaming the events themselves causes a massive migration issue: it is impossible to write an extension which is simultaneously compatible with Joomla 4 and 5. This means that it's impossible for users to upgrade to Joomla 5 unless they EITHER start from scratch OR remove all third party extensions losing all related data, i.e. start from scratch.

Furthermore, renaming the events makes two decades of knowledge and documentation invalid overnight. That's a good way to dissuade new developers to come to Joomla.

There's really no good reason to rename the events and there are many good reasons to keep the names the same.

Creating Event classes: YES

That was the intention all along. I had created a few event classes for the Table events as a sampler and Proof of Concept — even more so since the scope of my original PR 6 years ago was to refactor Tags and other behaviors into “orthogonal” plugins, handling specific Events.

What you did for the Media Manager is on track with the original intention.

IMHO all core events such as onAfterInitialise, onContentPrepare (especially this event; each instance of it is ridiculously inconsistent with every other instance!) and so on should have their own Event classes. This will make them more discoverable, self–documenting and also remove any ambiguities about their call arguments.

FWIW even plugins which currently expect a generic Event object will work as the more specific event classes still extend from the base Event class. In other words, introducing specific event classes should not affect b/c. It will just make core events discoverable and self–documenting.

Once the core starts doing that 3PDs can also follow suit if they feel so inclined (FWIW it is on my to–do list for my extensions).

final in core plugin methods: ALTERNATIVE

Make the plugin class itself final, not the methods. Actually, thinking about it, it should be a documented best practice to make plugin classes final. These should not be extended.

The methods should simply be private.

Of course we cannot do any of that until 5.0. In the meantime (4.1 onwards) we can mark the classes as @internal. This phpDocumentor tag indicates that the entire class is not part of the public API and may change drastically in any upcoming version. This means that drastic changes, like what this PR does, could happen in a patch release without violating semantic versioning.

In short, we will be treating core plugins as implementation details.

Plugins need more refactoring

The core plugins are for the most part legacy plugins. Those which are not, like what this PR here does, is a hybrid. They are not full Joomla 4 compatible extensions, they are legacy plugins implementing the SubscriberInterface.

Fully native plugins would have their own service provider and a different directory structure. This means that come Joomla 5 we can remove the legacy extension support from ExtensionManagerTrait. It will also mean that all core code — including plugins, modules and templates — will be auto–loaded through the PSR-4 autoloader.

The extensions installed needs improving

There is one things that needs to be addressed, however: installing plugins. Right now, the extensions installer expects the legacy filename bar.php for a plugin plg_foo_bar. The reason is that the plugin name is retrieved from the XML manifest through the special <filename plugin="bar">bar.php</filename> line. This leads to atrocities like what I had to do here: https://github.com/akeeba/sociallogin/tree/feature/joomla4/plugins/system/sociallogin Essentially, I have a useless empty plugin file which is never loaded by Joomla just so that Joomla 4 can install the plugin following Joomla 4 conventions. Modules are the same deal, they need an empty legacy module file which is never loaded (since ExtensionManagerTrait will only use the legacy files if the services/provider.php file does not exist).

While we are at this...

There are other legacy mannerisms in core code, like using Factory::getUser() to get the current user. Understandable since the application's Identity may not be populated. I would prefer it if the UserFactoryInterface had a getCurrentUser method to return the current user without having to write a dozen lines of code in my extensions, essentially replicating Factory::getUser.

Generally speaking, the only two valid use cases for using Factory should be getting the DI container or getting the application object (maybe also the mailer object, I can't remember off the top of my head). Everything else is already deprecated and should not be used in core code.

You can copy my entire reply to the Joomla 4 & 5 roadmaps. These are the low hanging fruit to migrate core code to no longer use deprecated stuff and eventually obsolete stuff in Joomla 5 already deprecated in Joomla 4 without causing a migration disaster ?

I opened the PR to help in the migration of the legacy stuff, but now it seems that we are again talking about the next major version. Joomla 5 is on the horizon ... (Nov 2021).

Migrating the legacy stuff is intrinsically linked to the next major version. Joomla 4 deprecated stuff but Joomla 4 itself is using the deprecated stuff. What you do is a necessary step to get rid of deprecated legacy stuff. How much do we change this plugin? It depends on what we want to do with the next major version ?

I would personally go with a full Joomla 4 native plugin (with a service provider) so that we don't need to refactor the refactored code in the future and get rid of deprecated legacy in the next major version. If we are OK kicking the can on removing legacy — essentially removing support for how we were writing plugins in 2007 when J5 is released circa, dunno, 2027? — we can just add SubscriberInterface and call it a day.

You see why discussing Joomla 5 long before any code is written for it makes sense so early in the 4.x release cycle? It's all about legacy handling and making sure we don't screw up Joomla in the future.

Okok. I'll give it a second pass to clean the hybrid approach and the plugin code.

@nikosdion Thanks for your detailed description, it's very helpful. I have still some questions and comments.

Renaming Events: NO!

You do NOT have to rename the events themselves. You just have to remove the legacy plugin support from CMSPlugin added by me and Michael. Its one and only job is to trawl through the public methods, find which ones start with on and register them as event handlers through a legacy wrapper.

Oh it seems I missed this while I looked the last time, so of course it doesn't make sense to change the names when we have a working b/c layer.

Furthermore, renaming the events makes two decades of knowledge and documentation invalid overnight. That's a good way to dissuade new developers to come to Joomla.

Sadly I think the documentation would be outdated anyway and have to be reworked to match the subscriber interface, but of course it's easier to find the right event.

Thanks for pointing this out.

Creating Event classes: YES

That was the intention all along. I had created a few event classes for the Table events as a sampler and Proof of Concept — even more so since the scope of my original PR 6 years ago was to refactor Tags and other behaviors into “orthogonal” plugins, handling specific Events.

What you did for the Media Manager is on track with the original intention.

IMHO all core events such as onAfterInitialise, onContentPrepare (especially this event; each instance of it is ridiculously inconsistent with every other instance!) and so on should have their own Event classes. This will make them more discoverable, self–documenting and also remove any ambiguities about their call arguments.

What you mean exactly with "(especially this event; each instance of it is ridiculously inconsistent with every other instance!)"? When we clean up/convert the event api it would make sense to do it right this time?

Once the core starts doing that 3PDs can also follow suit if they feel so inclined (FWIW it is on my to–do list for my extensions).

I had a long talk with @bembelimen couple of weeks ago about this topic to convert all events to subscriber events until 4.2 hopefully we get some traction here to achieve this earlier.

final in core plugin methods: ALTERNATIVE

Make the plugin class itself final, not the methods. Actually, thinking about it, it should be a documented best practice to make plugin classes final. These should not be extended.

That's what I mean with my note. Mark things final that should be final, and I think plugins are nothing that what you want to extend.

The methods should simply be private.

Maybe I misunderstood this but the event listener functions need to be public.

Of course we cannot do any of that until 5.0. In the meantime (4.1 onwards) we can mark the classes as @internal. This phpDocumentor tag indicates that the entire class is not part of the public API and may change drastically in any upcoming version. This means that drastic changes, like what this PR does, could happen in a patch release without violating semantic versioning.

Actually we have no semver promise on plugins, and I don't see a reason to wait for 5.0 to change them to use the service provider interface and use namespaces.

Plugins need more refactoring

The core plugins are for the most part legacy plugins. Those which are not, like what this PR here does, is a hybrid. They are not full Joomla 4 compatible extensions, they are legacy plugins implementing the SubscriberInterface.

Fully native plugins would have their own service provider and a different directory structure. This means that come Joomla 5 we can remove the legacy extension support from ExtensionManagerTrait. It will also mean that all core code — including plugins, modules and templates — will be auto–loaded through the PSR-4 autoloader.

Yeap, that means also that all Events have to be changed to their own EventAbstract implementation

The extensions installed needs improving

There is one things that needs to be addressed, however: installing plugins. Right now, the extensions installer expects the legacy filename bar.php for a plugin plg_foo_bar. The reason is that the plugin name is retrieved from the XML manifest through the special <filename plugin="bar">bar.php</filename> line. This leads to atrocities like what I had to do here: https://github.com/akeeba/sociallogin/tree/feature/joomla4/plugins/system/sociallogin Essentially, I have a useless empty plugin file which is never loaded by Joomla just so that Joomla 4 can install the plugin following Joomla 4 conventions. Modules are the same deal, they need an empty legacy module file which is never loaded (since ExtensionManagerTrait will only use the legacy files if the services/provider.php file does not exist).

Is this hard? It doesn't sound to be or do I miss something?

While we are at this...

There are other legacy mannerisms in core code, like using Factory::getUser() to get the current user. Understandable since the application's Identity may not be populated. I would prefer it if the UserFactoryInterface had a getCurrentUser method to return the current user without having to write a dozen lines of code in my extensions, essentially replicating Factory::getUser.

Only had a quick look but shouldn't the Application::getIdentity() already be populated when events are triggered? Because it's loaded in the CMSApplication::getInstance() function using the (the deprecated) Factory::getUser() function...

Generally speaking, the only two valid use cases for using Factory should be getting the DI container or getting the application object (maybe also the mailer object, I can't remember off the top of my head). Everything else is already deprecated and should not be used in core code.

Personally I would like to see joomla being able to run multiple times in the same process, but I think that will never happen.

You can copy my entire reply to the Joomla 4 & 5 roadmaps. These are the low hanging fruit to migrate core code to no longer use deprecated stuff and eventually obsolete stuff in Joomla 5 already deprecated in Joomla 4 without causing a migration disaster ?

Will do or at least take bigger parts of it. Will see how it fits and who will do it.

One other thing. How should the validation work in the events if you look at the new media event's I added much validation into the setArgument functions because we don't have an interface for the "file-object". Is this a way to go if we don't have an interface and object that can validate the data itself. In this case something like MediafileInterface with getter and setter defined which do the validation?

What you mean exactly with "(especially this event; each instance of it is ridiculously inconsistent with every other instance!)"? When we clean up/convert the event api it would make sense to do it right this time?

We use that event everywhere, from populating fields to processing plugin codes (e.g. {loadposition}). The former can operate on any object. The latter expect that the object being passed has a text property without actually checking even though this is not a given. I think that the core plugins should be consistent in that regard and we should type the content as an object (there are still some legacy 3PD plugins expecting an array which of course breaks in PHP 8).

I had a long talk with @bembelimen couple of weeks ago about this topic to convert all events to subscriber events until 4.2 hopefully we get some traction here to achieve this earlier.

Sounds very promising!

Maybe I misunderstood this but the event listener functions need to be public.

You are correct. My brain farted; I was replying past 1 am my time ?

Actually we have no semver promise on plugins, and I don't see a reason to wait for 5.0 to change them to use the service provider interface and use namespaces.

Is this mentioned anywhere? Because if it's not there's the implicit promise that plugins follow SemVer like the rest of the CMS and its core extensions. FWIW core components do follow SemVer so it would be at least internally inconsistent to treat ONE kind of core extensions differently than the rest of the CMS... I would recommend at the very least a marketing campaign before the work even starts so that 3PDs are not caught off–guard in the off–chance they are using a core plugin in a weird way — and yes, I have seen THINGS...

Yeap, that means also that all Events have to be changed to their own EventAbstract implementation

Precisely. That's what I meant.

Is this hard? It doesn't sound to be or do I miss something?

Well, two hurdles. First, how do you convey the name of the plugin/module if not by an attribute to the filename? An attribute to the root <extension> element? Second hurdle, what happens if someone tries to use both methods at once e.g. to support Joomla 4.0 and a newer Joomla version? These are neither hard nor easy, these are decisions that need to be made and which will be binding for years to come.

Only had a quick look but shouldn't the Application::getIdentity() already be populated when events are triggered? Because it's loaded in the CMSApplication::getInstance() function using the (the deprecated) Factory::getUser() function...

Not on CLI. Can't remember exact details, it was definitely after 4.0.1 that I was creating a CLI command that kept failing because the identity was null instead of a guest user.

Personally I would like to see joomla being able to run multiple times in the same process, but I think that will never happen.

I'd rather limit the probability of a 3PD reenacting Inception on a server...

One other thing. How should the validation work in the events if you look at the new media event's I added much validation into the setArgument functions because we don't have an interface for the "file-object". Is this a way to go if we don't have an interface and object that can validate the data itself. In this case something like MediafileInterface with getter and setter defined which do the validation?

The setters should do the validation. Also make sure you call the setters from the constructor. This keeps all checks straightforward.

Whether you should implement an interface or not... It depends and needs to be examined on a case by case basis. Unless you have a desperate need for an interface (as opposed to just making sure something is an object which has a specific method) go for it. If you want to leave the event more open–ended, e.g. like what you need for onContentPrepare, don't use an interface. I have obviously not used the media manager events myself that's why I am not handing down an opinion on them. I can tell you that most of the basic events for content and core application events shouldn't have their arguments tied to interfaces because it will break a lot of things in ways that cannot be unbroken. Don't go for pedantry over flexibility. Joomla's bread and butter is flexibility.

Yeah, I know, I am a pragmatist, not a purist. Come with the territory of publishing mass distributed software. Those who want purism can write their own apps from scratch using Symfony (I won't even say Laravel; purists will self–combust on contact with Laravel's façades).

We use that event everywhere, from populating fields to processing plugin codes (e.g. {loadposition}). The former can operate on any object. The latter expect that the object being passed has a text property without actually checking even though this is not a given. I think that the core plugins should be consistent in that regard and we should type the content as an object (there are still some legacy 3PD plugins expecting an array which of course breaks in PHP 8).

OK, now I understand what you mean.

Actually we have no semver promise on plugins, and I don't see a reason to wait for 5.0 to change them to use the service provider interface and use namespaces.

Is this mentioned anywhere? Because if it's not there's the implicit promise that plugins follow SemVer like the rest of the CMS and its core extensions. FWIW core components do follow SemVer so it would be at least internally inconsistent to treat ONE kind of core extensions differently than the rest of the CMS... I would recommend at the very least a marketing campaign before the work even starts so that 3PDs are not caught off–guard in the off–chance they are using a core plugin in a weird way — and yes, I have seen THINGS...

Our b/c Promise is only for the /library folder, point 6.1.1 at https://developer.joomla.org/development-strategy.html#backward_compatibility

All PHP code in the /libraries folder which is not flagged as private is considered to be part of the Joomla API and subject to backwards compatibility constraints. This may be extended to include other PHP code, such as component classes, in the future.

The fun part of this document is that javscript has a much harder b/c promise then the php part. (6.1.2 All JavaScript functions and classes that are not flagged as private.)

Of course, allowed to break things doesn't mean we should do this.

Well, two hurdles. First, how do you convey the name of the plugin/module if not by an attribute to the filename? An attribute to the root <extension> element? Second hurdle, what happens if someone tries to use both methods at once e.g. to support Joomla 4.0 and a newer Joomla version? These are neither hard nor easy, these are decisions that need to be made and which will be binding for years to come.

Supporting 4.0 and 5.0 shouldn't be a problem (even 3.x should be possible if the service provider is not loaded). Based on your https://github.com/akeeba/sociallogin/blob/feature/joomla4/plugins/system/sociallogin/services/provider.php the loader doesn't need to now the plugin name because it the service provider has a defined name. Or we talk about diffrent topics.

Not on CLI. Can't remember exact details, it was definitely after 4.0.1 that I was creating a CLI command that kept failing because the identity was null instead of a guest user.

Yeap I think I have saw this, the CLI application doesn't load a user, the question is should the cli application has an user, but maybe that's something for it's own discussion.

Personally I would like to see joomla being able to run multiple times in the same process, but I think that will never happen.

I'd rather limit the probability of a 3PD reenacting Inception on a server...

Yeah it could be hard, I really like reactPHP and with Fibers in php 8.1 I think we can increase performance on this topic but nothing for a short or midterm goal.

The setters should do the validation. Also make sure you call the setters from the constructor. This keeps all checks straightforward.

Whether you should implement an interface or not... It depends and needs to be examined on a case by case basis. Unless you have a desperate need for an interface (as opposed to just making sure something is an object which has a specific method) go for it. If you want to leave the event more open–ended, e.g. like what you need for onContentPrepare, don't use an interface. I have obviously not used the media manager events myself that's why I am not handing down an opinion on them. I can tell you that most of the basic events for content and core application events shouldn't have their arguments tied to interfaces because it will break a lot of things in ways that cannot be unbroken. Don't go for pedantry over flexibility. Joomla's bread and butter is flexibility.

Yeah, I know, I am a pragmatist, not a purist. Come with the territory of publishing mass distributed software. Those who want purism can write their own apps from scratch using Symfony (I won't even say Laravel; purists will self–combust on contact with Laravel's façades).

Of course, breaking things just for the sake of consistency is a bad idea. Trying to force existing events into a corset also causes more problems than it solves.

Thanks for your feedback.

I have re-organized the plugin according to the comments.

• Legacy methods removed onAfterRender, onAfterRoute, onAfterRespond
• The plugin class is final
• There are three helpers now to separate responsibilities: PageCacheKeyGenerator, PageCacheStorage and PageCachingChecker
• The plugin only has the minimum code to interact with Joomla and the Helpers are focused on a single responsibility and they don't produce side-effects.
• The PageCacheKeyGenerator is managed as a singleton in the container (let's use the container ;-))

One pending pain point is that the Helpers are not being autoloaded... even when the namespace is defined in the manifest. I still have to investigate this.

Also need to re-merge :)

Since you prefer a different way of implementing the plugin, I close the PR. Thanks for all your feedback.