? Pending

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000
okonomiyaki3000
7 Aug 2017

Pull Request for Issue # .

Summary of Changes

When attempting to load a CacheStorage handler, if UnsupportedCacheException is thrown, catch it and use the base class as the storage handler (which does nothing) instead of just dying. Write a log so there's some record of the intended cache not working.

There's a case to be made that you should fail whenever possible so that problems can be fixed. That's a good policy in a lot of cases. Not this case. Suppose you are using some code which is not yours and which attempts to use a specific cache handler. It assumes that handler is available on your system but this is not a safe assumption. If it can't load it, the code will fail hopelessly. This change makes it so that you will fall back on simply not caching when something like that happens.

Testing Instructions

Turn on caching, use 'file' as your storage type, then set your cache directory something unwritable.

Expected result

I think it's reasonable to expect that life will go on without caching and you will get a log that warns you that caching is not happening as you intended.

Actual result

Joomla will throw an exception and die because file storage is not supported when the cache directory is not writable.

Documentation Changes Required

no

avatar joomla-cms-bot joomla-cms-bot - change - 7 Aug 2017
Category Libraries
avatar okonomiyaki3000 okonomiyaki3000 - open - 7 Aug 2017
avatar okonomiyaki3000 okonomiyaki3000 - change - 7 Aug 2017
Status New Pending
avatar mbabker
mbabker - comment - 7 Aug 2017

This feels more like a hack than a fix. As is, it would actually be impossible to debug issues with the cache storage because you will arbitrarily catch them all and alter the operation in a way that can only be caught if you have the logging plugin turned on.

The cache API is not the place to be implementing high level error handling behaviors like this. It is correctly designed to throw errors in a way that consumers of the API can (and should if developers would properly catch and handle errors) catch them and decide locally if it is a critical failure or one that can be gracefully worked around (all throughout "critical" infrastructure pieces we are doing just that in core).

avatar okonomiyaki3000
okonomiyaki3000 - comment - 7 Aug 2017

Obviously I disagree or I wouldn't have done this. It is not impossible to debug because you should be logging while you're debugging and you should probably be logging warning and above at all times anyway.

However, maybe there is some case where you'd rather not fall back. Since an $options object is passed in, maybe it could optionally contain an array of alternate handler names that could be tried before finally deciding to fail. The default case would still be to throw the exception.

avatar mbabker
mbabker - comment - 7 Aug 2017

This might be a valid "keep things working" fix for a production environment, but in a development environment, this is not the type of error that you want to be blindly caught and ignored. The best fix for this issue is for developers to implement proper error handling mechanisms in their extensions. I think we've updated almost every use of JCache in core so that it implements said error handling, which covers all of the base use cases except for a developer who explicitly calls into the caching API on their own.

It is not impossible to debug because you should be logging while you're debugging and you should probably be logging warning and above at all times anyway.

Let's be honest here. How many people are running the logging plugin in production or have written their own logging mechanism to catch these types of issues? For that matter, where in our documentation is it written to guide users on how to most optimally configure Joomla logging? With the current code and error handling mechanisms, you will get a notification in some form of there being platform issues (filesystem permissions, Memcached down) that you'd want to investigate ASAP but with this change odds are you're going to miss that notification.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 8 Aug 2017

I do basically agree that the developer should handle the error. My problem was that there was (seems to be fixed now) code shipping with Joomla that caused this error and did not catch it. As long as that's fine, I'm satisfied, actually.

avatar okonomiyaki3000 okonomiyaki3000 - change - 8 Aug 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-08-08 00:03:15
Closed_By okonomiyaki3000
Labels Added: ?
avatar okonomiyaki3000 okonomiyaki3000 - close - 8 Aug 2017

Add a Comment

Login with GitHub to post a comment