@laoneo I've done both tests, the one with the scheduler task plugin to see if it still works and if we get an additional deprecated log with the PR, and the other one with reverting the change from PR #38132 and checking if your PR fixes the issue of that PR, too.

The first test was ok, but the 2nd one with the other PR was not. With your PR applied and the change from PR #38132 I get an error "Call to undefined method Joomla\Plugin\Multifactorauth\Webauthn\Extension\Webauthn::app()" in line 35 of file "plugins/multifactorauth/webauthn/tmpl/default.php" when testing as described in that PR.

Update: Ouch .. my mistake. Did not revert the other PR's change right.

Did you by accident leave () after app? Remove them as it is a variable and not a function.

Did you by accident leave () after app? Remove them as it is a variable and not a function.

Yes, I've just noticed when you typed.

I have tested this item ✅ successfully on 18e2949

Tested both, the test with the demo sleep task and the one with reverting PR #38132 .

I have tested this item ✅ successfully on 18e2949

Performed the suggested tests, works as expected. Thank you, Allon!

RTC

A new pull request has been created automatically to convert this PR to the PSR-12 coding standard. The pr can be found at Digital-Peak#22

@laoneo Here is a better example:

The original class:

class FooBarPlugin{
	protected $foo = 'bar';
}
$instance = new FooBarPlugin;

dump($instance->foo, $instance->app);

The code will produce Cannot access protected property FooBarPlugin::$foo.
That is correct behavior.

Now will add what you made:

class FooBarPlugin{
	protected $foo = 'Bar';

	public function __get($name)
	{
		if ($name === 'app')
		{
			return Factory::getApplication();
		}

		if (!isset($this->$name)) $this->$name = null;

		return $this->$name;
	}
}
$instance = new FooBarPlugin;

dump($instance->foo, $instance->app);

Now anyone have access to both $foo and $app. That hugge issue.

How to make it more correct:

class FooBarPlugin{
	protected $foo = 'Bar';

	public function __get($name)
	{
		if ($name === 'app')
		{
			return Factory::getApplication();
		}

		if (property_exists($this, $name))
		{
			throw new Exception('Access to protected/private property!!!');
		}

		return null;
	}
}
$instance = new FooBarPlugin;

Here dump($instance->app); will return App instance, and dump($instance->foo); will throw an exception.

The same bug in the PR for _db fix, that also should be fixed.

For the time being this here is correct.

Sorry, it is not, even if it work for this particular issue.

@Fedik is right. The magic getter should return values, it shouldn't modify the object and shouldn't change the visibility of inaccessible properties (private outside the class scope, protected outside the class and descendants scope). If a 3PD wants to do that they can of course do so and call the parent __get last (which is the only reasonable implementation of an overridden __get anyway).

The exception type in his example should be InvalidArgumentException, not plain Exception, to keep things semantically correct.

I would also say that the default __get implementation does still need to return null when you are trying to access a non-existent property for the reasons we discussed above.

@Fedik Are you going to make a PR or what is the next step?

@roland-d I think it's best to remove RTC on this PR and have @laoneo and @Fedik come to a conclusion on how to best handle it.

I stay with this one, If @Fedik wants to make another one, then feel free and I will close this one here.

Does anyone have an example of a plugin that fail? real one (not webauthn, that was its own bug caused by $app removing)

I have made more investigation.
And got conclusion that this fix is not need, even more: it make harm.

But I need a real example, to be sure.

Only that can fail as getApplication was introduced in 4.2, so only the core plugins can use that code for the final version. But that functionality will help in the transition to get rid of protected $app when other plugins do the switch.

So I guess there is no a real one plugin?

This one, will work as before, without this PR:

class FooBarPlugin extends JPlugin{
    protected $app;

    public function onAfterRoute() {
        dd($this->app);
    }
}

And our Debug plugin also still works.

I suggest to close this PR, and "do not search for holes in a flat floor" ?

@roland-d If you want to remove RTC, you have to change the status of the PR in the issue tracker. Just removing the label here on GitHub will make the issue tracker add it back with every comment issued there.

@Fedik Alice, a third party developer, has created a plugin, let's call it plg_system_foobar, which displays a cookie banner using the view template plugins/system/foobar/tmpl/default.php.

