joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.79k stars 3.66k forks source link

Joomla mistakenly reuses the installer script file #41087

Open nikosdion opened 1 year ago

nikosdion commented 1 year ago

Steps to reproduce the issue

Expected result

The script file runs once, for Extension A.

Actual result

The script file runs twice, for extensions A and B. Even though it's Extension A's file and has nothing to do with Extension B.

Bonus fun: if the extensions are of a different type (e.g. one Package and one Plugin) and the script file has its parameters type-hinted you get a lovely error message:

0 Pkg_blahgblahInstallerScript::postflight(): Argument #2 ($parent) must be of type Joomla\CMS\Installer\Adapter\PackageAdapter, Joomla\CMS\Installer\Adapter\PluginAdapter given

If you uninstall the extensions separately, however, no error occurs. Which proves that the problem is with Joomla!. And that's how I got dragged into debugging this, kicking and screaming…

System information (as much as possible)

Joomla 4.3 latest dev

Additional comments

BTW, this also applies to extension updates. We had observed that a few months ago on several client sites. The third person on the same day to report it told us he was indeed updating multiple extensions at once, and updating each extension separately "fixed" the issue for him. Then we never heard about it again and I had forgotten about it. Until today. But I digress.

The problem is in \Joomla\CMS\Installer\InstallerAdapter::setupScriptfile(), namely here:

        // When no script file, do nothing
        if (!$manifestScript) {
            return;
        }

A quick fix for that is:

        if (!$manifestScript) {
            $this->parent->manifestClass = null;

            return;
        }

The obvious reason is that the Installer instance ($this->parent) is reused whenever you loop through extensions (update, uninstallation). Therefore, $this->parent->manifestclass may be set and non-empty.

A better solution would be NOT reusing Joomla\CMS\Installer\Installer and Joomla\CMS\Updater\Updater objects. I do not understand why they are Singletons. The Singleton pattern would make sense if loadAllAdapters() was called —that's the only method with a performance impact which would benefit from a Singleton— but this is done literally nowhere in the CMS.

Therefore I would propose the band-aid patch for Joomla 4.x series with deprecation of getInstance() in the aforementioned objects. Joomla 4.4 / 5.0 should stop using getInstance for these objects, instead creating them afresh (just like the PackageAdapter does, presumably because of similar issues). Probably something for @laoneo and @HLeithner to think about.

BTW, this could very easily explain the issue I have reported under “Files are not copied on update” in https://github.com/joomla/joomla-cms/issues/32592 and which @SniperSister has observed with non-Akeeba extensions. The now obvious explanation is that an installer script loaded earlier could return false from preflight / update, or otherwise cock up the files of an extension it was not meant to handle (because the developer very reasonably tests their own script with their own extension, not any random extension under the sun!). It also explains why adding installer scripts seems to alleviate the problem, a workaround I have applied a while ago to my Joomla 4 extensions by sheer dumb luck (explaining why I stopped seeing this issue on Joomla 4 without the installer code having changed in any significant way).

laoneo commented 1 year ago

I would welcome the deprecation of getInstance. Deprecating it in 4.4 sounds like a good plan, if it can be changed also in 4.4 then even better, otherwise it needs to go into 5.0.

nikosdion commented 1 year ago

Awesome! I have spotted just two files where it's used and it's buried deep, no chance of upsetting 3PD code. You can definitely deprecate starting with 4.4 with a removal target of 6.0. I'll do a PR over the weekend when I have some time.

laoneo commented 1 year ago

I'm more or less offline till mid August so I can't help here much. But @MacJoom will take care of our pr's.