Skip to content

Commit

Permalink
MDL-37500 cache: integration review touchups.
Browse files Browse the repository at this point in the history
Issues:
1. Fix setType calls
2. Changing from default to all and editing again, default is still selected.
3. Uncheck all options causes error.
4. Doesn't seem to be restricting based upon option.
5. Picked all and got coding error with the database thing.
6. Bad title: samhemelryk@wip-MDL-37500-m25#L1R157
7. Amend comments - should only be removed once 2.5 is the minimum version for an upgrade.
8. Document the defaultsharing option.

Outcomes:
1. Fixed - copy paste error.
2. Fixed - mforms was applying the default value despite a value being provided. A quirk of elements with array names.
3. Fixed - validation now requires at least one option to be selected.
4. Fixed - issue rose from definitions not being re-parsed. cache/admin.php now reparses the first time a user visits the page.
5. Fixed - better purging of definitions when working with them anonymously. Unit test added.
6. Fixed - new string added and used.
7. Fixed - comments amended.

New issue to address parsing of definitions during upgrade.
New issue to add debugging notice if definition sets only one possible sharing option and that option is user input.
  • Loading branch information
Sam Hemelryk committed May 7, 2013
1 parent 46e17f0 commit 5f5776c
Show file tree
Hide file tree
Showing 8 changed files with 110 additions and 14 deletions.
2 changes: 2 additions & 0 deletions cache/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ A definition:
'contextmarkeddirty'
),
'sharingoptions' => null // Optional
'defaultsharing' => null // Optional
)
);

Expand Down Expand Up @@ -149,6 +150,7 @@ The following optional settings can also be defined:
* mappingsonly - This definition can only be used if there is a store mapping for it. More on this later.
* invalidationevents - An array of events that should trigger this cache to invalidate.
* sharingoptions - The sum of the possible sharing options that are applicable to the definition. An advanced setting.
* defaultsharing - The default sharing option to use. It's highly recommended that you don't set this unless there is a very specific reason not to use the system default.

It's important to note that internally the definition is also aware of the component. This is picked up when the definition is read, based upon the location of the caches.php file.

Expand Down
17 changes: 14 additions & 3 deletions cache/admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,15 @@
require_once($CFG->dirroot.'/cache/locallib.php');
require_once($CFG->dirroot.'/cache/forms.php');

// The first time the user visits this page we are going to reparse the definitions.
// Just ensures that everything is up to date.
// We flag is session so that this only happens once as people are likely to hit
// this page several times if making changes.
if (empty($SESSION->cacheadminreparsedefinitions)) {
cache_helper::update_definitions();
$SESSION->cacheadminreparsedefinitions = true;
}

$action = optional_param('action', null, PARAM_ALPHA);

