[4.2] Inject the application into the service provider plugins

[laoneo]

Summary of Changes

The application and database should not be magically set in the constructor by reflection. Instead they should be injected by the service provider.

Additionally it adds a new translate function to the base CMSPlugin class which works on the internal application instance.

Testing Instructions

Execute some actions like enable the cache plugin or save an article.

Actual result BEFORE applying this Pull Request

All is working.

Expected result AFTER applying this Pull Request

All is working.

[roland-d]

Thanks everybody

[brianteeman]

This PR breaks things :(

Adding the new protected function translate broke at least one extension with a fatal error

php 8.1 Fatal error: Access level to plgSystemN3tCookieConsent::translate() must be protected (as in class Joomla\CMS\Plugin\CMSPlugin) or weaker in plugins\system\n3tcookieconsent\n3tcookieconsent.php on line 327

That shouldnt happen in a minor release

[brianteeman]

oh and what a suprise - a pr with ZERO tests gets mnerges and it breaks

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38060.}

[wilsonge]

php 8.1 Fatal error: Access level to plgSystemN3tCookieConsent::translate() must be protected (as in class Joomla\CMS\Plugin\CMSPlugin) or weaker in plugins\system\n3tcookieconsent\n3tcookieconsent.php on line 327

In fairness this just means we've added a function with the same name that the plugin used - but they had their function as public and ours as protected. This sort of thing is fine in a minor release - it's us adding features and it not being compatible with 3rd party extensions things. In this very specific case honestly I'd consider renaming that function to just be _ to be consistent with our other methods for translations. But the principle of adding a new function even if it breaks another extension using that name isn't inherently wrong for a minor (admittedly it should have been done in the alpha phase and not beta)

[brianteeman]

Exactly the point. We should not be adding untested stuff like this so late

[joomdonation]

Shouldn't we add some kind of check here? If application is not injected, this line would cause fatal error.

[laoneo]

Would be not bad. Can you make a pr?

[joomdonation]

In fairness this just means we've added a function with the same name that the plugin used - but they had their function as public and ours as protected

Actually, if their method is public or protected, it should be fine. In this case, I guess their method is private and it is causing the error

But the principle of adding a new function even if it breaks another extension using that name isn't inherently wrong for a minor

I think we should avoid breaking extensions if it is possible. Imagine a site uses a system plugin has private method translate, updating to 4.2 will make the site broken and not accessible anymore. How about removing this translate method and it's usages to be safe?

[laoneo]

That would mean we can't never add any new function to the CMSPlugin class as it can break an extension?

[nikosdion]

Merging this PR as it is does break backwards compatibility and you have not even realised it.

Any plugins which have view templates using $this->app no longer work. This affects Joomla 4.2 in two ways.

1. First party plugins were haphazardly modified by this PR without taking into account any plugin view templates. Plugin view templates which was already using $this->app no longer work. Some of it will be discovered in the betas and RCs, some will end up breaking sites and lead to emergency releases.

2. Any template overrides in existing sites for these plugin view templates cannot be updated before upgrading to Joomla 4.2. As a result the upgrade experience for advanced/expert users will be upgrade to 4.2 -> break site -> fix broken view templates -> working site. For non-expert users it will be upgrade to 4.2 -> site is broken -> Joomla is broken.

At the very least there should be a magic __get in CMSPlugin to return $this->app if the property is not defined but the plugin implements the interface (with a deprecation target for this shim of Joomla 5.0). This would make the upgrade experience smoother insofar neither core nor third party plugins would break upon upgrading to 4.2, allowing site owners to have up until Joomla 5.0 to upgrade their plugins' template overrides AND allow third party developers to upgrade their own plugins without breaking their clients' sites.

[brianteeman]

Is it really so hard to just accept that this should not have been merged and to revert it and the hacks applied elsewhere as a consequence of this untested pr being merged

[laoneo]

As this pr added new functions it can't be called a BC break per definition, otherwise you guys have a different view than me what a BC break is. If you read that pr correctly then you would see that nothing has taken away. Magically setting $this->app got deprecated but it is NOT removed. When you remove the $app variable in the plugin then you need to properly inject the application object.

The intention was to allow the plugin developer in a clear way to set the application in the service provider. Because when a class sets it's own dependencies, then this violates a basic principle of inversion of control.

First party plugins were haphazardly modified by this PR without taking into account any plugin view templates. Plugin view templates which was already using $this->app no longer work. Some of it will be discovered in the betas and RCs, some will end up breaking sites and lead to emergency releases.

As long as the plugin declares a protected $app variable it still works and this is the case for all existing plugins, core and 3rd party ones, not modified by this pr. There is no point to force the plugin developer to remove this variable during version 4 lifecycle. But yes, I forgot to have a look on the default.php file as I did this pr and it was a mistake I did. Simple as that.

At the very least there should be a magic __get in CMSPlugin to return $this->app if the property is not defined but the plugin implements the interface (with a deprecation target for this shim of Joomla 5.0).

