-1 for exactly the reasons you've said here. I don't remember if the PR
was merged for 3.6.1 or not but I did some work to keep the cache
controllers and JCache from instantiating the storage adapters needlessly.
And honestly, if we're going to make this change on this one line, then the
entire factory method should always return null objects. And I don't think
it's a good idea for us to be adding more code that absorbs errors, we've
already got enough monstrosities like JApplicationCms::getMenu().

The better fix is to fix the code in front of the factory, fixing the real
issue, not the side effect. JCache and the controllers shouldn't be
reaching a point where any of those classes load a storage adapter if
caching is disabled.

On Tuesday, August 2, 2016, Elijah Madden notifications@github.com wrote:

Summary of Changes

Instead of throwing an exception when a requested cache handler is not
available, get an instance of the base JCacheStorage class (which does
not store anything) instead. Log a warning but do not die.

I suppose this is controversial change. I recognize and agree that it's
best to die anytime something goes wrong so that problems can be found and
fixed asap. However, I think this is a bad place to strictly follow that
philosophy. The reason is that Joomla tries to create cache handlers even
when caching is turned off. It tries to create them with whatever handler
they'd have used had caching been turned on. So your config may have cache
set to 0 and cache_handler set to file and Joomla will attempt to create
an instance of JCacheStorageFile. This might fail even though you had no
intention of using caching at all. To me, that's just silly.
Testing Instructions

1. Turn on caching and set it to use file
2. Remove or rename your cache folder
3. Notice that Joomla dies
4. Try to run cli/finder_indexer.php from the command line, notice that it dies even though it does not use caching
5. Apply the patch
6. Notice that these things do not die

This all seems terribly unlikely in real life, right? Take this case. You
have a site which uses apc instead of file caching. You don't have a cache
folder because you don't need one and you strictly limit write permissions
for security reasons etc etc. You try to run the finder indexer which
doesn't need to cache anything anyway. It dies because it turns off caching

but sets the handler to file.

You can view, comment on, or merge this pull request online at:

#11405
Commit Summary

• There is no need to die here. Log and carry on.

File Changes

• M libraries/joomla/cache/storage.php https://github.com/joomla/joomla-cms/pull/11405/files#diff-0 (4)

Patch Links:

• https://github.com/joomla/joomla-cms/pull/11405.patch
• https://github.com/joomla/joomla-cms/pull/11405.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11405, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoUxvFRUnHHfaap16bKuIOACSoJGmks5qcAaPgaJpZM4JbQkH
.

#11121

Guess it wasn't.

I thought about that but the problem is that cache may be disabled and enabled on the fly. So you can't really just look at the cache config variable and think that, just because it's 0, we will never need to cache anything. So we first need to address whether we want to keep the functionality of switching it on and off.

That's why my approach was to check whether caching is enabled based on the
current JCache instance, not the global config. So even if global caching
is disabled and you call
JFactory::getCache()->setCaching(true)->get('cached') it should result in
the cache API correctly trying to connect to the storage layer to read
data. If it's disabled when you call get(), we can skip trying to hit the
storage layer and return quickly to prevent the connection attempt and
erroring out in a scenario that you shouldn't reach an error at all.

On Tuesday, August 2, 2016, Elijah Madden notifications@github.com wrote:

I thought about that but the problem is that cache may be disabled and
enabled on the fly. So you can't really just look at the cache config
variable and think that, just because it's 0, we will never need to cache
anything. So we first need to address whether we want to keep the
functionality of switching it on and off.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11405 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoTKLK8U7zl35c0vzz7jAMpotuWdQks5qcBOqgaJpZM4JbQkH
.

Sounds pretty good.

@mbabker chase @wilsonge about merging #11121

Shall I close this PR then?

On 3 August 2016 at 05:14, Elijah Madden notifications@github.com wrote:

Sounds pretty good.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11405 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8aSF9JJq9FuPUoAScOvi64dgtdy1ks5qcBWbgaJpZM4JbQkH
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

In favor of #11121