Joomla\CMS\Installer\InstallerScript::getItemArray() does not return unique results

[mbabker]

It looks like this might have been exposed by #22090 as things were working OK in 3.8 and earlier, but that PR in and of itself isn't actually the problem here yet somehow applying it exposes the problem.

Extensions have to have a unique element name based on the extension type (and in the case of plugins, the folder; in the case of modules and templates I think this is also based on the client ID). So there are actually multiple records in that table where the element column is named "joomla" and this is perfectly acceptable.

If you have an extension script that extends Joomla\CMS\Installer\InstallerScript, its preflight method will try to validate that you aren't installing an older version if instructed to do so. To do that check, Joomla\CMS\Installer\InstallerScript::getItemArray() is called with a set of params that basically results in this query:

SELECT `manifest_cache` FROM `#__extensions` WHERE `element` = '$extension';

So going back to that example with extensions where the element column has the value of "joomla", this creates a non-unique result set. How did I run into this problem you ask? The joomla.org template is named "joomla" and trying to test installing an update I got a "Downgrading from version 13.1 to version 3.0.1-dev is not allowed." error message; except the template version isn't 13.1, that is the version of the Joomla library extension.

[Fedik]

it looks like getItemArray limited to single where clause by $identifier (that fine if select by ID),
possible solution is to allow $column and $identifier to be array, but this would be break of b/c, right?

[mbabker]

possible solution is to allow $column and $identifier to be array, but this would be break of b/c, right?

For the most B/C behavior you'd have to introduce a new method because there are too many what ifs and we all know how people groan if you change anything (in theory since not typehinted you can change $column and $identifier to support both single scalar arguments and an array, but you'd need to type check the arguments and throw an Exception if the types don't match (both should be an array or both should be single scalar item (int, string, bool, etc.) or the number of array elements doesn't match; then there's the theoretical "adding new Exception is B/C break" but the method already doesn't catch exceptions from the database so adding a new RuntimeException in theory is OK since it matches what most would be catching if not handling the database specific exceptions only, which are newer than the installer script class in the first place; then there's the theoretical "but I've extended this method in a subclass" argument; then there's the even more painful "adding a new method to a class is a potential B/C break because I use this name in my subclass" argument).

[wilsonge]

I just came across this from elsewhere.

I originally based this on Nic's FOF installer script with a few common other things I'd seen around mixed in - so the original intent of this was just to be for components. Then I think that getItemArray i threw in from some module code I built a while ago. Just as some background for this. So definitely just an oversight and should be fixed in some way!

[SharkyKZ]

Re-opening as this hasn't been solved. And the core preflight method is broken for the same reason.

[SharkyKZ]

We need to make extension type, client ID and plugin folder available in the script. What would be the best way to do this?

1. Add these properties to the script and have developers set them manually in their update scripts?
2. Add these properties and getter methods in base InstallerAdapter?
3. Add type getter to base adapter and the rest only to adapters that use them? Wrap in type and/or method_exists checks?
4. Some other way?

[Hackwar]

This is the declaration of the method in question:

    /**
     * Builds a standard select query to produce better DRY code in this script.
     * This should produce a single unique cell which is json encoded - it will then
     * return an associated array with this data in.
     *
     * @param   string  $element     The element to get from the query
     * @param   string  $table       The table to search for the data in
     * @param   string  $column      The column of the database to search from
     * @param   mixed   $identifier  The integer id or the string
     *
     * @return  array  Associated array containing data from the cell
     *
     * @since   3.6
     */
    public function getItemArray($element, $table, $column, $identifier)

$identifier already can have multiple different values, so in order to be able to fix this in 4.x, why not extend the permissible values to an associative array? So when $identifier is an array and $column is null, we could then check for several columns. In Joomla\CMS\Installer\InstallerScript::preflight() we would have to then handle each extension type specifically. Yes, that is nasty, but it would allow us to fix this issue right now. What do you guys think? @HLeithner, @wilsonge, @zero-24 ?

[Hackwar]

@laoneo ?

[laoneo]

But then I would rather create a new function which does it right and deprecate the faulty one.

[zero-24]

I do agree with @laoneo