User tests: Successful: Unsuccessful:
This PR makes several small tweaks to the JLibraryHelper
class, including:
JLibraryHelper::_load()
in favor of JLibraryHelper::load()
for code style reasons (no underscore methods)JLibraryHelper::load()
to issue only a single database query for all libraries versus a query per library, similar to the other helper loadersJLibraryHelper::load()
resilient to a cache engine failure and no longer catching a database failureInteraction 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.
Document the deprecation.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
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.
Take a look again:
$components = $cache->get($loader, array(), $option, false);
you put explicitly ID=$option, you do not use spl_object_hash
.
Grr...
I hate these classes and inconsistencies with a passion. This is why it's so painful to do things right.
Maybe JLibraryHelper::load() should call JControllerHelper::load() then cache will be only on one side.
The library helper's querying library extensions, component helper for components. So it wouldn't help any.
OK, thinking this through, here's my plan of action for both helpers:
I'll make relevant changes for this PR (the library helper) tomorrow and open a separate PR for the component helper shortly after.
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.
When I'm done it'll be a randomly generated ID by the callback controller, not a known value.
Labels |
Added:
?
|
This is rebased and ready for new tests.
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.
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.
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.
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
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.
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.
Moved. This is becoming one painful PR.
I have tested this item
I have tested this item
Works as expected:
Version hash is available in query string for assets
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
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:
"SELECT * FROM table WHERE indexed_column = 1"
vsunserialize
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.
True there is only one active component per request, but the parameters of several components are read during the request.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-02-21 15:40:34 |
Closed_By | ⇒ | mbabker |
I'm done here then. Because ultimately we have now come full circle and that is exactly what the current behavior is.
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.
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.
The reason is a collision in cache (id) between
JLibraryHelper::_load()
andJComponentHelper::load()
. Take a look atJoomla 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.