Moodle
  1. Moodle
  2. MDL-13435

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

    Details

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

      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.

      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
        Tim Hunt added a comment -

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

        Show
        Tim Hunt added a comment - This is not really anything to do with the question bank, so removing that component.
        Hide
        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
        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
        Tim Hunt added a comment -

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

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

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

        Show
        Tim Hunt added a comment - Slightly improved patch. $index was not being used, so omit it to save memory.
        Hide
        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
        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
        Petr Škoda added a comment -

        +1 for 2.0 too
        -5 for any stable branch

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

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

        Resolving as fixed..ciao

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

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

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