Moodle
  1. Moodle
  2. MDL-8582

Ampersand in Course Name/Shortname Breaks Strict

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8
    • Fix Version/s: 1.8
    • Component/s: Accessibility
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE
    • Rank:
      29138

      Description

      If you create a course with the title 'Design & Technology' and shortname 'D&T', then it'll break Strict in many places all over moodle (list of courses, course page etc)

      1. 28-02-07_8582.patch
        15 kB
        Nicolas Connault
      2. amp.patch
        6 kB
        Petr Škoda
      3. course_category.patch
        0.6 kB
        Dan Poltawski
      4. danp_course_lib.patch
        1 kB
        Dan Poltawski
      5. danp_revised_course_lib.patch
        4 kB
        Dan Poltawski
      6. navigation.patch
        3 kB
        Petr Škoda
      7. nick_last_ampersand.patch
        14 kB
        Nicolas Connault
      8. nick_path.diff
        30 kB
        Nicolas Connault
      9. nick_path.diff
        90 kB
        Nicolas Connault
      10. testweblib.php
        2 kB
        Nicolas Connault

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          the same goes for sitename

          Show
          Dan Poltawski added a comment - the same goes for sitename
          Hide
          Martin Dougiamas added a comment -

          Have a try of this, Nicolas.

          Show
          Martin Dougiamas added a comment - Have a try of this, Nicolas.
          Hide
          Nicolas Connault added a comment -

          Resolved:
          1. Used the weblib::s() function to clean up site, category and course name in xhtml output
          2. Extended weblib::s() to do more thorough cleanup (especially ampersand)
          3. Wrote a set of unit tests for weblib::s() (lib/simpletest/testweblib.php). If there is a centralised way to run these tests, please show me, since there is no documentation on unit tests on the Moodle wiki. This file runs itself in the browser at the moment.
          4. Edited the following files, adding calls to weblib::s() wherever applicable:
          a. course/lib.php
          b. course/index.php
          c. blocks/course_list/block_course_list.php
          d. lib/weblib.php::print_navigation()
          e. All the header.html templates in the themes
          f. user/view.php
          g. blocks/admin/block_admin.php

          Unfortunately I don't currently have CVS write privileges to any of these files

          Show
          Nicolas Connault added a comment - Resolved: 1. Used the weblib::s() function to clean up site, category and course name in xhtml output 2. Extended weblib::s() to do more thorough cleanup (especially ampersand) 3. Wrote a set of unit tests for weblib::s() (lib/simpletest/testweblib.php). If there is a centralised way to run these tests, please show me, since there is no documentation on unit tests on the Moodle wiki. This file runs itself in the browser at the moment. 4. Edited the following files, adding calls to weblib::s() wherever applicable: a. course/lib.php b. course/index.php c. blocks/course_list/block_course_list.php d. lib/weblib.php::print_navigation() e. All the header.html templates in the themes f. user/view.php g. blocks/admin/block_admin.php Unfortunately I don't currently have CVS write privileges to any of these files
          Hide
          Nicolas Connault added a comment -

          This is an RCS diff patch file generated by CVS diff -c -n.
          I'm attaching this because I can't yet commit my changes due to "insufficient karma".
          Maybe I need to meditate for a while, or do some good deeds...

          The only file I've created and is scheduled to be added through a commit is /lib/simpletest/testweblib.php. This is just a unit test so it isn't part of any dependencies.

          Show
          Nicolas Connault added a comment - This is an RCS diff patch file generated by CVS diff -c -n. I'm attaching this because I can't yet commit my changes due to "insufficient karma". Maybe I need to meditate for a while, or do some good deeds... The only file I've created and is scheduled to be added through a commit is /lib/simpletest/testweblib.php. This is just a unit test so it isn't part of any dependencies.
          Hide
          Nicolas Connault added a comment -

          One more comment:
          I initially thought about storing the html entities in the Database, to avoid having to patch a large number of files throughout Moodle which output xhtml to the browser. However, this workaround only adds to the problem: all the parts of Moodle that use that Data without outputting it to XHTML would end up with unwanted html entities which would then have to be cleanup/converted. I always prefer storing data in the database in as pure a form as possible. The following code example shows a possible solution using simple OOP:

          $category->name('html');
          instead of
          $category->name

          The name, shortname and fullname variables are often used (for site, course, category etc...) with the stdClass. if the stdClass was a proper class (with method definitions), it could become responsible for its own output methods.

          class moodleClass {
          var $name;

          function name($output)

          { return $this->$output($this->name); }

          function html($string)

          { // apply formatting return $string; }

          }

          With PHP5's reflection, you could even generate these methods during runtime, which means you could still keep using stdClass with arbitrary variables. Once you have initialised these variables, you would add the methods through reflections.

          Show
          Nicolas Connault added a comment - One more comment: I initially thought about storing the html entities in the Database, to avoid having to patch a large number of files throughout Moodle which output xhtml to the browser. However, this workaround only adds to the problem: all the parts of Moodle that use that Data without outputting it to XHTML would end up with unwanted html entities which would then have to be cleanup/converted. I always prefer storing data in the database in as pure a form as possible. The following code example shows a possible solution using simple OOP: $category->name('html'); instead of $category->name The name, shortname and fullname variables are often used (for site, course, category etc...) with the stdClass. if the stdClass was a proper class (with method definitions), it could become responsible for its own output methods. class moodleClass { var $name; function name($output) { return $this->$output($this->name); } function html($string) { // apply formatting return $string; } } With PHP5's reflection, you could even generate these methods during runtime, which means you could still keep using stdClass with arbitrary variables. Once you have initialised these variables, you would add the methods through reflections.
          Hide
          Dan Poltawski added a comment -

          Martin, I wonder if you or somebody could review Nicolas' changes.

          thanks

          Show
          Dan Poltawski added a comment - Martin, I wonder if you or somebody could review Nicolas' changes. thanks
          Hide
          Martin Dougiamas added a comment -

          Wow, Nick, that was fast! Uploading the diff was what I wanted in this case because you don't have write privs yet.

          I'm just looking at it now.

          Show
          Martin Dougiamas added a comment - Wow, Nick, that was fast! Uploading the diff was what I wanted in this case because you don't have write privs yet. I'm just looking at it now.
          Hide
          Martin Dougiamas added a comment -

          The first thing I notice is that the diff has a lot of lines in it that are not actually changed ... is your editor saving in "dos" text format (CRLF) by any chance? (we use "unix" format, just LF).

          Show
          Martin Dougiamas added a comment - The first thing I notice is that the diff has a lot of lines in it that are not actually changed ... is your editor saving in "dos" text format (CRLF) by any chance? (we use "unix" format, just LF).
          Hide
          Nicolas Connault added a comment -

          Sorry about that, I will generate a new diff as soon as I find out how to change this in Eclipse. Somehow I always assumed it was saving in Unix format. I guess assumptions are dangerous

          Show
          Nicolas Connault added a comment - Sorry about that, I will generate a new diff as soon as I find out how to change this in Eclipse. Somehow I always assumed it was saving in Unix format. I guess assumptions are dangerous
          Hide
          Nicolas Connault added a comment -

          Ok I found out what happens. I am outputting in Unix format just fine. The issue is that my editor trims empty whitespace. See the following diff for an example:

          ? nick_path.diff
          Index: view.php
          ===================================================================
          RCS file: /cvsroot/moodle/moodle/user/view.php,v
          retrieving revision 1.143.2.1
          diff -n -c -r1.143.2.1 view.php

              • view.php 21 Feb 2007 21:57:10 -0000 1.143.2.1
              • view.php 22 Feb 2007 12:47:13 -0000
                ***************
              • 1,7 ****
                <?PHP // $Id: view.php,v 1.143.2.1 2007/02/21 21:57:10 skodak Exp $

          // Display profile for a particular user
          !
          require_once("../config.php");
          $id = optional_param('id', 0, PARAM_INT); // user id
          $course = optional_param('course', SITEID, PARAM_INT); // course id (defaults to Site)
          — 1,7 ----
          <?PHP // $Id: view.php,v 1.143.2.1 2007/02/21 21:57:10 skodak Exp $

          // Display profile for a particular user
          !
          require_once("../config.php");
          $id = optional_param('id', 0, PARAM_INT); // user id
          $course = optional_param('course', SITEID, PARAM_INT); // course id (defaults to Site)
          ***************

          The lines with the exclamation marks refer to places where the original code has blank space characters which got trimmed by my editor.

          I can switch that off in Eclipse if you like, since it's likely that each time I make a commit it will end up making these kinds of edits. Or maybe there's a way in CVS to ignore differences in whitespace. I'll look it up.

          Show
          Nicolas Connault added a comment - Ok I found out what happens. I am outputting in Unix format just fine. The issue is that my editor trims empty whitespace. See the following diff for an example: ? nick_path.diff Index: view.php =================================================================== RCS file: /cvsroot/moodle/moodle/user/view.php,v retrieving revision 1.143.2.1 diff -n -c -r1.143.2.1 view.php view.php 21 Feb 2007 21:57:10 -0000 1.143.2.1 view.php 22 Feb 2007 12:47:13 -0000 *************** 1,7 **** <?PHP // $Id: view.php,v 1.143.2.1 2007/02/21 21:57:10 skodak Exp $ // Display profile for a particular user ! require_once("../config.php"); $id = optional_param('id', 0, PARAM_INT); // user id $course = optional_param('course', SITEID, PARAM_INT); // course id (defaults to Site) — 1,7 ---- <?PHP // $Id: view.php,v 1.143.2.1 2007/02/21 21:57:10 skodak Exp $ // Display profile for a particular user ! require_once("../config.php"); $id = optional_param('id', 0, PARAM_INT); // user id $course = optional_param('course', SITEID, PARAM_INT); // course id (defaults to Site) *************** The lines with the exclamation marks refer to places where the original code has blank space characters which got trimmed by my editor. I can switch that off in Eclipse if you like, since it's likely that each time I make a commit it will end up making these kinds of edits. Or maybe there's a way in CVS to ignore differences in whitespace. I'll look it up.
          Hide
          Nicolas Connault added a comment -

          Ok here's the patch file again, this time output using

          cvs diff -n -c -b -B > nick_path.diff

          thus ignoring trailing white space and blank line deletes/adds

          The resulting file looks good to me.

          Show
          Nicolas Connault added a comment - Ok here's the patch file again, this time output using cvs diff -n -c -b -B > nick_path.diff thus ignoring trailing white space and blank line deletes/adds The resulting file looks good to me.
          Hide
          Martin Dougiamas added a comment -

          OK, thanks, Nick.

          There's some issues in this case where the change has been made to all the theme headers, where it would make more sense to fix it just once in print_header. Also, s() is being called more than once on the same string in some places, eg from print_header_simple().

          Yu's fixing those and getting it into CVS, though. See you Monday!

          Show
          Martin Dougiamas added a comment - OK, thanks, Nick. There's some issues in this case where the change has been made to all the theme headers, where it would make more sense to fix it just once in print_header. Also, s() is being called more than once on the same string in some places, eg from print_header_simple(). Yu's fixing those and getting it into CVS, though. See you Monday!
          Hide
          Petr Škoda added a comment -

          Hi, why did you deprecate addslashes_js() ? If there is a better alternative please document in the code why it is depracated and what to use instead, thanks.

          Show
          Petr Škoda added a comment - Hi, why did you deprecate addslashes_js() ? If there is a better alternative please document in the code why it is depracated and what to use instead, thanks.
          Hide
          Nicolas Connault added a comment -

          Hi Petr, sorry I misread the comment above it, I thought it referred to the function itself. The @deprecated tag can go.

          Show
          Nicolas Connault added a comment - Hi Petr, sorry I misread the comment above it, I thought it referred to the function itself. The @deprecated tag can go.
          Hide
          Petr Škoda added a comment -

          Could you please also explain the changes in s() function?
          1/ why the change in regex? is it doing something else, if yes why?
          2/ why the new function parameter? we should always fix inline docs when adding params to core functions

          Show
          Petr Škoda added a comment - Could you please also explain the changes in s() function? 1/ why the change in regex? is it doing something else, if yes why? 2/ why the new function parameter? we should always fix inline docs when adding params to core functions
          Hide
          Petr Škoda added a comment -

          hi Nicolas, do you use skype? we could discuss the s() function, my nick is petr.skoda

          Show
          Petr Škoda added a comment - hi Nicolas, do you use skype? we could discuss the s() function, my nick is petr.skoda
          Hide
          Nicolas Connault added a comment -

          I used to use Skype (nicolasconnault) but took it out because of two main reasons: rumours of spyware and simply no one to talk to However I use Trillian and have an account for the following IM services:

          ICQ: 826611
          AIM: NikoZeta
          MSN: nikozeta@hotmail.com
          Yahoo: nicolasconnault

          But if you want to chat (vocally) on Skype, why not just use the telephone? Let me know if you would still prefer Skype and I will reinstall it. But since I am starting work at the office on Monday, communication probably won't be an issue.

          Show
          Nicolas Connault added a comment - I used to use Skype (nicolasconnault) but took it out because of two main reasons: rumours of spyware and simply no one to talk to However I use Trillian and have an account for the following IM services: ICQ: 826611 AIM: NikoZeta MSN: nikozeta@hotmail.com Yahoo: nicolasconnault But if you want to chat (vocally) on Skype, why not just use the telephone? Let me know if you would still prefer Skype and I will reinstall it. But since I am starting work at the office on Monday, communication probably won't be an issue.
          Hide
          Nicolas Connault added a comment -

          To save in lib/simpletest

          Show
          Nicolas Connault added a comment - To save in lib/simpletest
          Hide
          Dan Poltawski added a comment -

          Oh, ampersands in course categories also break strict. Here are two lines missed from course/lib.php

          Show
          Dan Poltawski added a comment - Oh, ampersands in course categories also break strict. Here are two lines missed from course/lib.php
          Hide
          Dan Poltawski added a comment -

          Also, ':' isn't being caught
          (change your test course name to 'D&T: How to break things' and see)

          Show
          Dan Poltawski added a comment - Also, ':' isn't being caught (change your test course name to 'D&T: How to break things' and see)
          Hide
          Dan Poltawski added a comment -

          Hmm sorry, ':' is perfectly valid, just firefox doesn't like it when presented as xml

          Show
          Dan Poltawski added a comment - Hmm sorry, ':' is perfectly valid, just firefox doesn't like it when presented as xml
          Hide
          Dan Poltawski added a comment -

          Lots of places in course/lib.php which need s()

          Show
          Dan Poltawski added a comment - Lots of places in course/lib.php which need s()
          Hide
          Dan Poltawski added a comment -

          Attached are some places in course/category.php missing

          Show
          Dan Poltawski added a comment - Attached are some places in course/category.php missing
          Hide
          Petr Škoda added a comment -

          We have discussed it a bit with Nick via ICQ - the result is new function format_title() and reverting to original s(). The reasons were:
          1/ proper handling of 𛙕 entities
          2/ security - both s() and format_title() now accept untrusted content and return safe html or plaintext

          The $plaintext parameter should be used for activity titles (where we always 'unofficially' supported html), the rest such as course title, course shortname, forum posts titles, etc. should be IMO expecting plain text only.

          TODO:

          • fix all titles
          • add detection of lonely < and > and convert them to html entities (low priority, the lonely & was much bigger problem)
          Show
          Petr Škoda added a comment - We have discussed it a bit with Nick via ICQ - the result is new function format_title() and reverting to original s(). The reasons were: 1/ proper handling of 𛙕 entities 2/ security - both s() and format_title() now accept untrusted content and return safe html or plaintext The $plaintext parameter should be used for activity titles (where we always 'unofficially' supported html), the rest such as course title, course shortname, forum posts titles, etc. should be IMO expecting plain text only. TODO: fix all titles add detection of lonely < and > and convert them to html entities (low priority, the lonely & was much bigger problem)
          Hide
          Nicolas Connault added a comment -

          Now using the new format_title function. However, one more issue is the outputting of the course name and shortname in the breadcrumbs: the <a> tags are part of the $navigation variable that gets cleaned up by either s() or format_title(). At the moment I have disabled format_title()'s removing of <a> tags.

          Show
          Nicolas Connault added a comment - Now using the new format_title function. However, one more issue is the outputting of the course name and shortname in the breadcrumbs: the <a> tags are part of the $navigation variable that gets cleaned up by either s() or format_title(). At the moment I have disabled format_title()'s removing of <a> tags.
          Hide
          Petr Škoda added a comment -

          The link removal is needed to cleanup the links created by some filters, maybe we could add new filter settings to enable only some filters in filter_string() and filter_title().

          Show
          Petr Škoda added a comment - The link removal is needed to cleanup the links created by some filters, maybe we could add new filter settings to enable only some filters in filter_string() and filter_title().
          Hide
          Petr Škoda added a comment -

          Looking at the code it might be better to change the way breadcrums are passed to the print_header() function - simple array 'title'=>url would be easier to construct in scripts and much easier to process in print_header

          Show
          Petr Škoda added a comment - Looking at the code it might be better to change the way breadcrums are passed to the print_header() function - simple array 'title'=>url would be easier to construct in scripts and much easier to process in print_header
          Hide
          Petr Škoda added a comment -

          The more I think about this the harder it seems:

          Please also consider the fact that for security reasons we 'clean' the submitted titles (==remove dangerous js) before the storage into database and we always considered the titles to be html, NOT plaintext. The patches above break existing data in database.

          There is also the problem with multilang titles, it was allowed nearly everywhere except the site name, shortname and course name AFAIK.

          IMO we have to decide on the format of text used in titles:
          1/ plaintext - then we can use current print_title() in cvs; this option breaks existing content; no way to type "less than" or "greater than" characters in titles
          2/ html - current standard with PARAM_MULTILANG cleaning; & must be entered as html entity & amp ; - we would not need format_title(), instead format_string() and sometimes s(format_string()) could do the job

          Please note that since conversion to new forms html is not allowed in activity titles and course anymore - ok for me, but I am sure MD will not like it

          I am attaching a patch with print_navigation that support also array navigation parameter and uses print_title()

          my +1 for 2/ - only fixing the multilang issue and educating people about html entities

          Show
          Petr Škoda added a comment - The more I think about this the harder it seems: Please also consider the fact that for security reasons we 'clean' the submitted titles (==remove dangerous js) before the storage into database and we always considered the titles to be html, NOT plaintext. The patches above break existing data in database. There is also the problem with multilang titles, it was allowed nearly everywhere except the site name, shortname and course name AFAIK. IMO we have to decide on the format of text used in titles: 1/ plaintext - then we can use current print_title() in cvs; this option breaks existing content; no way to type "less than" or "greater than" characters in titles 2/ html - current standard with PARAM_MULTILANG cleaning; & must be entered as html entity & amp ; - we would not need format_title(), instead format_string() and sometimes s(format_string()) could do the job Please note that since conversion to new forms html is not allowed in activity titles and course anymore - ok for me, but I am sure MD will not like it I am attaching a patch with print_navigation that support also array navigation parameter and uses print_title() my +1 for 2/ - only fixing the multilang issue and educating people about html entities
          Hide
          Petr Škoda added a comment -

          We should IMO discuss this on Monday with MD in HQ chat.

          The solution should not be difficult when we decide which way to go...

          Show
          Petr Škoda added a comment - We should IMO discuss this on Monday with MD in HQ chat. The solution should not be difficult when we decide which way to go...
          Hide
          Petr Škoda added a comment -

          ... if we want to fix only the & char, we can also add the "detection and replacement of lonely ampersands" into form submission, it could also detect the "lonely < and >" and tweak them too before the cleaning

          Show
          Petr Škoda added a comment - ... if we want to fix only the & char, we can also add the "detection and replacement of lonely ampersands" into form submission, it could also detect the "lonely < and >" and tweak them too before the cleaning
          Hide
          Petr Škoda added a comment -

          We might also use htmltidy to clean other submitted HTML to get xhtml strict everywhere - it would be too costly to do that before each display, but before the storage it should not be a big problem.

          Show
          Petr Škoda added a comment - We might also use htmltidy to clean other submitted HTML to get xhtml strict everywhere - it would be too costly to do that before each display, but before the storage it should not be a big problem.
          Hide
          Nicolas Connault added a comment -

          Petr, I'm on ICQ if you want to chat now.

          Show
          Nicolas Connault added a comment - Petr, I'm on ICQ if you want to chat now.
          Hide
          Nicolas Connault added a comment -

          So the main issue is at which point to clean up the user input: before DB storage or after DB retrieval. I think it's a good overall idea to do it before storage, but different inputs need different cleanups. As you pointed out, we want links for some data and plain text for others. Some require some input to convert into "sep" divs (for breadcrumbs). That cleanup should occur in a central location, not scattered among a bunch of unrelated functions. What is considered "harmful" should be clearly defined and conceptualised before being implemented in code.

          Also, we are trying to achieve both clean XHTML strict AND security. These two aspects aren't necessarily dependent or coexistent with each other: we can have code that is clean but unsafe, and have safe but dirty code. This is why I think that cleaning up user input before storage in DB should have security as a goal, while tidying up for clean XHTML can be performed during retrieval, depending on the use made of that data. Sometimes that data is not used within XHTML output at all...

          Show
          Nicolas Connault added a comment - So the main issue is at which point to clean up the user input: before DB storage or after DB retrieval. I think it's a good overall idea to do it before storage, but different inputs need different cleanups. As you pointed out, we want links for some data and plain text for others. Some require some input to convert into "sep" divs (for breadcrumbs). That cleanup should occur in a central location, not scattered among a bunch of unrelated functions. What is considered "harmful" should be clearly defined and conceptualised before being implemented in code. Also, we are trying to achieve both clean XHTML strict AND security. These two aspects aren't necessarily dependent or coexistent with each other: we can have code that is clean but unsafe, and have safe but dirty code. This is why I think that cleaning up user input before storage in DB should have security as a goal, while tidying up for clean XHTML can be performed during retrieval, depending on the use made of that data. Sometimes that data is not used within XHTML output at all...
          Hide
          Petr Škoda added a comment -

          The problem with cleanup is that we do not always storing data in final html format in db, the cleanup must be done after the last html processing. In general we store titles cleaned up in db while general text in raw untrusted form (and of course there is one ugly exception called trusttext).

          What I am proposing here is to use html tidy filtering in forms before the data storage - this would solve this problem and could make user submitted html much nicer.

          Anyway I have a working patch that needs some cleanup and exception handling for our custom tags. I am running to pub now to meet a team of female ice hockey players :-D

          I am very glad you started with this hard bug that is related to our security mechanisms, tomorrow I will search for all my unfinished documents/conference papers, etc. related to security and mail it to you There should be also some recordings available online.

          skodak

          Show
          Petr Škoda added a comment - The problem with cleanup is that we do not always storing data in final html format in db, the cleanup must be done after the last html processing. In general we store titles cleaned up in db while general text in raw untrusted form (and of course there is one ugly exception called trusttext). What I am proposing here is to use html tidy filtering in forms before the data storage - this would solve this problem and could make user submitted html much nicer. Anyway I have a working patch that needs some cleanup and exception handling for our custom tags. I am running to pub now to meet a team of female ice hockey players :-D I am very glad you started with this hard bug that is related to our security mechanisms, tomorrow I will search for all my unfinished documents/conference papers, etc. related to security and mail it to you There should be also some recordings available online. skodak
          Hide
          Dan Poltawski added a comment -

          Ooh, I wouldn't mind seeing those!

          Show
          Dan Poltawski added a comment - Ooh, I wouldn't mind seeing those!
          Hide
          Petr Škoda added a comment -

          That should not be a big problem, they want me to help them setup a web site - I will let you know when it is online

          Show
          Petr Škoda added a comment - That should not be a big problem, they want me to help them setup a web site - I will let you know when it is online
          Hide
          Dan Poltawski added a comment -

          The security documents!

          Show
          Dan Poltawski added a comment - The security documents!
          Hide
          Martin Dougiamas added a comment -

          Wow, this was supposed to be a simple bug for Nicolas to start with. :-D

          Yes, let's meet up Monday at HQ and talk about it.

          Show
          Martin Dougiamas added a comment - Wow, this was supposed to be a simple bug for Nicolas to start with. :-D Yes, let's meet up Monday at HQ and talk about it.
          Hide
          Nicolas Connault added a comment -

          Issue fixed, under review by QA before committing.

          Show
          Nicolas Connault added a comment - Issue fixed, under review by QA before committing.
          Hide
          Dan Poltawski added a comment -

          Which is the proposed fixed then?

          Show
          Dan Poltawski added a comment - Which is the proposed fixed then?
          Hide
          Nicolas Connault added a comment -

          Patch under review

          Show
          Nicolas Connault added a comment - Patch under review
          Hide
          Nicolas Connault added a comment -

          Not sure if the hotpot module is converting ampersands correctly. $heading and $title are set up differently than in other modules:
          instead of outputting $course->title, a temporary $title variable is created and assigned that value. That made it difficult for
          me to decide whether it was already formatted elsewhere or not.

          Also I didn't format_string() any of the variables included in $navigation strings, because I assume these strings get processed by
          print_navigation(), which includes format_string().

          It would be cool if a given string could contain information about which filters have been applied to it. So a String would be an
          object with a simple array used as a stack, and a few methods. When you apply a new filter to a string, it would automatically
          (and optionally) self-check and only apply the filter if it hasn't been applied beforehand

          Here are the changed files (98 + 1 new file):

          admin/handlevirus.php
          admin/oacleanup.php
          admin/process_email.php
          admin/register.php
          admin/mnet/enr_course_enrol.php
          admin/mnet/enr_courses.php
          admin/roles/assign.php
          admin/roles/override.php
          admin/roles/tabs.php
          auth/cas/forbidden.php
          auth/cas/login.php
          auth/mnet/auth.php
          backup/backup.php
          backup/restore_check.html
          backup/restorelib.php
          backup/try.php
          blocks/admin/block_admin.php
          blocks/course_list/block_course_list.php
          course/category.php
          course/delete.php
          course/edit.php
          course/grade.php
          course/grades.php
          course/groups.php
          course/importstudents.php
          course/index.php
          course/info.php
          course/lib.php
          course/pending.php
          course/recent.php
          course/scales.php
          course/search.php
          course/user.php
          course/import/activities/index.php
          course/import/groups/index.php
          course/report/log/index.php
          course/report/log/lib.php
          course/report/outline/index.php
          course/report/participation/index.php
          course/report/stats/index.php
          course/report/stats/report.php
          enrol/authorize/enrol.php
          enrol/authorize/locallib.php
          enrol/database/enrol.php
          enrol/imsenterprise/importnow.php
          enrol/mnet/allowed_courses.php
          enrol/paypal/ipn.php
          files/index.php
          grade/exceptions.php
          grade/lib.php
          group/assign.php
          group/group.php
          group/grouping.php
          group/index.php
          group/groupgui/index.php
          group/groupgui/printgrouping.php
          lib/accesslib.php
          lib/moodlelib.php
          lib/questionlib.php
          lib/weblib.php
          lib/db/mysql.php
          lib/db/postgres7.php
          lib/simpletest/testweblibphp +
          message/index.php
          mod/assignment/lib.php
          mod/chat/gui_header_js/index.php
          mod/chat/gui_sockets/index.php
          mod/choice/view.php
          mod/data/edit.php
          mod/data/view.php
          mod/forum/discuss.php
          mod/forum/index.php
          mod/forum/lib.php
          mod/forum/post.php
          mod/forum/user.php
          mod/glossary/export.php
          mod/glossary/print.php
          mod/glossary/showentry.php
          mod/hotpot/index.php
          mod/hotpot/report.php
          mod/hotpot/review.php
          mod/hotpot/view.php
          mod/lams/index.php
          mod/lesson/index.php
          mod/resource/index.php
          mod/resource/type/file/resource.class.php
          mod/resource/type/repository/resource.class.php
          mod/scorm/player.php
          mod/scorm/report.php
          mod/scorm/view.php
          mod/survey/report.php
          mod/workshop/assess.php
          question/category_class.php
          user/edit.php
          user/editadvanced.php
          user/tabs.php
          user/view.php
          user/profile/definelib.php
          user/profile/index.php

          Show
          Nicolas Connault added a comment - Not sure if the hotpot module is converting ampersands correctly. $heading and $title are set up differently than in other modules: instead of outputting $course->title, a temporary $title variable is created and assigned that value. That made it difficult for me to decide whether it was already formatted elsewhere or not. Also I didn't format_string() any of the variables included in $navigation strings, because I assume these strings get processed by print_navigation(), which includes format_string(). It would be cool if a given string could contain information about which filters have been applied to it. So a String would be an object with a simple array used as a stack, and a few methods. When you apply a new filter to a string, it would automatically (and optionally) self-check and only apply the filter if it hasn't been applied beforehand Here are the changed files (98 + 1 new file): admin/handlevirus.php admin/oacleanup.php admin/process_email.php admin/register.php admin/mnet/enr_course_enrol.php admin/mnet/enr_courses.php admin/roles/assign.php admin/roles/override.php admin/roles/tabs.php auth/cas/forbidden.php auth/cas/login.php auth/mnet/auth.php backup/backup.php backup/restore_check.html backup/restorelib.php backup/try.php blocks/admin/block_admin.php blocks/course_list/block_course_list.php course/category.php course/delete.php course/edit.php course/grade.php course/grades.php course/groups.php course/importstudents.php course/index.php course/info.php course/lib.php course/pending.php course/recent.php course/scales.php course/search.php course/user.php course/import/activities/index.php course/import/groups/index.php course/report/log/index.php course/report/log/lib.php course/report/outline/index.php course/report/participation/index.php course/report/stats/index.php course/report/stats/report.php enrol/authorize/enrol.php enrol/authorize/locallib.php enrol/database/enrol.php enrol/imsenterprise/importnow.php enrol/mnet/allowed_courses.php enrol/paypal/ipn.php files/index.php grade/exceptions.php grade/lib.php group/assign.php group/group.php group/grouping.php group/index.php group/groupgui/index.php group/groupgui/printgrouping.php lib/accesslib.php lib/moodlelib.php lib/questionlib.php lib/weblib.php lib/db/mysql.php lib/db/postgres7.php lib/simpletest/testweblibphp + message/index.php mod/assignment/lib.php mod/chat/gui_header_js/index.php mod/chat/gui_sockets/index.php mod/choice/view.php mod/data/edit.php mod/data/view.php mod/forum/discuss.php mod/forum/index.php mod/forum/lib.php mod/forum/post.php mod/forum/user.php mod/glossary/export.php mod/glossary/print.php mod/glossary/showentry.php mod/hotpot/index.php mod/hotpot/report.php mod/hotpot/review.php mod/hotpot/view.php mod/lams/index.php mod/lesson/index.php mod/resource/index.php mod/resource/type/file/resource.class.php mod/resource/type/repository/resource.class.php mod/scorm/player.php mod/scorm/report.php mod/scorm/view.php mod/survey/report.php mod/workshop/assess.php question/category_class.php user/edit.php user/editadvanced.php user/tabs.php user/view.php user/profile/definelib.php user/profile/index.php

            People

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

              Dates

              • Created:
                Updated:
                Resolved: