From 528a7b447adead6950f9338ba384a4635597129a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20S=CC=8Ckoda?= Date: Sat, 3 Aug 2013 10:48:15 +0200 Subject: [PATCH] MDL-41017 improve purify caching The improvements include: * HTMLPurifier cache is stored in localcachedir * allowobjectembed changes are not ignored any more * the cache keys include revision and all options which makes this suitable for local caches on cluster nodes * unchanged test is replaced by "true" value which should significantly improve performance * removal of purge_all_caches() hack for directory recreation * comments and coding style cleanup --- lib/db/caches.php | 1 + lib/moodlelib.php | 4 --- lib/testing/classes/util.php | 1 - lib/tests/htmlpurifier_test.php | 31 +++++++++++++++++ lib/weblib.php | 61 ++++++++++++++++++++++++--------- 5 files changed, 76 insertions(+), 22 deletions(-) diff --git a/lib/db/caches.php b/lib/db/caches.php index ffb2877d495..347c824c53b 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -81,6 +81,7 @@ $definitions = array( // This caches the html purifier cleaned text. This is done because the text is usually cleaned once for every user // and context combo. Text caching handles caching for the combination, this cache is responsible for caching the // cleaned text which is shareable. + // NOTE: this data may be safely stored in local caches on cluster nodes. 'htmlpurifier' => array( 'mode' => cache_store::MODE_APPLICATION, ), diff --git a/lib/moodlelib.php b/lib/moodlelib.php index c190a823f93..0f2f3fb3f9c 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -1596,10 +1596,6 @@ function purge_all_caches() { // Make sure cache dir is writable, throws exception if not. make_cache_directory(''); - // Hack: this script may get called after the purifier was initialised, - // but we do not want to verify repeatedly this exists in each call. - make_cache_directory('htmlpurifier'); - // This is the only place where we purge local caches, we are only adding files there. // The $CFG->localcachedirpurged flag forces local directories to be purged on cluster nodes. remove_dir($CFG->localcachedir, true); diff --git a/lib/testing/classes/util.php b/lib/testing/classes/util.php index 4dfa12c4cb6..5cef5ea5ba0 100644 --- a/lib/testing/classes/util.php +++ b/lib/testing/classes/util.php @@ -598,7 +598,6 @@ abstract class testing_util { closedir($handle); make_temp_directory(''); make_cache_directory(''); - make_cache_directory('htmlpurifier'); make_localcache_directory(''); // Reset the cache API so that it recreates it's required directories as well. cache_factory::reset(); diff --git a/lib/tests/htmlpurifier_test.php b/lib/tests/htmlpurifier_test.php index 3e803d9601f..88a16440ae1 100644 --- a/lib/tests/htmlpurifier_test.php +++ b/lib/tests/htmlpurifier_test.php @@ -124,6 +124,37 @@ class core_htmlpurifier_testcase extends basic_testcase { $this->assertSame('
Frog
', $result); } + public function test_allowobjectembed() { + global $CFG; + + $this->assertSame('0', $CFG->allowobjectembed); + + $text = ' + + + +hmmm'; + $result = purify_html($text, array()); + $this->assertSame('hmmm', trim($result)); + + $CFG->allowobjectembed = '1'; + + $expected = ' + + + + + +hmmm'; + $result = purify_html($text, array()); + $this->assertSame(str_replace("\n", '', $expected), str_replace("\n", '', $result)); + + $CFG->allowobjectembed = '0'; + + $result = purify_html($text, array()); + $this->assertSame('hmmm', trim($result)); + } + /** * Test if linebreaks kept unchanged. */ diff --git a/lib/weblib.php b/lib/weblib.php index c9b617574f1..43e58092ea2 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -1615,27 +1615,47 @@ function is_purify_html_necessary($text) { function purify_html($text, $options = array()) { global $CFG; + $text = (string)$text; + static $purifiers = array(); static $caches = array(); - $type = !empty($options['allowid']) ? 'allowid' : 'normal'; + // Purifier code can change only during major version upgrade. + $version = empty($CFG->version) ? 0 : $CFG->version; + $cachedir = "$CFG->localcachedir/htmlpurifier/$version"; + if (!file_exists($cachedir)) { + // Purging of caches may remove the cache dir at any time, + // luckily file_exists() results should be cached for all existing directories. + $purifiers = array(); + $caches = array(); + gc_collect_cycles(); + + make_localcache_directory('htmlpurifier', false); + check_dir_exists($cachedir); + } + + $allowid = empty($options['allowid']) ? 0 : 1; + $allowobjectembed = empty($CFG->allowobjectembed) ? 0 : 1; + + $type = 'type_'.$allowid.'_'.$allowobjectembed; if (!array_key_exists($type, $caches)) { $caches[$type] = cache::make('core', 'htmlpurifier', array('type' => $type)); } $cache = $caches[$type]; - $filteredtext = $cache->get($text); - if ($filteredtext !== false) { + // Add revision number and all options to the text key so that it is compatible with local cluster node caches. + $key = "|$version|$allowobjectembed|$allowid|$text"; + $filteredtext = $cache->get($key); + + if ($filteredtext === true) { + // The filtering did not change the text last time, no need to filter anything again. + return $text; + } else if ($filteredtext !== false) { return $filteredtext; } if (empty($purifiers[$type])) { - - // make sure the serializer dir exists, it should be fine if it disappears later during cache reset - $cachedir = $CFG->cachedir.'/htmlpurifier'; - check_dir_exists($cachedir); - require_once $CFG->libdir.'/htmlpurifier/HTMLPurifier.safe-includes.php'; require_once $CFG->libdir.'/htmlpurifier/locallib.php'; $config = HTMLPurifier_Config::createDefault(); @@ -1651,22 +1671,22 @@ function purify_html($text, $options = array()) { $config->set('URI.AllowedSchemes', array('http'=>true, 'https'=>true, 'ftp'=>true, 'irc'=>true, 'nntp'=>true, 'news'=>true, 'rtsp'=>true, 'teamspeak'=>true, 'gopher'=>true, 'mms'=>true, 'mailto'=>true)); $config->set('Attr.AllowedFrameTargets', array('_blank')); - if (!empty($CFG->allowobjectembed)) { + if ($allowobjectembed) { $config->set('HTML.SafeObject', true); $config->set('Output.FlashCompat', true); $config->set('HTML.SafeEmbed', true); } - if ($type === 'allowid') { + if ($allowid) { $config->set('Attr.EnableID', true); } if ($def = $config->maybeGetRawHTMLDefinition()) { - $def->addElement('nolink', 'Block', 'Flow', array()); // skip our filters inside - $def->addElement('tex', 'Inline', 'Inline', array()); // tex syntax, equivalent to $$xx$$ - $def->addElement('algebra', 'Inline', 'Inline', array()); // algebra syntax, equivalent to @@xx@@ - $def->addElement('lang', 'Block', 'Flow', array(), array('lang'=>'CDATA')); // old and future style multilang - only our hacked lang attribute - $def->addAttribute('span', 'xxxlang', 'CDATA'); // current problematic multilang + $def->addElement('nolink', 'Block', 'Flow', array()); // Skip our filters inside. + $def->addElement('tex', 'Inline', 'Inline', array()); // Tex syntax, equivalent to $$xx$$. + $def->addElement('algebra', 'Inline', 'Inline', array()); // Algebra syntax, equivalent to @@xx@@. + $def->addElement('lang', 'Block', 'Flow', array(), array('lang'=>'CDATA')); // Original multilang style - only our hacked lang attribute. + $def->addAttribute('span', 'xxxlang', 'CDATA'); // Current very problematic multilang. } $purifier = new HTMLPurifier($config); @@ -1681,11 +1701,18 @@ function purify_html($text, $options = array()) { if ($multilang) { $filteredtext = preg_replace('//', '', $filteredtext); } - $filteredtext = $purifier->purify($filteredtext); + $filteredtext = (string)$purifier->purify($filteredtext); if ($multilang) { $filteredtext = preg_replace('//', '', $filteredtext); } - $cache->set($text, $filteredtext); + + if ($text === $filteredtext) { + // No need to store the filtered text, next time we will just return unfiltered text + // because it was not changed by purifying. + $cache->set($key, true); + } else { + $cache->set($key, $filteredtext); + } return $filteredtext; }