? Pending

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
12 Feb 2017

Summary of Changes

This PR makes several small tweaks to the JLibraryHelper class, including:

  1. Deprecating JLibraryHelper::_load() in favor of JLibraryHelper::load() for code style reasons (no underscore methods)
  2. Changing JLibraryHelper::load() to issue only a single database query for all libraries versus a query per library, similar to the other helper loaders
  3. Making JLibraryHelper::load() resilient to a cache engine failure and no longer catching a database failure

Testing Instructions

Interaction with the library helper still works as normal. This PR mainly requires ensuring the Joomla library parameters still load OK (if you have a query string on your media URLs and don't get a "error loading library" message, you're good.

Documentation Changes Required

Document the deprecation.

avatar mbabker mbabker - open - 12 Feb 2017
avatar mbabker mbabker - change - 12 Feb 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Feb 2017
Category Libraries
avatar csthomas
csthomas - comment - 16 Feb 2017

It's the same thing the component helper is doing, see #6031 for more.

The reason is a collision in cache (id) between JLibraryHelper::_load() and JComponentHelper::load(). Take a look at

libraries/cms/component/helper.php-433-                 $query = $db->getQuery(true)
libraries/cms/component/helper.php-434-                         ->select($db->quoteName(array('extension_id', 'element', 'params', 'enabled'), array('id', 'option', null, null)))
libraries/cms/component/helper.php-435-                         ->from($db->quoteName('#__extensions'))
libraries/cms/component/helper.php-436-                         ->where($db->quoteName('type') . ' = ' . $db->quote('component'));
libraries/cms/component/helper.php-437-                 $db->setQuery($query);
libraries/cms/component/helper.php-438-
libraries/cms/component/helper.php-439-                 return $db->loadObjectList('option');
libraries/cms/component/helper.php-440-         };
libraries/cms/component/helper.php-441-
libraries/cms/component/helper.php-442-         /** @var JCacheControllerCallback $cache */
libraries/cms/component/helper.php:443:         $cache = JFactory::getCache('_system', 'callback');
libraries/cms/component/helper.php-444-
libraries/cms/component/helper.php-445-         try
libraries/cms/component/helper.php-446-         {
libraries/cms/component/helper.php-447-                 $components = $cache->get($loader, array(), $option, false);
libraries/cms/component/helper.php-448-
libraries/cms/component/helper.php-449-                 /**
--
libraries/cms/library/helper.php-138-   protected static function _load($element)
libraries/cms/library/helper.php-139-   {
libraries/cms/library/helper.php-140-           $db = JFactory::getDbo();
libraries/cms/library/helper.php-141-           $query = $db->getQuery(true)
libraries/cms/library/helper.php-142-                   ->select($db->quoteName(array('extension_id', 'element', 'params', 'enabled'), array('id', 'option', null, null)))
libraries/cms/library/helper.php-143-                   ->from($db->quoteName('#__extensions'))
libraries/cms/library/helper.php-144-                   ->where($db->quoteName('type') . ' = ' . $db->quote('library'))
libraries/cms/library/helper.php-145-                   ->where($db->quoteName('element') . ' = ' . $db->quote($element));
libraries/cms/library/helper.php-146-           $db->setQuery($query);
libraries/cms/library/helper.php-147-
libraries/cms/library/helper.php:148:           $cache = JFactory::getCache('_system', 'callback');
libraries/cms/library/helper.php-149-
libraries/cms/library/helper.php-150-           try
libraries/cms/library/helper.php-151-           {
libraries/cms/library/helper.php-152-                   static::$libraries[$element] = $cache->get(array($db, 'loadObject'), null, $element, false);
libraries/cms/library/helper.php-153-           }
libraries/cms/library/helper.php-154-           catch (RuntimeException $e)

Joomla uses the same cache_id for two different places and values.

So the solution could be to make that functions compatibility or change cache_id for one of them.

avatar mbabker
mbabker - comment - 16 Feb 2017

With 3.7 the ID thing shouldn't be an issue unless the two closures have the same object hash.

Either way I'd rather just do the B/C break and not have a filter (same as the plugin helper) and be done with it.

avatar csthomas
csthomas - comment - 16 Feb 2017

Take a look again:

$components = $cache->get($loader, array(), $option, false);

you put explicitly ID=$option, you do not use spl_object_hash.

avatar mbabker
mbabker - comment - 16 Feb 2017

Grr...

avatar mbabker
mbabker - comment - 16 Feb 2017

I hate these classes and inconsistencies with a passion. This is why it's so painful to do things right.

avatar csthomas
csthomas - comment - 16 Feb 2017

Maybe JLibraryHelper::load() should call JControllerHelper::load() then cache will be only on one side.

avatar mbabker
mbabker - comment - 16 Feb 2017

The library helper's querying library extensions, component helper for components. So it wouldn't help any.

avatar mbabker
mbabker - comment - 17 Feb 2017

OK, thinking this through, here's my plan of action for both helpers:

  1. Deprecate the load method's parameter, other methods in the class can read the static array directly and fetch the right record using its key.
  2. The lambda functions will execute one query for all records of the extension type, this will remove the need to pass the param into the lambda for the query's WHERE filter.
  3. In 3.7 the load method still returns the single item, in 4.0 load will have a null return and throw Exceptions on critical failures (basically let the database exceptions bubble up while handling cache exceptions as is being done already).

I'll make relevant changes for this PR (the library helper) tomorrow and open a separate PR for the component helper shortly after.

avatar csthomas
csthomas - comment - 18 Feb 2017

What about only change cacheId in you current changes.

static::$libraries = $cache->get($loader, array(), $element, false);

replace to:

// It loads all libraries, so cacheId should be more general/static like `JLibraryHelperLoad`.
static::$libraries = $cache->get($loader, array(), 'JLibraryHelperLoad', false);

from now there won't be any collision.

avatar mbabker
mbabker - comment - 18 Feb 2017

When I'm done it'll be a randomly generated ID by the callback controller, not a known value.

avatar mbabker mbabker - change - 18 Feb 2017
Labels Added: ?
avatar mbabker
mbabker - comment - 18 Feb 2017

This is rebased and ready for new tests.

avatar csthomas
csthomas - comment - 18 Feb 2017

I don't understand the closure and spl_object_hash in joomla cache controller.
I did a simple test below and closure and spl_object_hash() does not work in the same way as serialize().

$ php -a
php > echo spl_object_hash(function ($a) { return 'a';});
000000004e2e564200000000208b55a8
php > echo spl_object_hash(function ($a) { return 'a';});
000000004e2e564200000000208b55a8
php > echo spl_object_hash(function ($a) { return 'a';});
000000004e2e564200000000208b55a8

# Close process and open the next one

$ php -a
php > echo spl_object_hash(function ($a) { return 'a';});
0000000043000aba0000000052d343e2
php > echo spl_object_hash(function ($a) { return 'a';});
0000000043000aba0000000052d343e2

IMO spl_object_hash generates unique ID per process.
If apache run more that one php process then we have a lots of duplications.
I have not tested it yet.

avatar mbabker
mbabker - comment - 18 Feb 2017

That needs to be addressed in the callback controller then. The fix isn't for every use of the callback controller with Closures to manually specify an ID.

avatar mbabker
mbabker - comment - 18 Feb 2017

Short term, let's just get this in. I've set an ID based on the method name, that'll be good enough for now.

avatar joomdonation
joomdonation - comment - 19 Feb 2017

I haven't used this library myself, so don't really know what need to be tested. If you could provide more clear testing instructions (for example, running what code and check the result...), I will be happy to test it

avatar mbabker
mbabker - comment - 19 Feb 2017

It loads a version hash to be used as a query string for assets by default. As long as that query string gets loaded in (and you can validate the cache behavior works resulting in the query not being run when data is stored to cache) you're good.

avatar csthomas
csthomas - comment - 20 Feb 2017

Libraries which has been got from cache does not have correct params class.

My test:

$ php -a
Interactive mode enabled
php > require 'console.php';
php > print_r($a = JLibraryHelper::getLibrary('phpass', true));                                                                     
stdClass Object                                                                                                                     
(                                                                                                                                   
    [id] => 106                                                                                                                     
    [option] => phpass                                                                                                              
    [params] => Joomla\Registry\Registry Object                                                                                     
        (                                                                                                                           
            [data:protected] => stdClass Object                                                                                     
                (                                                                                                                   
                )                                                                                                                   
                                                                                                                                    
            [initialized:protected] =>                                                                                              
            [separator] => .
        )

    [enabled] => 1
)
php > print_r($a = JLibraryHelper::getLibrary('fof', true));      
stdClass Object
(
    [id] => 105
    [option] => fof
    [params] => Joomla\Registry\Registry Object
        (
            [data:protected] => stdClass Object
                (
                )

            [initialized:protected] => 
            [separator] => .
        )

    [enabled] => 1
)
php > print_r($a = JLibraryHelper::getLibrary('phputf8', true));   
stdClass Object
(
    [id] => 102
    [option] => phputf8
    [params] => Joomla\Registry\Registry Object
        (
            [data:protected] => stdClass Object
                (
                )

            [initialized:protected] => 
            [separator] => .
        )

    [enabled] => 1
)
php > print_r($a = JLibraryHelper::getLibrary('phpass', true));    
stdClass Object
(
    [id] => 106
    [option] => phpass
    [params] => Joomla\Registry\Registry Object
        (
            [data:protected] => stdClass Object
                (
                )

            [initialized:protected] => 
            [separator] => .
        )

    [enabled] => 1
)

php > ^D 
$ curl https://patch-diff.githubusercontent.com/raw/joomla/joomla-cms/pull/14046.diff | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2844    0  2844    0     0   3089      0 --:--:-- --:--:-- --:--:--  3087

$ php -a
Interactive mode enabled

php > require 'console.php';
php > print_r($a = JLibraryHelper::getLibrary('phpass', true));
stdClass Object
(
    [id] => 106
    [option] => phpass
    [params] => Joomla\Registry\Registry Object
        (
            [data:protected] => stdClass Object
                (
                )

            [initialized:protected] => 
            [separator] => .
        )

    [enabled] => 1
)
php > print_r($a = JLibraryHelper::getLibrary('phpass', true));
stdClass Object
(
    [id] => 106
    [option] => phpass
    [params] => Joomla\Registry\Registry Object
        (
            [data:protected] => stdClass Object
                (
                )

            [initialized:protected] => 
            [separator] => .
        )

    [enabled] => 1
)
php > print_r($a = JLibraryHelper::getLibrary('phputf8', true));
stdClass Object
(
    [id] => 102
    [option] => phputf8
    [params] => 
    [enabled] => 1
)
php > print_r($a = JLibraryHelper::getLibrary('fof', true));
stdClass Object
(
    [id] => 105
    [option] => fof
    [params] => 
    [enabled] => 1
)

Please move/copy the code:

if ...
{
    static::$libraries[$element]->params = new Registry(static::$libraries[$element]->params);
}

to getLibrary() method.

avatar mbabker
mbabker - comment - 20 Feb 2017

Moved. This is becoming one painful PR.

avatar csthomas csthomas - test_item - 21 Feb 2017 - Tested successfully
avatar csthomas
csthomas - comment - 21 Feb 2017

I have tested this item successfully on 282c513


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14046.

avatar joomdonation joomdonation - test_item - 21 Feb 2017 - Tested successfully
avatar joomdonation
joomdonation - comment - 21 Feb 2017

I have tested this item successfully on 282c513

Works as expected:

  1. Version hash is available in query string for assets

  2. Cache works as expected (query not being run when data is available in cache)

Just a minor thing. I tried to search through the code of Joomla installation and see that only joomla library is used this class. Before, only that library is loaded into memory, now all libraries is loaded, so this change cost abit more memory compare to the old one


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14046.

avatar csthomas
csthomas - comment - 21 Feb 2017

The same way is used in component helper but usual we use only one component per request.
IMO we should not load all records but only one record which can be fast retrieved from db.
Joomla has only a few libraries, but components can be in total 30+ (frontend and backend).

Compare:

  • Load a few (1,2) queries from db "SELECT * FROM table WHERE indexed_column = 1" vs
  • Get data from cache and unserialize list of items. For 30+ records it may be slower than above.

I have found that joomla use all components also in other places like assets or url route so we can not load only one.

avatar mbabker
mbabker - comment - 21 Feb 2017

True there is only one active component per request, but the parameters of several components are read during the request.

avatar mbabker mbabker - change - 21 Feb 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-02-21 15:40:34
Closed_By mbabker
avatar mbabker mbabker - close - 21 Feb 2017
avatar mbabker
mbabker - comment - 21 Feb 2017

I'm done here then. Because ultimately we have now come full circle and that is exactly what the current behavior is.

avatar csthomas
csthomas - comment - 21 Feb 2017

Not quite, old behaviour has a conflict because of cache_id = $option, METHOD as a cache_id is better. I would create a new PR if you do not want to back to this.

avatar mbabker
mbabker - comment - 21 Feb 2017

If you're making the cache ID only __METHOD__ then that won't work. You would HAVE to make it a concat of __METHOD__ . $element at a minimum (talking about the library helper).

Either way I'm done getting annoyed with the inconsistencies of these extension helpers.

Add a Comment

Login with GitHub to post a comment