There is no interface to implement which is related to the application instance in the plugin. When $this->app gets removed in the plugin, then the plugin developer needs to know what he is doing and that the view files need to be updated as well. As I made this pr, I wanted to show the new way to get the application object. There was no intention to have both ways in a plugin, that $this->app should be used together with the injected application in getApplication. If this is a requirement, then yes, we need to have a magic getter. But it was not something I did consider as I made this pr.

As last point I want to say that this pr is fine as it is. Again, I made a mistake by not have a look on the default.php file. But this is really something which can happen in beta phase. I do not say it should, but it can. When there are new requirements emerged since this pr was made, than I'm willing to help and bring this further.

[nikosdion]

@laoneo Again, you are not understanding the problem. You have removed $this->app from core plugins BUT their view templates and their overrides still use it. As a result you have broken b/c because there is no clean upgrade path.

You can already see in your PR that you have broken the WebAuthn plugin by doing that for the reason I stated.

Even though the core view template was updated in another PR (fixing what you broke here) this will NOT be possible prior to upgrading to Joomla 4.2 for template overrides on existing sites.

If you replace the templates/foobar/html/plg_system_webauthn/default.php prior to upgrading to Joomla 4.2 your Joomla 4.1 site is broken before the upgrade. If you upgrade your site first then your upgraded Joomla 4.2 site is broken until you replace the file. This is not a clean way to do this.

Having a temporary magic getter in CMSPlugin would solve the problem. Existing view template overrides would still work. Moreover, accessing $this->app would generate a deprecated notice in the log allowing diligent site integrators to modify their overrides after the upgrade, without breaking their sites before or after the upgrade.

Joomla is not a classroom Computer Science assignment. It's real world software used by millions. Implementing new architectural features does not mean we have to break existing sites. See what I did in my PR for the concrete event classes, spending a lot of time thinking how we can simultaneously support legacy plugins (supported until 5.0), plugins using the non-concrete events which were the only thing available in Joomla 4.0 and 4.1 (also to be supported until 5.0) and have them work with new plugins which will be using concrete events. It wasn't easy or obvious and I could have said "nah, we don't need it, the Right Way is concrete events even if it breaks all sites". I chose to be practical, as every mass distributed software developer needs to be, instead of a pedant.

To sum it up. Yes, we do need proper IoC. No, we don't have to break existing sites implementing proper IoC — even if it means adding a bit of messy code only to have it removed in a couple of years.

[laoneo]

I did very well understand the problem. BUT with all the authentication changes you made in 4.2 I had the impression that the webauthn plugin was a new one in 4.2, so there would not exist any override for it. But it is not. Should have been checked that better.

Honestly I wanted to avoid the magic getter as I don't like them. But it will help in the transition, so I will do a pr in the next days.

Again, my intention is never to break sites or break BC, but this kind of architectural pr's don't get any attention at all. And you should know it better than me, when I have to wait for weeks till an architecture pr get merged, then we will never reach the goal to remove all these deprecated code in the next major. Yes it comes with the cost that these kind of bugs can happen. I mean during Joomla 4.0 development there were at least Michael and George here to discus. But now I'm pretty much alone.

[brianteeman]

. But now I'm pretty much alone.

all the more reason for a pull request to be single purpose and not include additional changes not mentioned in the title

[nikosdion]

BUT with all the authentication changes you made in 4.2 I had the impression that the webauthn plugin was a new one in 4.2,

Thank you for explaining the misunderstanding :)

Honestly I wanted to avoid the magic getter as I don't like them.

Me either, unless they are used to implement an immutable object. But sometimes they can be useful for managing legacy.

Again, my intention is never to break sites or break BC

I understand that and I apologise if I came across as implying that you didn't care. I know you, I know you are not one to break sites on a whim. I did not mean to offend you. I am sorry.

And you should know it better than me, when I have to wait for weeks till an architecture pr get merged, then we will never reach the goal to remove all these deprecated code in the next major.

Believe me, I KNOW! 🤣 I kept bugging Harald to get the concrete events PR merged before it's too late.

FWIW I prefer it when architectural and implementation PRs are separate, though. I've found that it makes it easier for everyone to manage and we can definitely merge implementation PRs in the lifetime of a minor version as long as b/c is not affected. Right now this is not the case. Since you do have a way to table this matter in the production leadership meetings please do. If there's a decision made that PRs implementing new full b/c architecture can be merged in the lifetime of a minor version instead of only at .0 we won't have to rush our code and produce much better results for our users.

I mean during Joomla 4.0 development there were at least Michael and George here to discus. But now I'm pretty much alone.

I am always happy to help with architecture, even without an official badge. I have the experience, I have the motivation (Joomla's architecture is why I chose Joomla over WordPress ever since it was called Mambo!), I usually have the time. At-mention me next time. If I don't have availability — because, well, life happens — I will tell you.

If you need to contact me in private before a PR you know my email, it's nicholas at either of my two sites (business and blog). If you need a video or voice call even if you just want me to be your rubber ducky I am down for it. Nobody needs to feel alone working on making Joomla better. We are the PHP CMS hippies, aren't we? 😉

[laoneo]

There will definitely be some more stuff to review in 4.3 then. Will ping you. In the meantime, have a look on #38153 which adds the magic getter.

[nikosdion]

Done! Thank you :)