admin_externalpage_setup('cacheconfig');
Expand Down Expand Up @@ -155,9 +164,10 @@
if (!array_key_exists($definition, $definitions)) {
throw new cache_exception('Invalid cache definition requested');
}
$title = get_string('editdefinitionmappings', 'cache', $definition);
$title = get_string('editdefinitionsharing', 'cache', $definition);
$sharingoptions = $definitions[$definition]['sharingoptions'];
$mform = new cache_definition_sharing_form($PAGE->url, array('definition' => $definition, 'sharingoptions' => $sharingoptions));
$customdata = array('definition' => $definition, 'sharingoptions' => $sharingoptions);
$mform = new cache_definition_sharing_form($PAGE->url, $customdata);
$mform->set_data(array(
'sharing' => $definitions[$definition]['selectedsharingoption'],
'userinputsharingkey' => $definitions[$definition]['userinputsharingkey']
Expand All @@ -167,7 +177,8 @@
} else if ($data = $mform->get_data()) {
$component = $definitions[$definition]['component'];
$area = $definitions[$definition]['area'];
cache_helper::purge_by_definition($component, $area);
// Purge the stores removing stale data before we alter the sharing option.
cache_helper::purge_stores_used_by_definition($component, $area);
$writer = cache_config_writer::instance();
$sharing = array_sum(array_keys($data->sharing));
$userinputsharingkey = $data->userinputsharingkey;
Expand Down
12 changes: 6 additions & 6 deletions cache/classes/config.php
Original file line number Diff line number Diff line change
Expand Up @@ -232,18 +232,18 @@ public function load($configuration = false) {
continue;
}
// Default the sharing option as it was added for 2.5.
// This can be removed sometime after the release of 2.6.
if (!array_key_exists('sharingoptions', $conf)) {
// This can be removed sometime after 2.5 is the minimum version someone can upgrade from.
if (!isset($conf['sharingoptions'])) {
$conf['sharingoptions'] = cache_definition::SHARING_DEFAULTOPTIONS;
}
// Default the selected sharing option as it was added for 2.5.
// This can be removed sometime after the release of 2.6.
if (!array_key_exists('selectedsharingoption', $conf)) {
// This can be removed sometime after 2.5 is the minimum version someone can upgrade from.
if (!isset($conf['selectedsharingoption'])) {
$conf['selectedsharingoption'] = cache_definition::SHARING_DEFAULT;
}
// Default the user input sharing key as it was added for 2.5.
// This can be removed sometime after the release of 2.6.
if (!array_key_exists('userinputsharingkey', $conf)) {
// This can be removed sometime after 2.5 is the minimum version someone can upgrade from.
if (!isset($conf['userinputsharingkey'])) {
$conf['userinputsharingkey'] = '';
}
$this->configdefinitions[$id] = $conf;
Expand Down
3 changes: 3 additions & 0 deletions cache/classes/definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@
* [array] An array of events that should cause this cache to invalidate some or all of the items within it.
* + sharingoptions
* [int] The sharing options that are appropriate for this definition. Should be the sum of the possible options.
* + defaultsharing
* [int] The default sharing option to use. It's highly recommended that you don't set this unless there is a very
* specific reason not to use the system default.
*
* For examples take a look at lib/db/caches.php
*
Expand Down
26 changes: 26 additions & 0 deletions cache/classes/helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,11 @@ public static function invalidate_by_event($event, array $keys) {
/**
* Purges the cache for a specific definition.
*
* If you need to purge a definition that requires identifiers or an aggregate and you don't
* know the details of those please use cache_helper::purge_stores_used_by_definition instead.
* It is a more aggressive purge and will purge all data within the store, not just the data
* belonging to the given definition.
*
* @todo MDL-36660: Change the signature: $aggregate must be added.
*
* @param string $component
Expand Down Expand Up @@ -455,6 +460,27 @@ public static function purge_store($storename) {
return true;
}

/**
* Purges all of the stores used by a definition.
*
* Unlike cache_helper::purge_by_definition this purges all of the data from the stores not
* just the data relating to the definition.
* This function is useful when you must purge a definition that requires setup but you don't
* want to set it up.
*
* @param string $component
* @param string $area
*/
public static function purge_stores_used_by_definition($component, $area) {
$factory = cache_factory::instance();
$config = $factory->create_config_instance();
$definition = $factory->create_definition($component, $area);
$stores = $config->get_stores_for_definition($definition);
foreach ($stores as $store) {
self::purge_store($store['name']);
}
}

/**
* Returns the translated name of the definition.
*
Expand Down
40 changes: 35 additions & 5 deletions cache/forms.php
Original file line number Diff line number Diff line change
Expand Up @@ -198,18 +198,19 @@ protected final function definition() {
$form = $this->_form;

$form->addElement('hidden', 'definition', $definition);
$form->setType('sharing', PARAM_SAFEPATH);
$form->setType('definition', PARAM_SAFEPATH);
$form->addElement('hidden', 'action', 'editdefinitionsharing');
$form->setType('sharing', PARAM_ALPHA);
$form->setType('action', PARAM_ALPHA);

// We use a group here for validation.
$count = 0;
$group = array();
foreach ($sharingoptions as $value => $text) {
$count++;
$name = ($count == 1) ? get_string('sharing', 'cache') : null;
$form->addElement('checkbox', 'sharing['.$value.']', $name, $text);
$group[] = $form->createElement('checkbox', $value, null, $text);
}
$form->addGroup($group, 'sharing', get_string('sharing', 'cache'), '<br />');
$form->setType('sharing', PARAM_INT);
$form->setDefault('sharing', cache_administration_helper::get_definition_sharing_options(cache_definition::SHARING_DEFAULT));

$form->addElement('text', 'userinputsharingkey', get_string('userinputsharingkey', 'cache'));
$form->addHelpButton('userinputsharingkey', 'userinputsharingkey', 'cache');
Expand All @@ -227,6 +228,35 @@ protected final function definition() {

$this->add_action_buttons();
}

/**
* Sets the data for this form.
*
* @param array $data
*/
public function set_data($data) {
if (!isset($data['sharing'])) {
// Set the default value here. mforms doesn't handle defaults very nicely.
$data['sharing'] = cache_administration_helper::get_definition_sharing_options(cache_definition::SHARING_DEFAULT);
}
parent::set_data($data);
}

/**
* Validates this form
*
* @param array $data
* @param array $files
* @return array
*/
public function validation($data, $files) {
$errors = parent::validation($data, $files);
if (count($errors) === 0 && !isset($data['sharing'])) {
// They must select at least one sharing option.
$errors['sharing'] = get_string('sharingrequired', 'cache');
}
return $errors;
}
}

/**
Expand Down
22 changes: 22 additions & 0 deletions cache/tests/cache_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -1305,4 +1305,26 @@ public function test_application_locking() {
$this->assertTrue($cache->delete('a'));
$this->assertFalse($cache->has('a'));
}

/**
* Test the static cache_helper method purge_stores_used_by_definition.
*/
public function test_purge_stores_used_by_definition() {
$instance = cache_config_phpunittest::instance(true);
$instance->phpunit_add_definition('phpunit/test_purge_stores_used_by_definition', array(
'mode' => cache_store::MODE_APPLICATION,
'component' => 'phpunit',
'area' => 'test_purge_stores_used_by_definition'
));
$cache = cache::make('phpunit', 'test_purge_stores_used_by_definition');
$this->assertInstanceOf('cache_application', $cache);
$this->assertTrue($cache->set('test', 'test'));
unset($cache);

cache_helper::purge_stores_used_by_definition('phpunit', 'test_purge_stores_used_by_definition');

$cache = cache::make('phpunit', 'test_purge_stores_used_by_definition');
$this->assertInstanceOf('cache_application', $cache);
$this->assertFalse($cache->get('test'));
}
}
2 changes: 2 additions & 0 deletions lang/en/cache.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
$string['editstore'] = 'Edit store';
$string['editstoresuccess'] = 'Succesfully edited the cache store.';
$string['editdefinitionmappings'] = '{$a} definition store mappings';
$string['editdefinitionsharing'] = 'Edit definition sharing for {$a}';
$string['ex_configcannotsave'] = 'Unable to save the cache config to file.';
$string['ex_nodefaultlock'] = 'Unable to find a default lock instance.';
$string['ex_unabletolock'] = 'Unable to acquire a lock for caching.';
Expand Down Expand Up @@ -130,6 +131,7 @@
$string['sharing_help'] = 'This allows you to determine how the cache data can be shared if you have a clustered setup, or if you have multiple sites all set up with the same store and wish to share the data. This is an advanced setting please make sure you understand its purpose before changing it.';
$string['sharing_siteid'] = 'Sites with the same site id.';
$string['sharing_version'] = 'Sites running the same version.';
$string['sharingrequired'] = 'You must select at least one sharing option.';
$string['sharingselected_all'] = 'Everyone';
$string['sharingselected_input'] = 'Custom key';
$string['sharingselected_siteid'] = 'Site identifier';
Expand Down

0 comments on commit 5f5776c

Please sign in to comment.