Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-8713

Big display problems when filterall is enabled

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.8
    • Fix Version/s: 1.8
    • Component/s: Filters
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE

      Description

      When filterall is enabled and some autolinking filter (glossary, activity name) is also active, big display problems occur (see screenshot attached): HTML code displays in the window, tabs are broken, bad links are shown in breadcrumb, etc.

        Gliffy Diagrams

        1. display_problems.png
          59 kB
        2. display_problems.png
          59 kB
        3. display-OK.png
          38 kB
        4. Some progress....jpg
          57 kB

          Activity

          Hide
          mina Nicolas Martignoni added a comment -

          Here is the screenshot showing how it displays with filterall disabled.

          Show
          mina Nicolas Martignoni added a comment - Here is the screenshot showing how it displays with filterall disabled.
          Hide
          mina Nicolas Martignoni added a comment -

          Any progress on this?

          This is very very annoying on multilingual sites

          Show
          mina Nicolas Martignoni added a comment - Any progress on this? This is very very annoying on multilingual sites
          Hide
          dougiamas Martin Dougiamas added a comment -

          Nick C can you look into this? I wonder if it's related to the changes from a couple weeks ago.

          Show
          dougiamas Martin Dougiamas added a comment - Nick C can you look into this? I wonder if it's related to the changes from a couple weeks ago.
          Hide
          dougiamas Martin Dougiamas added a comment -

          Actually, sorry, the problem is actually just the filters that insert links. We need to skip them.

          We can do this by:

          1) Defining a temporary global like $CFG->formatstring like this in format_string()

          if (!empty($CFG->filterall))

          { $CFG->formatstring = true; // new $string = filter_text($string, $courseid); unset($CFG->formatstring); // new }

          2) Adding this to the beginning of all the filters which create links:

          global $CFG;
          if (!empty($CFG->formatstring))

          { return $text; }

          Ping me if there's any questions!

          Show
          dougiamas Martin Dougiamas added a comment - Actually, sorry, the problem is actually just the filters that insert links. We need to skip them. We can do this by: 1) Defining a temporary global like $CFG->formatstring like this in format_string() if (!empty($CFG->filterall)) { $CFG->formatstring = true; // new $string = filter_text($string, $courseid); unset($CFG->formatstring); // new } 2) Adding this to the beginning of all the filters which create links: global $CFG; if (!empty($CFG->formatstring)) { return $text; } Ping me if there's any questions!
          Hide
          skodak Petr Skoda added a comment -

          It might be IMHO better to add new function filter_string() and new function into filters xx_filter_string() because some filters are not appropriate for strings (such as hotlinking).

          I am not sure if any tags should be allowed in formatted strings by default, maybe changing the default from "$striplinks = false" to "$stripanytags = true" (the new formslib strips all tags except multilang).

          Show
          skodak Petr Skoda added a comment - It might be IMHO better to add new function filter_string() and new function into filters xx_filter_string() because some filters are not appropriate for strings (such as hotlinking). I am not sure if any tags should be allowed in formatted strings by default, maybe changing the default from "$striplinks = false" to "$stripanytags = true" (the new formslib strips all tags except multilang).
          Hide
          nicolasconnault Nicolas Connault added a comment -

          Correct me if I'm wrong:

          I looked in the filter/ directory for filters that need this edit (made sense to me), and found only one that output straight links: filter/emailprotect/filter.php

          So I'm probably missing something. Also I have no idea how to reproduce this error. Could someone please outline the steps needed?
          Thanks!

          Show
          nicolasconnault Nicolas Connault added a comment - Correct me if I'm wrong: I looked in the filter/ directory for filters that need this edit (made sense to me), and found only one that output straight links: filter/emailprotect/filter.php So I'm probably missing something. Also I have no idea how to reproduce this error. Could someone please outline the steps needed? Thanks!
          Hide
          dougiamas Martin Dougiamas added a comment -

          Thinking about it, a whitelist is a good idea. I don't think we need new functions for all the filters though.

          So how about if format_string doesn't call filter_text, but instead calls a new filter_string() as Petr suggested.

          This function, however, just calls certain known filters on a whitelist. I'll check this in now, see code shortly ...

          Show
          dougiamas Martin Dougiamas added a comment - Thinking about it, a whitelist is a good idea. I don't think we need new functions for all the filters though. So how about if format_string doesn't call filter_text, but instead calls a new filter_string() as Petr suggested. This function, however, just calls certain known filters on a whitelist. I'll check this in now, see code shortly ...
          Hide
          dougiamas Martin Dougiamas added a comment -

          Nicolas M, can you please test this out and make sure it's better on your site?

          Show
          dougiamas Martin Dougiamas added a comment - Nicolas M, can you please test this out and make sure it's better on your site?
          Hide
          mina Nicolas Martignoni added a comment -

          Martin, there is some progress, but not everything is OK (see screenshot).

          Show
          mina Nicolas Martignoni added a comment - Martin, there is some progress, but not everything is OK (see screenshot).
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Uhm... too late I saw this,

          isn't the $striplinks parameter in format_string() enough to prevent link broken and to return "pure" strings? It has been used in the activity menu and other places since ages....

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Uhm... too late I saw this, isn't the $striplinks parameter in format_string() enough to prevent link broken and to return "pure" strings? It has been used in the activity menu and other places since ages.... Ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I'm reopening this because, although working, there are some important thoughts we have been discussing on Skype. Let's summarise them:

          0) Latest change of the format_string() function seems to be slightly incorrect: Caching is broken & filter functions are called twice.

          All this ignited one long discussion in chat about how things were going to be handled in 1.8 and finally we agreed that this could be a clear path to follow:

          1) For activity names, is order to allow pre-existing ones to display html tags, global switch controls is we allow tags or no. We use complete format_string() to display them in course page.

          2) For course/site titles we only allow <span> and process by format_string() function.

          3) All we (developers/testers) create a course named #@#@#.... to detect where format_string is missing. format_string() will strip it so it'll be easy to detect where we are missing the format_string() function.

          4/ rewrite format_string() to strip all tags by default. It's supposed that only non-html text is going to be processed by it.

          5/ format_string() apply only SAFE filters. Those safe filters will be defined inside each filter by checking results of one "xxxx_filter_string()" (true/false) function (instead of the current $CFG->stringfilters new global. Only enabled filters being safe will be processed by format_string.

          This should be decided ASAP. Should help to clarify things definitively.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I'm reopening this because, although working, there are some important thoughts we have been discussing on Skype. Let's summarise them: 0) Latest change of the format_string() function seems to be slightly incorrect: Caching is broken & filter functions are called twice. All this ignited one long discussion in chat about how things were going to be handled in 1.8 and finally we agreed that this could be a clear path to follow: 1) For activity names, is order to allow pre-existing ones to display html tags, global switch controls is we allow tags or no. We use complete format_string() to display them in course page. 2) For course/site titles we only allow <span> and process by format_string() function. 3) All we (developers/testers) create a course named #@#@#.... to detect where format_string is missing. format_string() will strip it so it'll be easy to detect where we are missing the format_string() function. 4/ rewrite format_string() to strip all tags by default. It's supposed that only non-html text is going to be processed by it. 5/ format_string() apply only SAFE filters. Those safe filters will be defined inside each filter by checking results of one "xxxx_filter_string()" (true/false) function (instead of the current $CFG->stringfilters new global. Only enabled filters being safe will be processed by format_string. This should be decided ASAP. Should help to clarify things definitively. Ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Correction! In 1) above it should say We use complete format_text() to display them in course page. !

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Correction! In 1) above it should say We use complete format_text() to display them in course page. !
          Hide
          dougiamas Martin Dougiamas added a comment -

          Some replies:

          > Latest change of the format_string() function seems to be slightly incorrect: Caching is broken & filter functions are called twice.

          Ah, whoops, that is a simple mistake ... I had modified Nick's modification without noticing that he had duplicated the call to filter_text!

          http://moodle.cvs.sourceforge.net/moodle/moodle/lib/weblib.php?r1=1.812.2.30&r2=1.812.2.31

          Now fixed in CVS.

          I'll go through and add $striplinks to more of the format_string calls too, to fix Nicolas M's remaining problem.

          > 1) For activity names, is order to allow pre-existing ones to display html tags, global switch controls is we allow tags or no. We use complete format_text() to display them in course page.

          OK, perhaps this can just override $striplinks == false in format_string ... $CFG->forcestriplinksinstrings

          I don't really understand some of the other things though ... we need to keep tags because of multilang. And the site admin right now has complete control over what filters get applied to strings (by setting $CFG->stringfilters) ... I thought later we could add this to same GUI that defines $CFG->textfilters. If this CFG is not defined, then ONLY multilang gets applied by default, and even that ONLY if multilang was enabled for filters in general.

          Show
          dougiamas Martin Dougiamas added a comment - Some replies: > Latest change of the format_string() function seems to be slightly incorrect: Caching is broken & filter functions are called twice. Ah, whoops, that is a simple mistake ... I had modified Nick's modification without noticing that he had duplicated the call to filter_text! http://moodle.cvs.sourceforge.net/moodle/moodle/lib/weblib.php?r1=1.812.2.30&r2=1.812.2.31 Now fixed in CVS. I'll go through and add $striplinks to more of the format_string calls too, to fix Nicolas M's remaining problem. > 1) For activity names, is order to allow pre-existing ones to display html tags, global switch controls is we allow tags or no. We use complete format_text() to display them in course page. OK, perhaps this can just override $striplinks == false in format_string ... $CFG->forcestriplinksinstrings I don't really understand some of the other things though ... we need to keep tags because of multilang. And the site admin right now has complete control over what filters get applied to strings (by setting $CFG->stringfilters) ... I thought later we could add this to same GUI that defines $CFG->textfilters. If this CFG is not defined, then ONLY multilang gets applied by default, and even that ONLY if multilang was enabled for filters in general.
          Hide
          dougiamas Martin Dougiamas added a comment -

          Actually, thinking about it, just changing the default for $striplinks in format_string from false to true should have the desired effect ...

          Nicolas, can you try your test site now on latest 1.8?

          Show
          dougiamas Martin Dougiamas added a comment - Actually, thinking about it, just changing the default for $striplinks in format_string from false to true should have the desired effect ... Nicolas, can you try your test site now on latest 1.8?
          Hide
          dougiamas Martin Dougiamas added a comment -

          I believe this should be resolved now.

          Show
          dougiamas Martin Dougiamas added a comment - I believe this should be resolved now.
          Hide
          mina Nicolas Martignoni added a comment -

          A BIG thank you! This is indeed fixed and verified. Closing.

          Show
          mina Nicolas Martignoni added a comment - A BIG thank you! This is indeed fixed and verified. Closing.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                31/Mar/07