From 43d270e4ef3012c41cc9eea93d7bbf243fdfa16e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 20 Jan 2015 18:05:23 +0000 Subject: [PATCH 01/10] MDL-47962 filterlib: Fix lots of coding style --- lib/filterlib.php | 246 ++++++++++++++++++++++------------------------ 1 file changed, 117 insertions(+), 129 deletions(-) diff --git a/lib/filterlib.php b/lib/filterlib.php index bbb928947c7..bd6f6d96cd8 100644 --- a/lib/filterlib.php +++ b/lib/filterlib.php @@ -216,7 +216,7 @@ class filter_manager { public function filter_text($text, $context, array $options = array(), array $skipfilters = null) { $text = $this->apply_filter_chain($text, $this->get_text_filters($context), $options, $skipfilters); - // tags removed for XHTML compatibility + // Remove tags for XHTML compatibility. $text = str_replace(array('', ''), '', $text); return $text; } @@ -465,28 +465,28 @@ abstract class moodle_text_filter { * * @copyright 1999 onwards Martin Dougiamas {@link http://moodle.com} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - **/ + */ class filterobject { /** @var string */ - var $phrase; - var $hreftagbegin; - var $hreftagend; + public $phrase; + public $hreftagbegin; + public $hreftagend; /** @var bool */ - var $casesensitive; - var $fullmatch; + public $casesensitive; + public $fullmatch; /** @var mixed */ - var $replacementphrase; - var $work_phrase; - var $work_hreftagbegin; - var $work_hreftagend; - var $work_casesensitive; - var $work_fullmatch; - var $work_replacementphrase; + public $replacementphrase; + public $work_phrase; + public $work_hreftagbegin; + public $work_hreftagend; + public $work_casesensitive; + public $work_fullmatch; + public $work_replacementphrase; /** @var bool */ - var $work_calculated; + public $work_calculated; /** - * A constructor just because I like constructing + * Constructor. * * @param string $phrase * @param string $hreftagbegin @@ -499,15 +499,15 @@ class filterobject { $hreftagend = '', $casesensitive = false, $fullmatch = false, - $replacementphrase = NULL) { + $replacementphrase = null) { - $this->phrase = $phrase; - $this->hreftagbegin = $hreftagbegin; - $this->hreftagend = $hreftagend; - $this->casesensitive = $casesensitive; - $this->fullmatch = $fullmatch; - $this->replacementphrase= $replacementphrase; - $this->work_calculated = false; + $this->phrase = $phrase; + $this->hreftagbegin = $hreftagbegin; + $this->hreftagend = $hreftagend; + $this->casesensitive = $casesensitive; + $this->fullmatch = $fullmatch; + $this->replacementphrase = $replacementphrase; + $this->work_calculated = false; } } @@ -574,7 +574,6 @@ function filter_set_global_state($filtername, $state, $move = 0) { } if (strpos($filtername, 'filter/') === 0) { - //debugging("Old filtername '$filtername' parameter used in filter_set_global_state()", DEBUG_DEVELOPER); $filtername = substr($filtername, 7); } else if (strpos($filtername, '/') !== false) { throw new coding_exception("Invalid filter name '$filtername' used in filter_set_global_state()"); @@ -588,7 +587,7 @@ function filter_set_global_state($filtername, $state, $move = 0) { $on = array(); $off = array(); - foreach($filters as $f) { + foreach ($filters as $f) { if ($f->active == TEXTFILTER_DISABLED) { $off[$f->filter] = $f; } else { @@ -675,13 +674,13 @@ function filter_set_global_state($filtername, $state, $move = 0) { $i = 1; foreach ($on as $f) { if ($f->sortorder != $i) { - $DB->set_field('filter_active', 'sortorder', $i, array('id'=>$f->id)); + $DB->set_field('filter_active', 'sortorder', $i, array('id' => $f->id)); } $i++; } foreach ($off as $f) { if ($f->sortorder != $i) { - $DB->set_field('filter_active', 'sortorder', $i, array('id'=>$f->id)); + $DB->set_field('filter_active', 'sortorder', $i, array('id' => $f->id)); } $i++; } @@ -696,7 +695,6 @@ function filter_set_global_state($filtername, $state, $move = 0) { */ function filter_is_enabled($filtername) { if (strpos($filtername, 'filter/') === 0) { - //debugging("Old filtername '$filtername' parameter used in filter_is_enabled()", DEBUG_DEVELOPER); $filtername = substr($filtername, 7); } else if (strpos($filtername, '/') !== false) { throw new coding_exception("Invalid filter name '$filtername' used in filter_is_enabled()"); @@ -707,7 +705,6 @@ function filter_is_enabled($filtername) { /** * Return a list of all the filters that may be in use somewhere. * - * @staticvar array $enabledfilters * @return array where the keys and values are both the filter name, like 'tex'. */ function filter_get_globally_enabled() { @@ -965,7 +962,7 @@ function filter_get_active_in_context($context) { $contextids = str_replace('/', ',', trim($context->path, '/')); // The following SQL is tricky. It is explained on - // http://docs.moodle.org/dev/Filter_enable/disable_by_context + // http://docs.moodle.org/dev/Filter_enable/disable_by_context. $sql = "SELECT active.filter, fc.name, fc.value FROM (SELECT f.filter, MAX(f.sortorder) AS sortorder FROM {filter_active} f @@ -1007,7 +1004,7 @@ function filter_preload_activities(course_modinfo $modinfo) { $FILTERLIB_PRIVATE = new stdClass(); } - // Don't repeat preload + // Don't repeat preload. if (!isset($FILTERLIB_PRIVATE->preloaded)) { $FILTERLIB_PRIVATE->preloaded = array(); } @@ -1016,7 +1013,7 @@ function filter_preload_activities(course_modinfo $modinfo) { } $FILTERLIB_PRIVATE->preloaded[$modinfo->get_course_id()] = true; - // Get contexts for all CMs + // Get contexts for all CMs. $cmcontexts = array(); $cmcontextids = array(); foreach ($modinfo->get_cms() as $cm) { @@ -1025,16 +1022,16 @@ function filter_preload_activities(course_modinfo $modinfo) { $cmcontexts[] = $modulecontext; } - // Get course context and all other parents... + // Get course context and all other parents. $coursecontext = context_course::instance($modinfo->get_course_id()); $parentcontextids = explode('/', substr($coursecontext->path, 1)); $allcontextids = array_merge($cmcontextids, $parentcontextids); - // Get all filter_active rows relating to all these contexts + // Get all filter_active rows relating to all these contexts. list ($sql, $params) = $DB->get_in_or_equal($allcontextids); $filteractives = $DB->get_records_select('filter_active', "contextid $sql", $params, 'sortorder'); - // Get all filter_config only for the cm contexts + // Get all filter_config only for the cm contexts. list ($sql, $params) = $DB->get_in_or_equal($cmcontextids); $filterconfigs = $DB->get_records_select('filter_config', "contextid $sql", $params); @@ -1044,20 +1041,20 @@ function filter_preload_activities(course_modinfo $modinfo) { // filter_get_active_in_context, this does seem to be correct. // Build course default active list. Initially this will be an array of - // filter name => active score (where an active score >0 means it's active) + // filter name => active score (where an active score >0 means it's active). $courseactive = array(); - // Also build list of filter_active rows below course level, by contextid + // Also build list of filter_active rows below course level, by contextid. $remainingactives = array(); - // Array lists filters that are banned at top level + // Array lists filters that are banned at top level. $banned = array(); - // Add any active filters in parent contexts to the array + // Add any active filters in parent contexts to the array. foreach ($filteractives as $row) { $depth = array_search($row->contextid, $parentcontextids); if ($depth !== false) { - // Find entry + // Find entry. if (!array_key_exists($row->filter, $courseactive)) { $courseactive[$row->filter] = 0; } @@ -1072,7 +1069,7 @@ function filter_preload_activities(course_modinfo $modinfo) { $banned[$row->filter] = true; } } else { - // Build list of other rows indexed by contextid + // Build list of other rows indexed by contextid. if (!array_key_exists($row->contextid, $remainingactives)) { $remainingactives[$row->contextid] = array(); } @@ -1081,7 +1078,7 @@ function filter_preload_activities(course_modinfo $modinfo) { } // Chuck away the ones that aren't active. - foreach ($courseactive as $filter=>$score) { + foreach ($courseactive as $filter => $score) { if ($score <= 0) { unset($courseactive[$filter]); } else { @@ -1095,7 +1092,7 @@ function filter_preload_activities(course_modinfo $modinfo) { $FILTERLIB_PRIVATE->active = array(); } foreach ($cmcontextids as $contextid) { - // Copy course list + // Copy course list. $FILTERLIB_PRIVATE->active[$contextid] = $courseactive; // Are there any changes to the active list? @@ -1242,16 +1239,15 @@ function filter_context_may_have_filter_settings($context) { /** * Process phrases intelligently found within a HTML text (such as adding links). * - * @staticvar array $usedpharses - * @param string $text the text that we are filtering - * @param array $link_array an array of filterobjects + * @param string $text the text that we are filtering + * @param array $linkarray an array of filterobjects * @param array $ignoretagsopen an array of opening tags that we should ignore while filtering * @param array $ignoretagsclose an array of corresponding closing tags * @param bool $overridedefaultignore True to only use tags provided by arguments * @return string - **/ -function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagsclose=NULL, - $overridedefaultignore=false) { + */ +function filter_phrases($text, $linkarray, $ignoretagsopen = null, $ignoretagsclose = null, + $overridedefaultignore = false) { global $CFG; @@ -1261,14 +1257,14 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl $tags = array(); // To store all the simple tags to be ignored. if (!$overridedefaultignore) { - // A list of open/close tags that we should not replace within - // Extended to include ', '', '',''); + '', '', '', ''); } else { // Set an empty default list. $filterignoretagsopen = array(); @@ -1285,29 +1281,26 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl } } - // Invalid prefixes and suffixes for the fullmatch searches + // Invalid prefixes and suffixes for the fullmatch searches. // Every "word" character, but the underscore, is a invalid suffix or prefix. // (nice to use this because it includes national characters (accents...) as word characters. $filterinvalidprefixes = '([^\W_])'; $filterinvalidsuffixes = '([^\W_])'; - // Double up some magic chars to avoid "accidental matches" - $text = preg_replace('/([#*%])/','\1\1',$text); + // Double up some magic chars to avoid "accidental matches". + $text = preg_replace('/([#*%])/', '\1\1', $text); + // Remove everything enclosed by the ignore tags from $text. + filter_save_ignore_tags($text, $filterignoretagsopen, $filterignoretagsclose, $ignoretags); - //Remove everything enclosed by the ignore tags from $text - filter_save_ignore_tags($text,$filterignoretagsopen,$filterignoretagsclose,$ignoretags); + // Remove tags from $text. + filter_save_tags($text, $tags); - // Remove tags from $text - filter_save_tags($text,$tags); + // Time to cycle through each phrase to be linked. + foreach ($linkarray as $linkobject) { - // Time to cycle through each phrase to be linked - $size = sizeof($link_array); - for ($n=0; $n < $size; $n++) { - $linkobject =& $link_array[$n]; - - // Set some defaults if certain properties are missing - // Properties may be missing if the filterobject class has not been used to construct the object + // Set some defaults if certain properties are missing. + // Properties may be missing if the filterobject class has not been used to construct the object. if (empty($linkobject->phrase)) { continue; } @@ -1318,8 +1311,8 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl continue; } - // All this work has to be done ONLY it it hasn't been done before - if (!$linkobject->work_calculated) { + // All this work has to be done ONLY it it hasn't been done before. + if (!$linkobject->work_calculated) { if (!isset($linkobject->hreftagbegin) or !isset($linkobject->hreftagend)) { $linkobject->work_hreftagbegin = 'work_hreftagend = ''; @@ -1330,7 +1323,7 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl // Double up chars to protect true duplicates // be cleared up before returning to the user. - $linkobject->work_hreftagbegin = preg_replace('/([#*%])/','\1\1',$linkobject->work_hreftagbegin); + $linkobject->work_hreftagbegin = preg_replace('/([#*%])/', '\1\1', $linkobject->work_hreftagbegin); if (empty($linkobject->casesensitive)) { $linkobject->work_casesensitive = false; @@ -1343,58 +1336,58 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl $linkobject->work_fullmatch = true; } - // Strip tags out of the phrase + // Strip tags out of the phrase. $linkobject->work_phrase = strip_tags($linkobject->phrase); // Double up chars that might cause a false match -- the duplicates will // be cleared up before returning to the user. - $linkobject->work_phrase = preg_replace('/([#*%])/','\1\1',$linkobject->work_phrase); + $linkobject->work_phrase = preg_replace('/([#*%])/', '\1\1', $linkobject->work_phrase); - // Set the replacement phrase properly - if ($linkobject->replacementphrase) { //We have specified a replacement phrase - // Strip tags + // Set the replacement phrase properly. + if ($linkobject->replacementphrase) { // We have specified a replacement phrase. + // Strip tags. $linkobject->work_replacementphrase = strip_tags($linkobject->replacementphrase); - } else { //The replacement is the original phrase as matched below + } else { // The replacement is the original phrase as matched below. $linkobject->work_replacementphrase = '$1'; } - // Quote any regular expression characters and the delimiter in the work phrase to be searched + // Quote any regular expression characters and the delimiter in the work phrase to be searched. $linkobject->work_phrase = preg_quote($linkobject->work_phrase, '/'); - // Work calculated + // Work calculated. $linkobject->work_calculated = true; - } - // If $CFG->filtermatchoneperpage, avoid previously (request) linked phrases + // If $CFG->filtermatchoneperpage, avoid previously (request) linked phrases. if (!empty($CFG->filtermatchoneperpage)) { - if (!empty($usedphrases) && in_array($linkobject->work_phrase,$usedphrases)) { + if (!empty($usedphrases) && in_array($linkobject->work_phrase, $usedphrases)) { continue; } } - // Regular expression modifiers - $modifiers = ($linkobject->work_casesensitive) ? 's' : 'isu'; // works in unicode mode! + // Regular expression modifiers. + $modifiers = ($linkobject->work_casesensitive) ? 's' : 'isu'; // Works in unicode mode! // Do we need to do a fullmatch? - // If yes then go through and remove any non full matching entries + // If yes then go through and remove any non full matching entries. if ($linkobject->work_fullmatch) { $notfullmatches = array(); - $regexp = '/'.$filterinvalidprefixes.'('.$linkobject->work_phrase.')|('.$linkobject->work_phrase.')'.$filterinvalidsuffixes.'/'.$modifiers; + $regexp = '/'.$filterinvalidprefixes.'('.$linkobject->work_phrase.')|('. + $linkobject->work_phrase.')'.$filterinvalidsuffixes.'/'.$modifiers; - preg_match_all($regexp,$text,$list_of_notfullmatches); + preg_match_all($regexp, $text, $listofnotfullmatches); - if ($list_of_notfullmatches) { - foreach (array_unique($list_of_notfullmatches[0]) as $key=>$value) { + if ($listofnotfullmatches) { + foreach (array_unique($listofnotfullmatches[0]) as $key => $value) { $notfullmatches['<*'.$key.'*>'] = $value; } if (!empty($notfullmatches)) { - $text = str_replace($notfullmatches,array_keys($notfullmatches),$text); + $text = str_replace($notfullmatches, array_keys($notfullmatches), $text); } } } - // Finally we do our highlighting + // Finally we do our highlighting. if (!empty($CFG->filtermatchonepertext) || !empty($CFG->filtermatchoneperpage)) { $resulttext = preg_replace('/('.$linkobject->work_phrase.')/'.$modifiers, $linkobject->work_hreftagbegin. @@ -1407,47 +1400,42 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl $linkobject->work_hreftagend, $text); } - - // If the text has changed we have to look for links again + // If the text has changed we have to look for links again. if ($resulttext != $text) { - // Set $text to $resulttext $text = $resulttext; - // Remove everything enclosed by the ignore tags from $text - filter_save_ignore_tags($text,$filterignoretagsopen,$filterignoretagsclose,$ignoretags); - // Remove tags from $text - filter_save_tags($text,$tags); - // If $CFG->filtermatchoneperpage, save linked phrases to request + // Remove everything enclosed by the ignore tags from $text. + filter_save_ignore_tags($text, $filterignoretagsopen, $filterignoretagsclose, $ignoretags); + // Remove tags from $text. + filter_save_tags($text, $tags); + // If $CFG->filtermatchoneperpage, save linked phrases to request. if (!empty($CFG->filtermatchoneperpage)) { $usedphrases[] = $linkobject->work_phrase; } } - - // Replace the not full matches before cycling to next link object + // Replace the not full matches before cycling to next link object. if (!empty($notfullmatches)) { - $text = str_replace(array_keys($notfullmatches),$notfullmatches,$text); + $text = str_replace(array_keys($notfullmatches), $notfullmatches, $text); unset($notfullmatches); } } - // Rebuild the text with all the excluded areas - + // Rebuild the text with all the excluded areas. if (!empty($tags)) { $text = str_replace(array_keys($tags), $tags, $text); } if (!empty($ignoretags)) { $ignoretags = array_reverse($ignoretags); // Reversed so "progressive" str_replace() will solve some nesting problems. - $text = str_replace(array_keys($ignoretags),$ignoretags,$text); + $text = str_replace(array_keys($ignoretags), $ignoretags, $text); } - // Remove the protective doubleups - $text = preg_replace('/([#*%])(\1)/','\1',$text); + // Remove the protective doubleups. + $text = preg_replace('/([#*%])(\1)/', '\1', $text); - // Add missing javascript for popus + // Add missing javascript for popus. $text = filter_add_javascript($text); - return $text; } @@ -1459,12 +1447,12 @@ function filter_phrases($text, &$link_array, $ignoretagsopen=NULL, $ignoretagscl */ function filter_remove_duplicates($linkarray) { - $concepts = array(); // keep a record of concepts as we cycle through - $lconcepts = array(); // a lower case version for case insensitive + $concepts = array(); // Keep a record of concepts as we cycle through. + $lconcepts = array(); // A lower case version for case insensitive. $cleanlinks = array(); - foreach ($linkarray as $key=>$filterobject) { + foreach ($linkarray as $key => $filterobject) { if ($filterobject->casesensitive) { $exists = in_array($filterobject->phrase, $concepts); } else { @@ -1494,21 +1482,21 @@ function filter_remove_duplicates($linkarray) { **/ function filter_save_ignore_tags(&$text, $filterignoretagsopen, $filterignoretagsclose, &$ignoretags) { - // Remove everything enclosed by the ignore tags from $text - foreach ($filterignoretagsopen as $ikey=>$opentag) { + // Remove everything enclosed by the ignore tags from $text. + foreach ($filterignoretagsopen as $ikey => $opentag) { $closetag = $filterignoretagsclose[$ikey]; - // form regular expression - $opentag = str_replace('/','\/',$opentag); // delimit forward slashes - $closetag = str_replace('/','\/',$closetag); // delimit forward slashes + // Form regular expression. + $opentag = str_replace('/', '\/', $opentag); // Delimit forward slashes. + $closetag = str_replace('/', '\/', $closetag); // Delimit forward slashes. $pregexp = '/'.$opentag.'(.*?)'.$closetag.'/is'; - preg_match_all($pregexp, $text, $list_of_ignores); - foreach (array_unique($list_of_ignores[0]) as $key=>$value) { - $prefix = (string)(count($ignoretags) + 1); + preg_match_all($pregexp, $text, $listofignores); + foreach (array_unique($listofignores[0]) as $key => $value) { + $prefix = (string) (count($ignoretags) + 1); $ignoretags['<#'.$prefix.TEXTFILTER_EXCL_SEPARATOR.$key.'#>'] = $value; } if (!empty($ignoretags)) { - $text = str_replace($ignoretags,array_keys($ignoretags),$text); + $text = str_replace($ignoretags, array_keys($ignoretags), $text); } } } @@ -1523,13 +1511,13 @@ function filter_save_ignore_tags(&$text, $filterignoretagsopen, $filterignoretag **/ function filter_save_tags(&$text, &$tags) { - preg_match_all('/<([^#%*].*?)>/is',$text,$list_of_newtags); - foreach (array_unique($list_of_newtags[0]) as $ntkey=>$value) { + preg_match_all('/<([^#%*].*?)>/is', $text, $listofnewtags); + foreach (array_unique($listofnewtags[0]) as $ntkey => $value) { $prefix = (string)(count($tags) + 1); $tags['<%'.$prefix.TEXTFILTER_EXCL_SEPARATOR.$ntkey.'%>'] = $value; } if (!empty($tags)) { - $text = str_replace($tags,array_keys($tags),$text); + $text = str_replace($tags, array_keys($tags), $text); } } @@ -1542,13 +1530,13 @@ function filter_save_tags(&$text, &$tags) { function filter_add_javascript($text) { global $CFG; - if (stripos($text, '') === FALSE) { + if (stripos($text, '') === false) { return $text; // This is not a html file. } - if (strpos($text, 'onclick="return openpopup') === FALSE) { + if (strpos($text, 'onclick="return openpopup') === false) { return $text; // No popup - no need to add javascript. } - $js =" + $js = " "; - if (stripos($text, '') !== FALSE) { + if (stripos($text, '') !== false) { // Try to add it into the head element. $text = str_ireplace('', $js.'', $text); return $text; From c633345265dc87ba9aa6b79905b81b88c1af0c36 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 20 Jan 2015 18:14:29 +0000 Subject: [PATCH 02/10] MDL-47962 filter_glossary: $GLOSSARY_EXCLUDEENTRY shouldn't stop caching Also, greatly improved unit tests, to test more cases of how the filter should work. --- filter/glossary/filter.php | 58 ++++++---- filter/glossary/tests/filter_test.php | 156 +++++++++++++++++++++++++- 2 files changed, 191 insertions(+), 23 deletions(-) diff --git a/filter/glossary/filter.php b/filter/glossary/filter.php index 5c4ba448122..46c5f917bed 100644 --- a/filter/glossary/filter.php +++ b/filter/glossary/filter.php @@ -50,8 +50,13 @@ class filter_glossary extends moodle_text_filter { } } - public function filter($text, array $options = array()) { - global $CFG, $USER, $GLOSSARY_EXCLUDEENTRY; + /** + * Get all the concepts for this context. + * @return filterobject[] the concepts, and filterobjects, with an added + * ->conceptid field. + */ + protected function get_all_concepts() { + global $CFG, $USER; // Try to get current course. $coursectx = $this->context->get_course_context(false); @@ -67,11 +72,8 @@ class filter_glossary extends moodle_text_filter { $this->cacheconceptlist = null; } - if (is_array($this->cacheconceptlist) and empty($GLOSSARY_EXCLUDEENTRY)) { - if (empty($this->cacheconceptlist)) { - return $text; - } - return filter_phrases($text, $this->cacheconceptlist); + if (is_array($this->cacheconceptlist)) { + return $this->cacheconceptlist; } list($glossaries, $allconcepts) = \mod_glossary\local\concept_cache::get_concepts($courseid); @@ -80,20 +82,15 @@ class filter_glossary extends moodle_text_filter { $this->cacheuserid = $USER->id; $this->cachecourseid = $courseid; $this->cacheconcepts = array(); - return $text; + return $this->cacheconceptlist; } $strcategory = get_string('category', 'glossary'); $conceptlist = array(); - $excluded = false; foreach ($allconcepts as $concepts) { foreach ($concepts as $concept) { - if (!empty($GLOSSARY_EXCLUDEENTRY) and $concept->id == $GLOSSARY_EXCLUDEENTRY) { - $excluded = true; - continue; - } if ($concept->category) { // Link to a category. // TODO: Fix this string usage. $title = $glossaries[$concept->glossaryid] . ': ' . $strcategory . ' ' . $concept->concept; @@ -102,6 +99,7 @@ class filter_glossary extends moodle_text_filter { 'href' => $link, 'title' => $title, 'class' => 'glossary autolink category glossaryid' . $concept->glossaryid); + $conceptid = 0; } else { // Link to entry or alias $title = $glossaries[$concept->glossaryid] . ': ' . $concept->concept; @@ -114,6 +112,7 @@ class filter_glossary extends moodle_text_filter { 'href' => $link, 'title' => str_replace('&', '&', $title), // Undo the s() mangling. 'class' => 'glossary autolink concept glossaryid' . $concept->glossaryid); + $conceptid = $concept->id; } // This flag is optionally set by resource_pluginfile() // if processing an embedded file use target to prevent getting nested Moodles. @@ -122,23 +121,42 @@ class filter_glossary extends moodle_text_filter { } $href_tag_begin = html_writer::start_tag('a', $attributes); - $conceptlist[] = new filterobject($concept->concept, $href_tag_begin, '', - $concept->casesensitive, $concept->fullmatch); + $filterobj = new filterobject($concept->concept, $href_tag_begin, '', + $concept->casesensitive, $concept->fullmatch);; + $filterobj->conceptid = $conceptid; + $conceptlist[] = $filterobj; } } usort($conceptlist, 'filter_glossary::sort_entries_by_length'); - if (!$excluded) { - // Do not cache the excluded list here, it is used once per page only. - $this->cacheuserid = $USER->id; - $this->cachecourseid = $courseid; - $this->cacheconceptlist = $conceptlist; + $this->cacheuserid = $USER->id; + $this->cachecourseid = $courseid; + $this->cacheconceptlist = $conceptlist; + return $this->cacheconceptlist; + } + + public function filter($text, array $options = array()) { + global $GLOSSARY_EXCLUDEENTRY; + + $conceptlist = $this->get_all_concepts(); + + if (empty($conceptlist)) { + return $text; + } + + if (!empty($GLOSSARY_EXCLUDEENTRY)) { + foreach ($conceptlist as $key => $filterobj) { + if ($filterobj->conceptid == $GLOSSARY_EXCLUDEENTRY) { + unset($conceptlist[$key]); + } + } } if (empty($conceptlist)) { return $text; } + return filter_phrases($text, $conceptlist); // Actually search for concepts! } diff --git a/filter/glossary/tests/filter_test.php b/filter/glossary/tests/filter_test.php index ccf54eda2ad..99e1b0935e7 100644 --- a/filter/glossary/tests/filter_test.php +++ b/filter/glossary/tests/filter_test.php @@ -33,6 +33,82 @@ require_once($CFG->dirroot . '/filter/glossary/filter.php'); // Include the code */ class filter_glossary_filter_testcase extends advanced_testcase { + public function test_link_to_entry_with_alias() { + global $CFG; + $this->resetAfterTest(true); + + // Enable glossary filter at top level. + filter_set_global_state('glossary', TEXTFILTER_ON); + $CFG->glossary_linkentries = 1; + + // Create a test course. + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + // Create a glossary. + $glossary = $this->getDataGenerator()->create_module('glossary', + array('course' => $course->id, 'mainglossary' => 1)); + + // Create two entries with ampersands and one normal entry. + $generator = $this->getDataGenerator()->get_plugin_generator('mod_glossary'); + $normal = $generator->create_content($glossary, array('concept' => 'entry name'), + array('first alias', 'second alias')); + + // Format text with all three entries in HTML. + $html = '

First we have entry name, then we have it twp aliases first alias and second alias.

'; + $filtered = format_text($html, FORMAT_HTML, array('context' => $context)); + + // Find all the glossary links in the result. + $matches = array(); + preg_match_all('~eid=([0-9]+).*?title="(.*?)"~', $filtered, $matches); + + // There should be 3 glossary links. + $this->assertEquals(3, count($matches[1])); + $this->assertEquals($normal->id, $matches[1][0]); + $this->assertEquals($normal->id, $matches[1][1]); + $this->assertEquals($normal->id, $matches[1][2]); + + // Check text of title attribute. + $this->assertEquals($glossary->name . ': entry name', $matches[2][0]); + $this->assertEquals($glossary->name . ': first alias', $matches[2][1]); + $this->assertEquals($glossary->name . ': second alias', $matches[2][2]); + } + + public function test_link_to_category() { + global $CFG; + $this->resetAfterTest(true); + + // Enable glossary filter at top level. + filter_set_global_state('glossary', TEXTFILTER_ON); + $CFG->glossary_linkentries = 1; + + // Create a test course. + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + // Create a glossary. + $glossary = $this->getDataGenerator()->create_module('glossary', + array('course' => $course->id, 'mainglossary' => 1)); + + // Create two entries with ampersands and one normal entry. + /** @var mod_glossary_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('mod_glossary'); + $category = $generator->create_category($glossary, array('name' => 'My category', 'usedynalink' => 1)); + + // Format text with all three entries in HTML. + $html = '

This is My category you know.

'; + $filtered = format_text($html, FORMAT_HTML, array('context' => $context)); + + // Find all the glossary links in the result. + $matches = array(); + preg_match_all('~hook=([0-9]+).*?title="(.*?)"~', $filtered, $matches); + + // There should be 1 glossary link. + $this->assertEquals(1, count($matches[1])); + $this->assertEquals($category->id, $matches[1][0]); + $this->assertEquals($glossary->name . ': Category My category', $matches[2][0]); + } + /** * Test ampersands. */ @@ -59,9 +135,6 @@ class filter_glossary_filter_testcase extends advanced_testcase { $amp1 = $generator->create_content($glossary, array('concept' => 'A&B')); $amp2 = $generator->create_content($glossary, array('concept' => 'C&D')); - filter_manager::reset_caches(); - \mod_glossary\local\concept_cache::reset_caches(); - // Format text with all three entries in HTML. $html = '

A&B C&D normal

'; $filtered = format_text($html, FORMAT_HTML, array('context' => $context)); @@ -81,4 +154,81 @@ class filter_glossary_filter_testcase extends advanced_testcase { $this->assertEquals($glossary->name . ': C&D', $matches[2][1]); $this->assertEquals($glossary->name . ': normal', $matches[2][2]); } + + public function test_exclude_excludes_link_to_entry_with_alias() { + global $CFG, $GLOSSARY_EXCLUDEENTRY; + + $this->resetAfterTest(true); + + // Enable glossary filter at top level. + filter_set_global_state('glossary', TEXTFILTER_ON); + $CFG->glossary_linkentries = 1; + + // Create a test course. + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + // Create a glossary. + $glossary = $this->getDataGenerator()->create_module('glossary', + array('course' => $course->id, 'mainglossary' => 1)); + + // Create two entries with ampersands and one normal entry. + $generator = $this->getDataGenerator()->get_plugin_generator('mod_glossary'); + $tobeexcluded = $generator->create_content($glossary, array('concept' => 'entry name'), + array('first alias', 'second alias')); + $normal = $generator->create_content($glossary, array('concept' => 'other entry')); + + // Format text with all three entries in HTML. + $html = '

First we have entry name, then we have it twp aliases first alias and second alias. ' . + 'In this case, those should not be linked, but this other entry should be.

'; + $GLOSSARY_EXCLUDEENTRY = $tobeexcluded->id; + $filtered = format_text($html, FORMAT_HTML, array('context' => $context)); + $GLOSSARY_EXCLUDEENTRY = null; + + // Find all the glossary links in the result. + $matches = array(); + preg_match_all('~eid=([0-9]+).*?title="(.*?)"~', $filtered, $matches); + + // There should be 1 glossary links. + $this->assertEquals(1, count($matches[1])); + $this->assertEquals($normal->id, $matches[1][0]); + $this->assertEquals($glossary->name . ': other entry', $matches[2][0]); + } + + public function test_exclude_does_not_exclude_categories() { + global $CFG, $GLOSSARY_EXCLUDEENTRY; + $this->resetAfterTest(true); + + // Enable glossary filter at top level. + filter_set_global_state('glossary', TEXTFILTER_ON); + $CFG->glossary_linkentries = 1; + + // Create a test course. + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + // Create a glossary. + $glossary = $this->getDataGenerator()->create_module('glossary', + array('course' => $course->id, 'mainglossary' => 1)); + + // Create two entries with ampersands and one normal entry. + /** @var mod_glossary_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('mod_glossary'); + $category = $generator->create_category($glossary, array('name' => 'My category', 'usedynalink' => 1)); + + // Format text with all three entries in HTML. + $html = '

This is My category you know.

'; + $GLOSSARY_EXCLUDEENTRY = $category->id; + $filtered = format_text($html, FORMAT_HTML, array('context' => $context)); + $GLOSSARY_EXCLUDEENTRY = null; + + // Find all the glossary links in the result. + $matches = array(); + preg_match_all('~hook=([0-9]+).*?title="(.*?)"~', $filtered, $matches); + + // There should be 1 glossary link. + $this->assertEquals(1, count($matches[1])); + $this->assertEquals($category->id, $matches[1][0]); + $this->assertEquals($glossary->name . ': Category My category', $matches[2][0]); + } } From b0a3b52fe91a1808170523238e83bf15f243c377 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 11 Sep 2018 16:49:10 +0100 Subject: [PATCH 03/10] MDL-47962 filter_glossary: Fix lang string concatenation --- filter/glossary/filter.php | 11 +++++------ filter/glossary/lang/en/filter_glossary.php | 2 ++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/filter/glossary/filter.php b/filter/glossary/filter.php index 46c5f917bed..fc38ae1ab9a 100644 --- a/filter/glossary/filter.php +++ b/filter/glossary/filter.php @@ -85,15 +85,13 @@ class filter_glossary extends moodle_text_filter { return $this->cacheconceptlist; } - $strcategory = get_string('category', 'glossary'); - $conceptlist = array(); foreach ($allconcepts as $concepts) { foreach ($concepts as $concept) { if ($concept->category) { // Link to a category. - // TODO: Fix this string usage. - $title = $glossaries[$concept->glossaryid] . ': ' . $strcategory . ' ' . $concept->concept; + $title = get_string('glossarycategory', 'filter_glossary', + ['glossary' => $glossaries[$concept->glossaryid], 'category' => $concept->concept]); $link = new moodle_url('/mod/glossary/view.php', array('g' => $concept->glossaryid, 'mode' => 'cat', 'hook' => $concept->id)); $attributes = array( 'href' => $link, @@ -102,7 +100,8 @@ class filter_glossary extends moodle_text_filter { $conceptid = 0; } else { // Link to entry or alias - $title = $glossaries[$concept->glossaryid] . ': ' . $concept->concept; + $title = get_string('glossaryconcept', 'filter_glossary', + ['glossary' => $glossaries[$concept->glossaryid], 'concept' => $concept->concept]); // Hardcoding dictionary format in the URL rather than defaulting // to the current glossary format which may not work in a popup. // for example "entry list" means the popup would only contain @@ -122,7 +121,7 @@ class filter_glossary extends moodle_text_filter { $href_tag_begin = html_writer::start_tag('a', $attributes); $filterobj = new filterobject($concept->concept, $href_tag_begin, '', - $concept->casesensitive, $concept->fullmatch);; + $concept->casesensitive, $concept->fullmatch); $filterobj->conceptid = $conceptid; $conceptlist[] = $filterobj; } diff --git a/filter/glossary/lang/en/filter_glossary.php b/filter/glossary/lang/en/filter_glossary.php index 0f14c51d5fc..d7917afb25c 100644 --- a/filter/glossary/lang/en/filter_glossary.php +++ b/filter/glossary/lang/en/filter_glossary.php @@ -25,5 +25,7 @@ defined('MOODLE_INTERNAL') || die(); +$string['glossarycategory'] = '{$a->glossary}: Category {$a->category}'; +$string['glossaryconcept'] = '{$a->glossary}: {$a->concept}'; $string['filtername'] = 'Glossary auto-linking'; $string['privacy:metadata'] = 'The Glossary auto-linking plugin does not store any personal data.'; From d22699af7fbd94b26203d89e35dc42d59831df20 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 11 Sep 2018 17:36:29 +0100 Subject: [PATCH 04/10] MDL-47962 filter_glossary: cache concept list in a MUC static cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a slight improvement on a suggestion by David Monllaó --- filter/glossary/filter.php | 45 +++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/filter/glossary/filter.php b/filter/glossary/filter.php index fc38ae1ab9a..a3655708ea5 100644 --- a/filter/glossary/filter.php +++ b/filter/glossary/filter.php @@ -33,12 +33,8 @@ defined('MOODLE_INTERNAL') || die(); * NOTE: multilang glossary entries are not compatible with this filter. */ class filter_glossary extends moodle_text_filter { - /** @var int $cachecourseid cache invalidation flag in case content from multiple courses displayed. */ - protected $cachecourseid = null; - /** @var int $cacheuserid cache invalidation flag in case user is switched. */ - protected $cacheuserid = null; - /** @var array $cacheconceptlist page level filter cache, this should be always faster than MUC */ - protected $cacheconceptlist = null; + /** @var null|cache_store cache used to store the terms for this course. */ + protected $cache = null; public function setup($page, $context) { if ($page->requires->should_create_one_time_item_now('filter_glossary_autolinker')) { @@ -58,6 +54,10 @@ class filter_glossary extends moodle_text_filter { protected function get_all_concepts() { global $CFG, $USER; + if ($this->cache === null) { + $this->cache = cache::make_from_params(cache_store::MODE_REQUEST, 'filter', 'glossary'); + } + // Try to get current course. $coursectx = $this->context->get_course_context(false); if (!$coursectx) { @@ -67,22 +67,25 @@ class filter_glossary extends moodle_text_filter { $courseid = $coursectx->instanceid; } - if ($this->cachecourseid != $courseid or $this->cacheuserid != $USER->id) { + $cached = $this->cache->get('concepts'); + if ($cached !== false && ($cached->cachecourseid != $courseid || $cached->cacheuserid != $USER->id)) { // Invalidate the page cache. - $this->cacheconceptlist = null; + $cached = false; } - if (is_array($this->cacheconceptlist)) { - return $this->cacheconceptlist; + if ($cached !== false && is_array($cached->cacheconceptlist)) { + return $cached->cacheconceptlist; } list($glossaries, $allconcepts) = \mod_glossary\local\concept_cache::get_concepts($courseid); if (!$allconcepts) { - $this->cacheuserid = $USER->id; - $this->cachecourseid = $courseid; - $this->cacheconcepts = array(); - return $this->cacheconceptlist; + $tocache = new stdClass(); + $tocache->cacheuserid = $USER->id; + $tocache->cachecourseid = $courseid; + $tocache->cacheconceptlist = []; + $this->cache->set('concepts', $tocache); + return []; } $conceptlist = array(); @@ -127,12 +130,18 @@ class filter_glossary extends moodle_text_filter { } } + // We sort longest first, so that when we replace the terms, + // the longest ones are replaced first. This does the right thing + // when you have two terms like 'Moodle' and 'Moodle 3.5'. You want the longest match. usort($conceptlist, 'filter_glossary::sort_entries_by_length'); - $this->cacheuserid = $USER->id; - $this->cachecourseid = $courseid; - $this->cacheconceptlist = $conceptlist; - return $this->cacheconceptlist; + $tocache = new stdClass(); + $tocache->cacheuserid = $USER->id; + $tocache->cachecourseid = $courseid; + $tocache->cacheconceptlist = $conceptlist; + $this->cache->set('concepts', $tocache); + + return $conceptlist; } public function filter($text, array $options = array()) { From 2abf8fbf8683c28b66dc6e80774e3c236d656e8c Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 13 Sep 2018 17:08:28 +0100 Subject: [PATCH 05/10] MDL-47962 filter_glossary: use preg features for fullmatch This eliminates a lot of necessary string manipulation for something that preg can just do with the \b assertion. The change also extracts all the work to prepare ->work_phrase (renamed ->regexp) into a separate function. This is to pave the way for future efficiency gains, but for now I have not done them, so it is easier to verify by inspection that this commit does not change behaviour. Also, some tidy-ups to the filterobject structure, to eliminate some redundant fields. --- lib/filterlib.php | 173 ++++++++++++++++++++++------------------------ 1 file changed, 84 insertions(+), 89 deletions(-) diff --git a/lib/filterlib.php b/lib/filterlib.php index bd6f6d96cd8..ec4bd17a951 100644 --- a/lib/filterlib.php +++ b/lib/filterlib.php @@ -469,18 +469,21 @@ abstract class moodle_text_filter { class filterobject { /** @var string */ public $phrase; + /** @var bool whether to only recognise full word matches. */ + public $fullmatch; + public $hreftagbegin; public $hreftagend; /** @var bool */ public $casesensitive; - public $fullmatch; + + /** @var null|string once initialised, holds the regexp for matching this phrase. */ + public $workregexp = null; + /** @var mixed */ public $replacementphrase; - public $work_phrase; public $work_hreftagbegin; public $work_hreftagend; - public $work_casesensitive; - public $work_fullmatch; public $work_replacementphrase; /** @var bool */ public $work_calculated; @@ -504,8 +507,8 @@ class filterobject { $this->phrase = $phrase; $this->hreftagbegin = $hreftagbegin; $this->hreftagend = $hreftagend; - $this->casesensitive = $casesensitive; - $this->fullmatch = $fullmatch; + $this->casesensitive = !empty($casesensitive); + $this->fullmatch = !empty($fullmatch); $this->replacementphrase = $replacementphrase; $this->work_calculated = false; @@ -1240,7 +1243,7 @@ function filter_context_may_have_filter_settings($context) { * Process phrases intelligently found within a HTML text (such as adding links). * * @param string $text the text that we are filtering - * @param array $linkarray an array of filterobjects + * @param filterobject[] $linkarray an array of filterobjects * @param array $ignoretagsopen an array of opening tags that we should ignore while filtering * @param array $ignoretagsclose an array of corresponding closing tags * @param bool $overridedefaultignore True to only use tags provided by arguments @@ -1251,11 +1254,15 @@ function filter_phrases($text, $linkarray, $ignoretagsopen = null, $ignoretagscl global $CFG; - static $usedphrases; + // Used if $CFG->filtermatchoneperpage is on. Array with keys being the workregexp + // for things that have already been matched on this page. + static $usedphrases = []; $ignoretags = array(); // To store all the enclosig tags to be completely ignored. $tags = array(); // To store all the simple tags to be ignored. + $linkarray = filter_prepare_phrases_for_filtering($linkarray); + if (!$overridedefaultignore) { // A list of open/close tags that we should not replace within. // Extended to include