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

lib/xmlize.php doesn't care about xml errors

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.4, 1.9, 2.0
    • Fix Version/s: 2.0
    • Component/s: Backup
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      The library function xmlize does not have any kind of error reporting at all so even quite subtle bugs in the processed xml code silently produce incorrect data. For example the attached file contains an HTML entity and this simply truncates the xml data at the point of the entity returning an ostensibly valid yet incomplete result structure.

      This is bad. It should, were possible, return an error condition.

        Gliffy Diagrams

        1. broken.xml
          0.1 kB
          Howard Miller
        2. MDL-13435.patch_with_lang_string.patch.txt
          5 kB
          Eloy Lafuente (stronk7)
        3. xml_errors.patch.txt
          4 kB
          Tim Hunt

          Activity

          Hide
          timhunt Tim Hunt added a comment -

          This is not really anything to do with the question bank, so removing that component.

          Show
          timhunt Tim Hunt added a comment - This is not really anything to do with the question bank, so removing that component.
          Hide
          timhunt Tim Hunt added a comment -

          We have a proposed fix for this thanks to my colleague Mahmoud: http://github.com/mkassaei/Moodle-Question-Engine-2/commit/acfbfec23cc4065e35fe5f3d6b77d0dc0cce57f6

          Would anyone like to review this for inclusion in Moodle 2.0?

          Show
          timhunt Tim Hunt added a comment - We have a proposed fix for this thanks to my colleague Mahmoud: http://github.com/mkassaei/Moodle-Question-Engine-2/commit/acfbfec23cc4065e35fe5f3d6b77d0dc0cce57f6 Would anyone like to review this for inclusion in Moodle 2.0?
          Hide
          timhunt Tim Hunt added a comment -

          Here is a slightly cleaned up version of the patch that will apply cleanly to HEAD.

          Show
          timhunt Tim Hunt added a comment - Here is a slightly cleaned up version of the patch that will apply cleanly to HEAD.
          Hide
          timhunt Tim Hunt added a comment -

          Slightly improved patch. $index was not being used, so omit it to save memory.

          Show
          timhunt Tim Hunt added a comment - Slightly improved patch. $index was not being used, so omit it to save memory.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Added patch that simply adds lang string in error.php.

          Tested, works ok. And it's full BC.

          My +1 for improving this only in HEAD. Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Added patch that simply adds lang string in error.php. Tested, works ok. And it's full BC. My +1 for improving this only in HEAD. Ciao
          Hide
          skodak Petr Skoda added a comment -

          +1 for 2.0 too
          -5 for any stable branch

          Show
          skodak Petr Skoda added a comment - +1 for 2.0 too -5 for any stable branch
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The patch has landed to HEAD. Thanks for proposal/feedback!

          Resolving as fixed..ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The patch has landed to HEAD. Thanks for proposal/feedback! Resolving as fixed..ciao
          Hide
          timhunt Tim Hunt added a comment -

          Thanks guys. Sorry about missing the lang string out of the patch.

          Show
          timhunt Tim Hunt added a comment - Thanks guys. Sorry about missing the lang string out of the patch.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                24/Nov/10