Moodle
  1. Moodle
  2. MDL-8713

Big display problems when filterall is enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker 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
    • Rank:
      29074

      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.

      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
        Nicolas Martignoni added a comment -

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

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

        Any progress on this?

        This is very very annoying on multilingual sites

        Show
        Nicolas Martignoni added a comment - Any progress on this? This is very very annoying on multilingual sites
        Hide
        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
        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
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        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
        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
        Martin Dougiamas added a comment -

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

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

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

        Show
        Nicolas Martignoni added a comment - Martin, there is some progress, but not everything is OK (see screenshot).
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Martin Dougiamas added a comment -

        I believe this should be resolved now.

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

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

        Show
        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: