Change how JPluginHelper caches data; support Closures in JCacheControllerCallback

[mbabker]

Summary of Changes

Changes how JPluginHelper::load() caches data to use the callback caching controller instead of the generic base class and implements a cache ID based on the authorized access levels for the user.

Testing Instructions

With caching enabled, the proper plugins should be loaded for an allowed usergroup list on each request and this data cached.

[Fedik]

I have tested this item ✅ successfully on 8991164

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10795.}

[roland-d]

@mbabker Please quoteName the fields.

[joomla-cms-bot]

This PR has received new commits.

CC: @Fedik

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10795.}

[ggppdk]

I have tested this item ✅ successfully on 703d721

Tested various backend / frontend views and forms,
works as expected, only 1st call executes the query, all subsequent method calls use the cached data

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10795.}

[csthomas]

Changes is OK,
but IMHO would be better to generate query only if cache does not exists.

I mean to move generating query outside of the method, to ex:

protected static function loadQuery($levels)

and then run something like:

static::$plugins = $cache->get(array(static, 'loadQuery'), array($levels), md5($levels), false);

[mbabker]

It would have to be a public function since the callable is being called from the JCacheControllerCallback class. Unless we change it to use a lambda function I wouldn't suggest any other changes at all at this point as I wouldn't suggest opening the public API to expose that result set directly.

[csthomas]

Yes one public static method which return only result of rows.

Example:

static function loadQuery($levels)
{
        $db = JFactory::getDbo();
        $query = $db->getQuery(true)
                ->select(array($db->quoteName('folder', 'type'), $db->quoteName('element', 'name'), $db->quoteName('params
')))
                ->from('#__extensions')
                ->where('enabled = 1')
                ->where('type =' . $db->quote('plugin'))
                ->where('state IN (0,1)')
                ->where('access IN (' . $levels . ')')
                ->order('ordering');
        $db->setQuery($query);
        return $db->loadObjectList();
}

 /**
 * Loads the published plugins.
 *
 * @return  array  An array of published plugins
 *
 * @since   3.2
 */
protected static function load()
{
        if (static::$plugins !== null)
        {
                return static::$plugins;
        }

        $user = JFactory::getUser();

        $levels = implode(',', $user->getAuthorisedViewLevels());

        /** @var JCacheControllerCallback $cache */
        $cache = JFactory::getCache('com_plugins', 'callback');

        static::$plugins = $cache->get(array(__CLASS__, 'loadQuery'), array($levels), md5($levels), false);

        return static::$plugins;
}

[mbabker]

If I'm going to change it (which I don't believe should be part of this PR honestly), I'd rather make it a lambda function versus another public method. Otherwise the load method here should become public too. Making the method to execute the query public might as well make the entire loading mechanism public as well, whereas making it a lambda keeps it internal to the class and any derivatives.

[csthomas]

Lambda may be more appropriate but you can move that method 'loadQuery' to other/new class.

I understand that my idea breaks design pattern and it is bad when protected method run public.
I admit you are right.

[joomla-cms-bot]

This PR has received new commits.

CC: @Fedik, @ggppdk

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10795.}

[mbabker]

I converted it to a lambda function. In converting it, I found that JCacheControllerCallback would not support lambda functions; it does now. So there's your bonus feature for this patch.

[mbabker]

I could really use tests here. Getting the callback cache handler to support all callable types will help massively to be able to implement graceful fallbacks against cache related failures in some critical points in the system.

[roland-d]

@mbabker Tried to make a PR against this branch but it came at as 120 changed files, so gave up. Here is the code I wanted to push:

->from($db->quoteName('#__extensions'))
->where($db->quoteName('enabled') . ' = 1')
->where($db->quoteName('type') . ' = ' . $db->quote('plugin'))
->where($db->quoteName('state') . ' IN (0,1)')
->where($db->quoteName('access') . ' IN (' . $levels . ')')
->order($db->quoteName('ordering'));

As for the caching mechanism, this works as expected. Only once is the query executed when caching is enabled.

[roland-d]

I have tested this item ✅ successfully on 7b06aec

Caching mechanism, this works as expected. Only once is the query executed when caching is enabled.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10795.}

[mbabker]

There isn't a hard pressed need to quote everything in the database queries. If that's going to become part of the standard, that needs to be done separately from existing PRs IMO.

[ggppdk]

Only names that are reserved keywords need to be name-quoted right ?
using this in all cases becomes unnecessary more complex looking , thus more difficult to read, are we worried that new DB versions will add more "keywords" ? or playing safe for "all" DBs ?

[pritalpatel]

I have tested this item ✅ successfully on 7b06aec

I have Tested content and User backend views. It's work normally. 😄

Thanks

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10795.}

[mbabker]

Is this going anywhere or can I close for lack of interest?

[rdeutz]

@mbabker It is not going to anywhere it is going into 3.7.0 :-)

[wilsonge]

Merged with 3d744c0