Bob, a site builder (see what I did there, Bob the Builder?), doesn't like the default way this banner displays so he creates a template override, templates/bobbuilder/html/plg_system_foobar/default.php.

That view template needs to add CSS and JS files to the application. The Joomla 4.0 and 4.1 versions of the view template do that with $this->app->getDocument()->getWebAssetManager->usePreset('plg_system_foobar.cookie_banner');. As a result, Bob's override is using this code.

When Alice updates her plugin for Joomla 4.2 there's no longer $this->app. Therefore she changes her view template to read $this->getApplication()->getDocument()->getWebAssetManager->usePreset('plg_system_foobar.cookie_banner');.

Now Bob the site builder is completely and utterly screwed. Let's look at the possibilities.

A. He's blissfully unaware of the change. He upgrades his site to 4.2, he upgrades Alice's plugin and his site is broken.

Bob's conclusion: Alice's software is broken. THIS IS WHAT 90% OF PEOPLE MET WITH THIS KIND OF CHANGE THINK.

B. He knows about the change. Well, what can he do about it?

1. He upgrades the plugin first. His site is broken. He has to upload a new view template override next. Unacceptable because the site is broken for a period of time.
2. He uploads the view template first. His site is broken. He has to upgrade the plugin next. Unacceptable because the site is broken for a period of time.
3. He removes his override, he upgrades the plugin, he uploads the new view template. Unacceptable because Alice's view template does not match his site at all.
4. He sets his site offline, he removes his override, he upgrades the plugin, he uploads the new view template, he sets his site online. Unacceptable because this is a ridiculous chore.

Bob's conclusion: Joomla is broken and Alice is stupid.

Alice understands this problem. Therefore she realises that is she does things the One True Joomla Way her clients will think she's an idiot and will no longer use her software. Therefore she will not update her code throughout the Joomla 4 release and bump the major version of her software for Joomla 5. Even so, she will have to provide a shim for $this->app through that major version's lifetime to allow her users to upgrade from Joomla 4 to 5. So for the next 10 years she has yet another Joomla-created disaster to manage.

And just to think that nearly two months before Joomla 4.2 was released there was a very clear warning that this would happen and a very clear way to prevent it from happening.

Alice's conclusion: Joomla leadership is a f...ing clown car that shouldn't be allowed near mass-distributed code.

You know what's worse, Fedir? This. Keeps. Happening! If you could ask 3PDs for their genuine opinion about the Joomla leadership I couldn't print their replies here even though I am comfortable using swear words in public. There are so many easily preventable issues. Why should we not prevent such an issue WHEN WE BLOODY WELL CAN?!!! Why intentionally screw over third party developers and Joomla users?! Come on, people, let's get a grip on reality. If you think the only thing that matters is the core you're part of the problem! A CMS without third party software has no users. A CMS without users does not exist. That's the reason why we are here contributing to Joomla, not Mambo.

Back to pending as stated here #38153 (comment) .

With the issue @Fedik pointed here out that the variable becomes then public accessible, how should that situation being fixed? Because since PHP 8.0 the magic getter has to be public.

Okay I see, so it about to cover asses for 3d developers who ignores posibility that someone use "override" for their extension ?

Use magic _get it is a bad fix, unfortunately. Because it changes object behavior a lot.
Then we probably should always create $app property if it not defined, that is a "less evil'

@Fedik No, mate. It's to cover our bases because people WILL override view templates, DEMAND that they can overwrite view templates and there's no good fucking reason to break their sites just because you want to be a pedantic dick about it. Do I have to swear for y'all to get it? Fuck's sake!

3rd party devs are like cats: If they can do something, they sooner or later will do it.

@laoneo Without opening an IDE I'd say something to the tune of

public function __get($name)
{
  if ($name === 'app') {
    return $this->getApplication();
  }

  throw \InvalidArgumentException(sprintf('You cannot request the inaccessible or undefined property %s on %s', $name, __CLASS__));
}

@nikosdion problem with that fix, if plugin do $this->foobar[1] = 2; for non existing foobar property, then plugin will fail.
Or anyone try access plugin instance from outside with the similar code.

I would just always add $app property then to every plugin, that more safe in theory.

