-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4.x] Architecture: CMSPlugin, autoloading language will never use $this->getApplication() #39077
Comments
I like the |
Addition to what I wrote above. We can have CMSPlugin implement BootableExtensionInterface and use boot() instead of Plugins are loaded via \Joomla\CMS\Extension\ExtensionManagerTrait::bootPlugin which calls \Joomla\CMS\Extension\ExtensionManagerTrait::loadExtension. After the extension is loaded (which for plugins calls \Joomla\CMS\Extension\ExtensionManagerTrait::loadPluginFromFilesystem) the loadExtension method checks if the extension implements the BootableExtensionInterface. If it does, it calls the Now, back to your question. Can we do late initialisation? I will say that this is already possible and up to the developer, not the framework, as it should be. Please read the blue info box in https://www.dionysopoulos.me/book/plg.html#plg-forms-j4-classic What I describe is a way to delegate the lengthy initialisation to service providers but instead of pushing the services I can pull them so they are late-initialised when I first try to use them. If you really need the framework to nanny the developer you would have to add a shim event handler with PHP_INT_MAX priority for every event name the plugin supports (and which would only really work on plugins implementing SubscriberInterface). The shim event handler would check an internal flag to see if it's been called before. If not, it would call I have mixed feelings about this. The shim event handler would essentially double the execution of callables made by the event dispatcher. That's a lot of context switching which makes me think that it would be worse for performance than the alternative described above. I am also philosophically opposed to frameworks nannying developers. There is no good way to prevent a bad developer from writing bad code without making it extremely hard for a good developer to write good code. You also cannot stop a determined idiot from just hijacking Far more is to be gained by having practical documentation and example code than trying to nanny developers. Disseminating information is how we empower developers and improve the ecosystem. Turning a “n00b” (and I say this with love and affection, for I have MOST DEFINITELY been one) to a decent developer and a decent developer to a good developer has far more positive impact than getting in the way of newbies. The former empowers and helps people and the ecosystem grow. The latter dissuades, punishes, and drives away newbies, making the ecosystem stagnate and wither. Maybe I'm an idealist but isn't being idealists why we all became FOSS developers? |
Sure, using the
Comes hand in hand with good documentation 😉 I really need to see an example, how this would look like, but sounds like a nice approach. |
If you are asking for a late initialisation service as an example, I can whip up something for you. The use case I mentioned in the documentation I am writing (forex conversion) is a real-world example I have some code for from an older project. I can turn this into a plugin and write a small component to demo how late initialisation works in practice. Please confirm that's what you are looking for and I'll get to it :) |
Or we can use a core plugin like one from the task group? So you don't spend time on something which will be thrown away. |
There are no core plugins with a lengthy initialisation. The task group is an especially bad example as these plugins have zero initialisation and only execute when the Schedule Tasks scheduler launches a task. The performance impact is far more a third party plugin issue. I have seen plenty of plugins —even some I write myself a long while ago, guilty as charged— doing heavy initialisation with DB queries, requests, and whatnot in the constructor. These are the plugins which have a massive impact in the page load time of every page. I'd rather show how a really horrific example can be improved by moving code to a service, initialising it lazily, and add caching. Since I am writing this I can also document it and we can adapt that documentation as a communiqué to 3PDs. In fact, we should do that for all the architecture improvements in Joomla. Looking at the code changes is overwhelming and hardly informative on the intent and practical use of it. If we could have a monthly newsletter for 3PDs with what's changing in the core that has an impact on and/or helps 3PDs —or a long JCM article— it would be massively helpful. I had mentioned that in passing in a meeting with @bembelimen and @softforge. Maybe three or four of us who get to deal with under the hood stuff in Joomla could pull this off? Realistically only 2 people are necessary but it needs more to reduce the bus factor. It would take about 2 hours a month from each of us and would definitely help shift the tone of the discussion around core code changes. I can volunteer for that :) |
Fine for me as well, when we have a 3rd party example. |
Take a look at https://github.com/nikosdion/plg_content_forex In the initial, naive version (tagged 1.0.0) you will see that the plugin simply creates the ForEx service in the constructor. This makes an HTTP request on every page load. Bad. In 1.0.1 we promote the naive service and our helper to services which are registered in the DI container and pushed to the plugin by the service provider. The same problem exists, we just pushed the slow code from the constructor to right after constructing the object. In 1.0.2 we do lazy service initialisation using the bootable extension interface and the boot method. The problem here is that we essentially hijack the boot method to store a reference to the DI container which we then use in getters to pull the services we need. Not a very clean solution. In 1.0.3 I simply added forced 24 hours caching in the service to show how to avoid constantly doing requests. In 1.0.4 I do it the “right” way. I have created a custom factory service which creates the ForEx and Formatter services used by our plugin. The factory is pushed to the plugin by the service provider. Since it's a factory, the services with the time consuming initialisation are NOT instantiated just yet. The plugin itself has getters for these services which return the existing instance or, if no instance is created yet, go through the factory service to get them. The key takeaway is that doing it right is complicated. Realistically speaking, 3PDs won't do that. The hack with the boot method? Yeah, I can see some of them doing that. If I wanted to tell people something is that they should be using this kind of getter I am using, even if they are instantiating their services in the plugin code itself instead of going through a proper factory. It's still better for performance. One final point. This is ultimately a bad example of showing what we can do with the boot method to split the plugin's constructor into early and late stages (which is what this issue was about). I think that maybe a PR against 5.0(?) would be best. At least this tangent here lets us show the many ways you can implement services in plugins and how someone who gets the Joomla internals (and code architecture) can work their way through refactoring a functioning but badly performing plugin into an elegantly architectured, speedy plugin. If you have feedback on something which could be improved with regards to service instantiation feel free to tell me. I pulled off the first tricks I had at the top of my head and within the limited time I could spare today. |
Can we ask the service provider (maybe with a seperate function) for the events to register? for the service provider this could be a static call to plugin::getSubribedEvents, this would allow us to initial the plugin not before it's called the first time. |
@HLeithner Writing the code to do that is not hard. I have a very good idea of what it would look like in my head — including how to make it backwards and forwards compatible. However, I said why this could be a very bad idea a bit earlier today:
Think about how many event handlers we have in a typical page load and how many times they are called. Each one of them would now be checking if the plugin is initialised and, if not, initialise it. I am afraid this would be a case of the remedy killing the patient faster than the disease it cures. |
I'm not so sure on this topic we have a bunch of systems plugins which are always loaded but never be triggered. |
It wouldn't be something that's loaded and not executed, it would be something that is executed every time any event handler handles any event. That's why I am saying it will be a performance hog. Remember, this workaround doesn't make the plugin initialisation disappear, it simply moves it — by adding code to execute in every event hander every time it is executed. Therefore we are adding more code to execute on every page load. By definition it will be slower. How much slower? Normally, you'd have to write the code before you can benchmark it. Even so, just the core CMS wouldn't give you a good idea. There are far more 3PD plugins on real world sites than there are core plugins. Just Admin Tools has two dozen event handlers. Content plugins are worse, they are called all over the place. A page with a lead article, ten summary articles, ten links and a latest articles module with ten entries calls three content events' handlers 31 times. If you have ten content plugins (not unheard of) that's already 310 calls which is much more than what you can realistically benchmark in lab conditions. If you want a good approximation you'd have to write an approximation of this code and run 10,000 repeats on 10 calls times 50 different event handlers spread into 10 different pseudo-plugin classes to get a good approximation of a real world site. I believe on a 4-year old laptop-class i5 (which approximates an average shared hosting server) it would slow you down by 5–10 milliseconds, i.e. 5% increase in page load time. In the end of the day do we penalise sites using software written by developers who know what they are doing and care about performance to improve sites using software written by developers who don't care about performance and will not even use this code anyway? It sounds a lot like penalising the people doing it right and doing nothing for the people who cause the worst of slowdowns in page load times. Not to mention that most of these people make template frameworks and we can't even threaten theme with delisting from the JED or something to that effect. |
that's the reason why I would like to see a benchmark, if you look at how complicated is now to write a simple plugin it's already a pain for developers with 0 benefits. Having a delayed initialization has at least the possibility to bring something useful to this complex structure. I don't say that you are wrong but it would be great if we could find a way to reduce plugin overhead somehow. Auto loading the language for example is expensive for a plugin which is maybe never triggered. And yes I know how benchmarks works, I know the code must be written before you can test it ;-) |
If we're questioning this stuff now we're on the higher PHP version is there any need for these base plugin classes to actually exist anymore and instead strategically migrate them to traits so we're back to everything extending from CMSPlugin? |
@wilsonge OK, that's a good point. Please tell me if I am reading this right. In Joomla 5 we could have CMSPlugin simply be a "stupid" and deprecated class which just uses a bunch of traits. Plugins are expected to migrate to the traits. In Joomla 6 we'd drop CMSPlugin and all plugins implement the traits they need. I'd be thrilled with that. But how does that help with late initialisation? We'd have a trait which replaces the entire early initialisation code and adds the hooks to run before each event handler? |
Just to make sure we are all on the same page, I'll start writing some code for late initialisation, with and without event autoloading, against the 4.3-dev branch so we can play with it, benchmark it and see what gives. I am not doing it against the 5.0-dev branch because it is currently uninstallable (Composer spits PHP version compatibility errors for a bunch of dependencies, most of which I will address in another PR another time). |
What for traits do you have in mind? Because the functionality from the |
For starters I am going to refactor the legacy $app and $db features in a Trait. Legacy listener registration will also be broken down in its own Trait. I will then see if I can't break down the rest of the CMSPlugin functionality into self-contained units. For example, I would very much prefer it if a plugin was using a LanguageAutoloaderTrait instead of setting a flag. I am not sure if it can't be done in a b/c manner but it's worth giving it a go. |
I don't think so - I certainly would remove all the CMSPlugin subclasses to traits like Fields and ActionLogs as you say. However for CMSPlugin itself it's a bit more tricky (I think) The constructor for now in old style gives an easy way to access the plugin params and I'm really unhappy with re-quering the database like we do at the moment here https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/cache/services/provider.php#L39 when we already had this info booting the plugin. I think that constructor is part of the dual-initialisation question further down Also registerListeners despite being a 4 line piece of code - doesn't seem so terrible to keep that in a class rather than a trait https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Plugin/CMSPlugin.php#L194-L201 alongside DispatcherAwareTrait. CMSPlugin would literally be a 10 line class however (and not compulsory - just suggested to reduce developer burden)
Not sure yet! I had a more radical idea on that but wanted to think about it more on your code base before I wrote something up (plus I see the subclass issue as separate from delayed initialisation so if you hate my radical ideas I want us to not discard this plugin class trait stuff) |
You are right. It's used as a return hint and as an instanceof check in tests and, crucially, the scheduled tasks. It can't be removed. BUT it can be simplified to near-trivial status and that's a good thing indeed.
It does not re-query the database. Active plugins are loaded in one query and cached in memory. See the first three lines of \Joomla\CMS\Plugin\PluginHelper::load. This has been a major PITA for me trying to disable loaded plugins in some very exotic cases which I won't detail here as I don't want to give sinister ideas to other developers 🙊
We don't even need it! We can replace it with three lines in PluginHelper, as it should be. This method is a remnant of my original Plugins-using-Event PR from 2015. When SubscriberInterface was introduced by Michael you should've removed my method. Never too late to do it. I've already written that code. You'll see when I do the PR.
I can give you 120 lines out of which only 18 are actual code, the rest are whitespace and comments. Will you take it? 🤪 Look, if we didn't have the weirdo editors-xtd plugins I could actually reduce that to 10 lines of executable code but let's address one major legacy beast at a time.
There is nothing wrong with being radical while maintaining b/c and setting ourselves up for easier maintenance of future releases. For real, it is a VERY good idea doing both our ideas at once in the same PR. This was actually the missing puzzle piece I needed to make all of my ideas stick together, so thank you! |
OK so I'll start with devil's advocate - how crazy would it be for these "file downloads in a plugin" things that it should be a scheduled task rather than part of the plugin constructor (whether delayed or not) - it would also solve the hidden plugin parameters being a time cache issue too - because they would only be called at the right time AND outside of the page load time scope. Specifically thinking about the forex plugin here - obviously language file loading is a touch different because it's more intrinsic. I'm sure with the right sort of code interface we could force an initial run of that task on plugin install to ensure no chicken/egg scenarios to preempt anything like that. |
In theory, NOTHING stops us from putting this kind of initialisation code in a scheduled task. In practice it would be phenomenally bad and cause all kinds of borkage that 3PDs would have to provide support. Practical example. My silly example plugin for foreign exchange requires getting the exchange rate data at least once a day (a real world plugin would check every hour). If this data is out of date you have a problem in that you display the wrong price to international customers. If the exchange rate is in your favour it's annoying but not the end of the world. If the exchange rate is against your favour you are losing money and you start moving towards the developer of the plugin with a pitchfork and a torch. The developer can't do anything for your stupidity which led you to either NOT set up the scheduled task he documented is required, or removing it, or disabling the scheduled task plugin. Clearly, the only REASONABLE defence for a 3PD against that idiocy is to do the scheduled download during a page load, knowing that one page load per day (or hour) might suffer but he will not have to deal with a pile of stupid issues from users who shot their own two feet.
This is not what I am worried about. I am worried that scheduled tasks require a plugin and a scheduled task. Unpublish either and the whole thing falls apart. We can't have plugins and tasks even the Super User cannot unpublish. Even if we did, people mess around with the database. Having to check if the plugin is published, and the tasks exists and is published every time we initialise a plugin is actually far more disruptive in terms of performance than working around it with code straight in the plugin itself. Believe me when I say I have been there and tried that with even simpler, less critical stuff and saw users do unspeakable things… Besides, this is just one example and it's a fairly obvious one — also something which can probably be addressed with Fibers in PHP 8.2 and later. What about the plugins which need to load a bunch of database stuff and do lengthy operations on them? For example, an e-commerce component may have a content plugin to display prices. This requires pre-loading the sales tax calculation information and resolving it to the current user's detected or declared location. This is a lot of database queries and processing, to the tune of 100msec. Ideally, this should be a service instantiated via a factory as needed — as I did in the example plugin I demo'ed earlier in this thread. In practice, 3PDs put that code in the constructor. I know for I have sinned like that myself (and fixed that ages ago when I realised that what I am doing is stupid). Incidentally: The above use case is best served by caching the data which requires lengthy calculations with an expiration time of 1 year and clearing that cache whenever the data it is calculated from changes. The plugin can do the lengthy operation the next time it loads, in its constructor, and cache the results again. In most practical use cases this means a short delay for exactly one page load every several days to months. Of course this requires 3PDs to understand how Joomla's cache works even when disabled in Global Configuration (gasp!) which is a whole different discussion on how to best teach Giving them a
This is largely a non-issue. Instead of hidden parameters we can use a table. Or, maybe, Joomla could provide a transients management like WordPress. The only question about the latter feature is whether 3PDs would use it responsibly, storing small amounts of fairly short scalars to ensure loading the transients doesn't break the site / runs us out of memory. This is something I was thinking about today but I've not come to a conclusion just yet. Stay tuned for more ideas from Nicholas's Mad Labs experiments… 😈 |
So some of George's mad lab experiments (TM) to come are exactly going to be on plugins like this sort of scenario. We have several places in core where plugins are just hard linked to another extension (e.g. finder component relying on the finder plugin, fields on it's system plugin). All those template engines with the crazy system plugins to hack in their UIs into com_templates. And we are already putting users in this scenario. I think there is space for us to be able to specifically have plugins who's disabled status is hard linked to a parents extensions status (e.g. you can disable the forex plugin which would disable the scheduled task - but the scheduled task couldn't be disabled standalone (admittedly I can't stop people disabling the scheduled task component outright during it's lifetime but 🤷♂️ )). Similarly for these sorts of core plugins - we can allow the parent component/template to be disabled and in that case the totally dependent plugin is unpublished too. Don't dispute there's scope for abuse - but I think we're already in the case you state without dependencies - we just don't want to admit it. The first step of a problem is admitting it :P |
I also told you my ideas were radical in my self defence :D |
I am EXTREMELY reluctant in proposing plugins which cannot be unpublished. What happens if the plugin has a bug and needs to be disabled? The stack trace would lead the user to a plugin which cannot be disabled. Most users would be frustrated. Some users will head to the database and unpublish it, or delete its DB record, or delete the plugin files. Users go medieval too often for comfort. Remember, we can already have plugins which cannot be unpublished by writing a system plugin which re-enables itself when unpublished. I have one 😉 A determined user will still figure out how to disable it. Sometimes without reading my documentation for the recommended and reversible way to do it which causes a lot of its own problems. Whoopsie. I appreciate the radical nature of your idea, but I don't think it will solve any practical 3PD issue. In the end of the day, if the solution is a few orders of magnitude more complicated than the current hack-ish workaround the hack-ish workaround will persist. |
We already have quite a few |
@brianteeman I don't mean plugins which if unpublished crap will break. I mean plugins for which the unpublish button is deactivated in the Plugins page. Yes, I am aware that Joomla has plenty of plugins which should “always” be published. Of course there is a qualifier here about what “always” means. In reality, NONE of these has to be published no matter what. All of them are optional. You can of course decide you do not want to use tags, or fields even though doing so is not recommended. You unpublish the plugins. You may decide you do not want scheduled tasks; you unpublish the plugin. Heck, you might decide you do not want Joomla to be updated, ever, nor do you want to have extensions installed because you do a deployment from a staging site and nobody should ever be tempted to change the live site; you unpublish the extension and installer plugins. Everything that sounds absolutely cuckoo has a use case for someone. The question is, therefore, largely philosophical. Do we want users to be in absolute control, even if it means that they can brick their own site, or do we want to restrict them “for their own protection”? I think that most if not all of us participating in FOSS are philosophically aligned much more with the former than the latter. That's why I would never propose a feature which takes power away from the user, even if by doing so it would prevent a stark minority of users from shooting their feet. There's an old UNIX adage. You do not stop the user from shooting their own feet. Your job is to provide them with the biggest, meanest gun which will shoot their feet clean off. |
i disagree - I think it’s about owning your data and certain flexibility but good UX is around trying your best to stop people shooting themselves in their feet :D Either way I defer to a completely impartial head of UX - hi @joomla/joomla-experience-team-jxt |
@wilsonge I agree about good UX. But you don't propose a good UX, you propose a jail. When did Joomla become Microsoft / Apple / Google? A better UX would be a message to the tune of “Are you sure you want to disable this plugin? It will make X Feature / X Software no longer work as intended” when you try disabling it — because you may be SURE that you don't care about this feature anyway! On the same note, it would make perfect sense to list the cross-dependencies of each plugin (Depends On, Is Depended On By) so users can make informed decisions. It's not just about owning your data — you do own your data when using Microsoft Word, technically the DOCX format is open, but I'd be hard pressed to call using it “free as in freedom”. The F in FOSS stands for Freedom and it's been explicitly explained by the FSF as Freedom of choice. You are not free to choose which plugins make sense for your site if your only "choice" is to edit the database or delete folders to disable plugins. Nor is it a real "choice" if you have to hack core. These are different ways of restricting use cases to the privileged cast of Developers. I think I am far more radical than you, mate 😉 |
Closing as having a pull request. Interested readers please test #39088 . Thanks in advance. |
The way plugins are supposed to replace the
$this->app
property is to push the application object into the plugin object in theservices/provider.php
file. Example:However this creates a chicken and egg issue if you have set
$autoloadLanguage
in your plugin.If you did, then the constructor of the plugin runs
loadLanguage
. However, this requires going through the application object to get the current language object and load the language files of the plugin.At this point we have NOT yet pushed the application, though.
Therefore it will always go through the fallback code of
Factory::getApplication()
.Only then we have a plugin object where we can push the application object using
setApplication
.Therefore I have to override the constructor, add a third argument which expects a CMSApplication object and push the dependency in the constructor, e.g.:
On the face of it, it's a pain in the posterior and inconsistent amongst my plugins.
However, there's a more sinister problem. When Joomla has a base plugin class (e.g. Finder, ActionLog, …) I cannot know if its constructor will add a third, fourth etc parameter in the future or not. If I add a parameter now and Joomla does the same but with a different signature PHP 8.1 (minimum for J5) will raise a FatalError.
This makes it impossible for me to write code in a way which will predictably work in Joomla 5 even though I understand Joomla's internals, track its development, and even contribute towards its architecture. If I am having a hard time figuring out how to do things the right way with all those qualifiers in my favour can we really blame other third party developers for being at a loss and (rightfully) complain that Joomla 4 makes their life hard? Plugins are supposed to be easy to write. They are not anymore. I am afraid the bar has been raised too high, made into a hoop, set on fire, and Joomla is asking developers to jump through it.
In my opinion, some parts of the plugin constructor need to move into a different public method (
initialisePlugin
, for example) which is by default called by the constructor unless a control protected property (let's call itautoInitialisePlugin
) is set to false.This way the default case of a CMSPlugin would still work as it works today BUT if I find myself in the chicken and egg problem I can set
autoInitialisePlugin
to false in my plugin's class and have my service provider read:This solution would be both fully backwards compatible AND forwards compatible AND improve developer experience. At the very least, we can document that this is how plugins' service providers need to be written. Developer won't necessarily need to understand why. They can blindly follow a tutorial and succeed instead of needing a major in Computer Science before they even start writing simple plugin code.
Tagging @laoneo @HLeithner
The text was updated successfully, but these errors were encountered: