? Pending

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
8 Jan 2017

Summary of Changes

  1. The code already has support for a cache_path parameter which allows a user to specify a custom cache path when using the filesystem for caching. This parameter is now exposed to the global configuration.

  2. When saving the global configuration, the code checking for the filesystem being usable did not account for this parameter and always used the hardcoded JPATH_SITE . '/cache' path for this check; the value is now used. In addition to this, we will also check if a custom cache path was given for this check and cannot be written to but the default path can be, we'll gracefully fall back to the default (and warn the user of this).

  3. Clearing the cache store when disabling the cache should not result in a fatal error saving the global configuration. Wrap the $cache->clean() call in try/catch blocks for the JCacheException* classes to handle these known errors (note this purposefully does not catch the parent RuntimeException as try/catch blocks should generally be specific to known error conditions).

  4. Since we clear the cache store when disabling the cache, we should also do it when changing the handler. That is implemented now.

Testing Instructions

Saving the global config should still work no matter what.

Setting a value for the cache_path var should result in that being the cache path used for the filesystem cache, when empty the code should not set the variable to the saved configuration. If this path can't be written to, the default path should be used instead and the cache_path value not be saved.

When changing cache handlers or disabling an active cache, the cache store should be cleaned. If the cache store can't be reached then a warning should be shown to the user and not block the save operation.

Documentation Changes Required

N/A

avatar mbabker mbabker - open - 8 Jan 2017
avatar mbabker mbabker - change - 8 Jan 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 8 Jan 2017
Category Administration com_config Language & Strings
avatar andrepereiradasilva
andrepereiradasilva - comment - 8 Jan 2017

I changed the cache path to a non existen folder and i got an exception: The file Cache Storage is not supported on this platform. . But my cache handler is file so is supported. The directory doesn't exist tough.

Also is the site cache folder or the admin cache folder?

avatar mbabker
mbabker - comment - 8 Jan 2017

I changed the cache path to a non existen folder and i got an exception: The file Cache Storage is not supported on this platform. . But my cache handler is file so is supported. The directory doesn't exist tough.

The isSupported check is is_writable(JFactory::getConfig()->get('cache_path', JPATH_CACHE)); so it's dependent on the configuration right now. Separate PR if you want to make changes to that logic (which probably should be done).

Also is the site cache folder or the admin cache folder?

Seems to be consistently inconsistent. The com_admin system info view uses it for both apps, the cache handler itself uses it for both apps, com_cache uses it for only the site app (same for elsewhere in com_config). Should be reviewed. Also note this is the only cache handler which has application awareness; every other handler doesn't distinguish based on app (IMO that's the right move, let's just have one consistent cache folder for all the things).

avatar andrepereiradasilva
andrepereiradasilva - comment - 8 Jan 2017

Seems to be consistently inconsistent.

Oh ... then all is fine! I has afraid this one time it would be consistently consistent. 😄

every other handler doesn't distinguish based on app (IMO that's the right move, let's just have one consistent cache folder for all the things).

Agree

avatar andrepereiradasilva
andrepereiradasilva - comment - 8 Jan 2017

I have tested this item ✅ successfully on 56ce7b8


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 8 Jan 2017 - Tested successfully
avatar dgt41
dgt41 - comment - 8 Jan 2017

I have tested this item ✅ successfully on 56ce7b8


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

avatar dgt41 dgt41 - change - 8 Jan 2017
Status Pending Ready to Commit
avatar dgt41 dgt41 - test_item - 8 Jan 2017 - Tested successfully
avatar dgt41
dgt41 - comment - 8 Jan 2017

RTC

avatar dgt41 dgt41 - change - 8 Jan 2017
Status Ready to Commit Pending
avatar dgt41 dgt41 - change - 8 Jan 2017
Status Pending Ready to Commit
avatar dgt41
dgt41 - comment - 8 Jan 2017

@joomla-bot are you drunk?

avatar wilsonge wilsonge - change - 9 Jan 2017
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-01-09 15:24:54
Closed_By wilsonge
Labels Added: ?
avatar wilsonge wilsonge - close - 9 Jan 2017
avatar wilsonge wilsonge - merge - 9 Jan 2017
avatar infograf768
infograf768 - comment - 16 Jan 2017

@mbabker
Notice:
#13608

avatar infograf768
infograf768 - comment - 16 Jan 2017

Maybe change to

               if (!empty($data['cache_path']))
		{
			$path = $data['cache_path'];
		}
		elseif (empty($data['cache_path']) && !empty($prev['cache_path']))
		{
			$path = $prev['cache_path'];
		}
		else
		{
			$path = JPATH_SITE . '/cache';
		}
avatar mbabker
mbabker - comment - 16 Jan 2017

Feel free to PR it.

avatar infograf768
infograf768 - comment - 16 Jan 2017

Done.
#13612

Add a Comment

Login with GitHub to post a comment