@richard67 That's the entire point of having third party code. To do what the core can't or won't do. Without 3PDs like me the core wouldn't have quick icons, paid extension update support, a valid way to update the core without breaking on most shared hosting, TFA/MFA, WebAuthn, security checks for uploaded files and so on and so forth.

And, just like cats, we also catch bugs and kill them for the fun of it, without making any difference in our diet ;)

problem with that fix, if plugin do $this->foobar[1] = 2; for non existing foobar property, then plugin will fail.

What are you even talking about, my good man? We create a __get, not a __set. The code you wrote would call __set. In the absence of __set it will create the property just fine on PHP <= 8.1, throw a warning on 8.2 <= PHP < 9.0, throw a fatal error on PHP >= 9.0.

If you are worried about the very different code

$this->foo = $this->foo ?? 'something';

you can change the code I wrote on the fly, in 30 seconds, without even opening my IDE to this:

public function __get($name)
{
  if ($name === 'app') {
    return $this->getApplication();
  }

  return null;
}

Try this:

class FooBar{}

$c = new FooBar;
$c->_cache[1] = 'foobar';

var_dump($c->_cache[1] ); // Will be "foobar"

And then add __get:

class FooBar{
  public function __get($name){ return null; }
}

$c = new FooBar;
$c->_cache[1] = 'foobar';

var_dump($c->_cache[1] ); // Will be NULL

By introducing __get we changing behavior. And that will be another surprice for 3d developers.

@nikosdion Have I mentioned yet that I like cats?

@richard67 I have two of them. They are the most spoiled cats ever. They get triple filtered, UV sanitised water from a dedicated water fountain and the best balanced diet wet and dry food. Somehow this is not enough. Seriously, it's much easier to merge a PR into Joomla than satisfy these two mad lads.

@Fedik In that case we're kinda screwed. The only possible course of action is to undo the application and database changes in CMSPlugin, redo them in 5.0 as they are backwards compatibility breaks.

The only semi-passable alternative (but NOT really) is to always set $db and $app in CMSPlugin and populate them in setDatabase and setApplication. There is only one major caveat. If a misguided soul does $this->app = $something and $something is NOT the same application as the one returned by $this->getApplication() the new and the old features are out of sync. I have NEVER seen that happening in the wild and I've troubleshooted quite a lot of code. I would personally live with it and prefer it over deferring the changes to 5.0 but I am not in a leadership position so my opinion means sod all.

The only semi-passable alternative (but NOT really) is to always set $db and $app in CMSPlugin

Yeah, that what I think also. It will be "less evill"

What if we code like this https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Application/ConsoleApplication.php#L159 ? I see we use that approach in many places in Joomla and it also cover the case @Fedik mentioned.

it also cover the case Fedik mentioned.

For #38153 (comment) it still will be NULL in result ?

Hmm, it's not null in my test code.

If we do it like in the Application class, then we need to probably fix a lot of undefined properties in core models and also 3rd party extensions. Which actually would be a good thing as they will fail in PHP 9 anyway.

Hmm, it's not null in my test code.

hm, maybe it also depend from PHP version, I tried on 8.1

Not sure, but not works for me:

https://onlinephp.io?s=s7EvyCjg5UrOSSwuVnDLz3dKLKrm5VJQKChNyslMVkgrzUsuyczPU4iPT08t0VDJS8xN1axWUKjl5QIiXi6VZAVbhbzUcqhWa5CIrl18cmJyRmq0YSxQUj0tPz8psUjdGqS8LLEoPqU0t0ADVZWmNQA%2C&v=8.1.7%2C7.1.33%2C7.0.33

@Fedik @joomdonation Guys, when in doubt use 3v4l: https://3v4l.org/d7IDK

The code only breaks when you try to assign an array key to a non-existent property. That was Fedir's example we were discussing before. If you initialise the non-existent property to an empty array it will still work with the magic getter.

The only semi-passable alternative (but NOT really) is to always set $db and $app in CMSPlugin and populate them in setDatabase and setApplication.

But this will produce an error when they are not declared in the class in PHP 9. This will mean we are forcing then for all plugins to have these variables declared.

@laoneo Instead of being optional properties they will be hard declared as protected properties in the CMSPlugin class.

Ok, that makes sense.

Would it then not crash plugins which have declared that property private?

