[4.2] Resolve application variable when it doesn't exist in base plugin #38153
Conversation
|
@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.
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. |
Yes, I've just noticed when you typed. |
|
I have tested this item ✅ successfully on 18e2949 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38153. |
|
I have tested this item ✅ successfully on 18e2949 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38153. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38153. |
|
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 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 The same bug in the PR for |
|
Then make a suggestion in the db model with your code and try to edit a module in the back end. When it works your way, then I'm fine with a pr and you can also an alternative here. No problem with that. 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 |
|
@Fedik Are you going to make a PR or what is the next step? |
|
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. 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. |
|
@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 |
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. |
For #38153 (comment) it still will be |
|
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. |
Not sure, but not works for me: |
|
@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. |
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”. |
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
|
|
One big point in the roadmap is to remove deprecated code (not decided by me). And then you wont have |
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 :
|
|
@laoneo Drink more coffee, mate. As a developer I define $app in my plugin, m'kay? My constructor can do something along the lines of Big fucking deal. In the grand scheme of things and the shit we have to eat with a straight face to make our software compatible with two major versions of Joomla this is NOTHING. It's barely a problem. Even a developer who doesn't quite "get it" will come up with a simple solution like When your changes only affect magic population of nice-to-have properties containing basic application facts do keep in mind that most developers were not even using them because they are not actually documented anywhere (and very few bother reading the bloody constructor's code!). Even those of us who know about these magic properties have had brainfarts and legacy code going through the Factory so we're used to doing that if we need to. About your comment regarding Factory::getDbo being removed, yup, but you'd still have Factory::getContainer()->get('DatabaseDriver') which already works since 4.0. @brianteeman Removal of deprecated code in new major releases has been a standard practice in Joomla (and software development in general). The |
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 |
|
@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. |
9 participants
Pull Request for Issue discovered in #38060 (comment).
Summary of Changes
Makes a proxy for the app variable in the
CMSPluginclass. It helps in the transition to the service providers where the application should be injected and not magically resolved by class reflection and static Factory access.Wondering if we should also handle the
dbvariable.Pink here @nikosdion for a review.
Testing Instructions
Create a demo sleep task. There is the app lookup done. Or revert the patch from #38132 and run the testing instructions, they should still work.
Actual result BEFORE applying this Pull Request
A deprecated warning is not logged, but the task works as before.
Expected result AFTER applying this Pull Request
A deprecated warning is logged, but the task works as before.