@Hackwar as far i can tell we can't use $this->app without this test:

        // Get the application if not done by JPlugin. This may happen during upgrades from Joomla 2.5.
        if (!$this->app)
        {
            $this->app = JFactory::getApplication();
        }

see e.g. here: #3553 (comment)

It should work here. From memory it only creates issues if you use it in the onAfterInitialise event. But I may be wrong of course.

The application object is added in the constructor. If that code is not part of JPlugin, it wont work, neither in onAfterInitialise nor in onAfterRender.
Before we add such code, I'd rather close this PR again. The goal is to reduce the number of LoCs and not increase it. Either this feature works, then we don't need that check, or it does not work, then we should remove it again from the whole system.Simple as that.

oh sorry @Hackwar I don't want to attack you. I only what to notice it as we had this problem on upgrading with the rememberme plugin.

The application object is added in the constructor. If that code is not part of JPlugin, it wont work, neither in onAfterInitialise nor in onAfterRender.

The app property is autoloading in the JPlugin constuctor:
https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/plugin/plugin.php#L97-106

So the issue could be if the event run at the upgrading time / bevor the file gets replaced.

@Hackwar The issue comes from the fact that during an upgrade the files are not replaced in a defined order. So plugins may already be the new version when JPlugin was still the old one.
On some servers it works, and some it doesn't. Depending on the order in which the files get replaced.

So you and @zero-24 are right. It may well be that it affects all events indeed. I just checked and it looks like we use $this->app only in a few plugins, and some obviously will not get triggered during an upgrade. That's why it mainly affected the remember me plugin.

For now, we need that workaround code to be present.

And in that case I would rather close this PR than merge it. As I said before, my goal is to clean up the code, not riddle it even further with this stupid stuff.

The other changes in that PR aren't problematic. We could still keep those if you don't want to add the fallback check.

For now, we need that workaround code to be present.

We really should be debugging the root issues and not adding hacks to our own code which implement our own features to make sure those features work...

We really should be debugging the root issues and not adding hacks to our own code which implement our own features to make sure those features work...

I tried to debug it. But without having a way to reproduce it, it's quite hard
And the fallback code seems to have worked fine.

We're better off just manually setting $this->app in the constructor than checking if it exists after it is supposed to be created. Does the fallback work? Yes. Does it fix the underlying issue? No. Is anyone trying to fix the underlying issue? Probably not, in part for the reason you gave (difficult to reproduce). So instead of finding a good solution for a deeper issue, we just implement hacks around our own feature set. THAT is a problem IMO and a path we shouldn't get comfortable traveling.

No argument from me. It's a hackish fallback for sure.
The only reason I mentioned it here is so updates don't get broken again due to this bug. I don't have a real solution and likely will not find one.

Then deprecate the feature in JPlugin and manually set the objects in each plugin's constructor. Seems not many of us are going to dig into the deeper issues to figure out how to fix them for one reason or another (I'll concede that finding a fix for those problems is probably going to be rather time intensive, and I for one don't have that kind of time right now given the other projects I'm knee deep in), so we should discontinue support for a feature which has known issues in those circumstances.