You are right.

Therefore you need to revert your changes to CMSPlugin (and defer them to 5.0) as they are breaking backwards compatibility and every possible solution also breaks b/c.

The changes can stay, but the webauthn needs to be reverted back because of the template files. If it is only a plugin class without view templates then we can migrate them to the service provider. Or did I miss something?

I have already explained this too many times. Do whatever you want but I reserve the right to say “I told you so”.

Or did I miss something?

Yes. Everything above

@brianteeman thanks for your input. Looking forward to your tweet.

The issue I have is, if we don't have a way to give the plugin developers a path to prepare their plugins in 4 for 5 and they must then have a version for 4 and 5 with a different code base, then I see this as a big problem.

That is a perfctly valid point. However

1. we still dont have a roadmap for 5
2. This is not th time in a release cycle (beta3) to make this kind of change

One big point in the roadmap is to remove deprecated code (not decided by me). And then you wont have Factory::getDbo. Better to start thinking about it now instead when it is too late. So we should fix our code base step by step. And this is what I'm doing.

One big point in the roadmap is to remove deprecated code (not decided by me

Where does it say that? #38043

To be fair, until now, unless I'm very wrong, I still don't see any backward incompatible changes :

• The bug which I fixed on plugin layout on PR #38132 belong Multi-factor Authentication - Web Authentication . Isn't that a new plugin added for 4.2 ?
• Every plugins which use $app property will still work as before
• If a third party developers do not want to switch to new structure, he can still keep the code as how it is, it will still work well, at least until Joomla 5. If they want to switch to new structure, and his plugin has layout file which uses $this->app, he can just add magic __get method to his plugin. Couldn't we provide documentation for this migration and it will be OK ?

@brianteeman Removal of deprecated code in new major releases has been a standard practice in Joomla (and software development in general).

I 10000% agree. I am referring to the comments about it being in the invisible roadmap. But I give up. Today is not a good day and I dont have the energy to explain again

I don't drink any coffee at all!

Please read the doc block of Factory::getContainer https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Factory.php#L204, then you will see that this is not a replacement. This should be deprecated right from the beginning, read more about it here #18894. After 4.2 gets released then it is time to deprecate that function as we have now a setup where we can inject the dependencies on all parts in our code base. There are still things left like access and the helpers.

@laoneo I am tired of your belligerence and semantics. The question was, if we remove Factory::getDbo is there are any other way to get the object and the answer is yes, through the container. I am sorry I didn't write a 5000 word diatribe on the different semantics between injected and magic-globally retrieved objects, I thought we are adults here.

Regarding removing Factory::getContainer(), are you kidding me?! Are you even in touch with reality? Have you seen how Joomla is used in the real world and what's the skill level of the average extension developer out there?

How do you get a user object or do simple DB lookups in a static method of a Helper class? I don't want to convert this to Traits, it's pulled out into a helper class so I can have an in-memory cache of expensive lookups across different objects which do not know about each other (therefore it cannot be a Trait). Having me do a contortionist act in my plugins to get the MVCFactory of my component so I can get a model which returns a factory for my helpers is slow as fuck and far exceeds the skill level of the vast majority of Joomla developers.

Speaking of models, how does a model, let's say a DatabaseModel, convert a user ID to a Joomla User object without access to the Container now that both Factory::getUser and the User::getInstance are deprecated?! Remember that the MVCFactory does not let me inject a UserFactory, the UserFactory is not accessible through the application and DB objects I have access to and I cannot change what is being injected without forking the MVCFactory (BOTH the provider AND the factory itself!) which is a big massive no-no!

What you are doing will mathematically lead in developers either leaving Joomla for WordPress en masse (I can't blame them) or trying to work around the problem with bad code.

You first need Joomla to inject dependencies on instantiated objects implicitly based on their type hinting instead of explicitly, i.e. your DI Container must act like an actual DI Container instead of a service locator.

Exactly because we have a service locator pretending to be a DI Container we need, for example, the fugly code in MVCFactory which looks for interfaces and performs explicit injection of dependencies using setter methods provided by these interfaces instead of implicitly injecting dependencies in the constructor.

So you are trying to remove access to the service locator without providing access to the services and without providing a DI Container. This is idiotic. Sorry, but it has to be said.