Expose cache_path config, catch exceptions cleaning cache on config save #13520

Merged
merged 1 commit into from Jan 9, 2017

Conversation

Projects
None yet
6 participants
@mbabker
Member

mbabker commented Jan 8, 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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Jan 8, 2017

Contributor

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?

Contributor

andrepereiradasilva commented Jan 8, 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?

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jan 8, 2017

Member

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).

Member

mbabker commented Jan 8, 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).

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Jan 8, 2017

Contributor

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

Contributor

andrepereiradasilva commented Jan 8, 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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Jan 8, 2017

Contributor

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.

Contributor

andrepereiradasilva commented Jan 8, 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.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jan 8, 2017

Member

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.

Member

dgrammatiko commented Jan 8, 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.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
Member

dgrammatiko commented Jan 8, 2017

RTC

@joomla-cms-bot joomla-cms-bot added RTC and removed RTC labels Jan 8, 2017

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jan 8, 2017

Member

@joomla-bot are you drunk?

Member

dgrammatiko commented Jan 8, 2017

@joomla-bot are you drunk?

@joomla-cms-bot joomla-cms-bot added the RTC label Jan 8, 2017

@wilsonge wilsonge merged commit 1481339 into joomla:staging Jan 9, 2017

3 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilsonge wilsonge added the New Feature label Jan 9, 2017

@wilsonge wilsonge added this to the Joomla 3.7.0 milestone Jan 9, 2017

@mbabker mbabker deleted the mbabker:config-cache-fixes branch Jan 9, 2017

@infograf768

This comment has been minimized.

Show comment
Hide comment
Member

infograf768 commented Jan 16, 2017

@mbabker
Notice:
#13608

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jan 16, 2017

Member

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';
		}
Member

infograf768 commented Jan 16, 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';
		}
@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jan 16, 2017

Member

Feel free to PR it.

Member

mbabker commented Jan 16, 2017

Feel free to PR it.

@infograf768

This comment has been minimized.

Show comment
Hide comment
Member

infograf768 commented Jan 16, 2017

Done.
#13612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment