Moodle
  1. Moodle
  2. MDL-37893

PHP error when parsing XML backup and the grouped element contains no final elements

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Backup
    • Labels:
    • Rank:
      47647

      Description

      Let us have an XML structure like this:

      <MOODLE_BACKUP>
        <COURSE>
          <FORMATDATA>
            <WEEKS>
              <WEEK>
                <SECTION>1</SECTION>
                <HIDENUMBER>1</HIDENUMBER>
              </WEEK>
            </WEEKS>
          </FORMATDATA>
        </COURSE>
      </MOODLE_BACKUP>
      

      Let us parse this XML with our progressive_parser and process it with progressive_parser_processor classes. Let us observe following paths:

      $pr->add_path('/MOODLE_BACKUP/COURSE/FORMATDATA', true);
      $pr->add_path('/MOODLE_BACKUP/COURSE/FORMATDATA/WEEKS/WEEK');
      

      As you can see, the FORMATDATA elements was requested to be grouped (the second parameter of the add_path() method). To refresh your backup/restore API knowledge, if a path is processed as a grouped one, it is returned in one chunk together with all its subpaths.

      Expected behaviour:

      We should get an array like FORMATDATA => WEEKS => WEEK[0], WEEK[1], WEEK[2] etc.

      Actual behaviour:

      PHP error "array_key_exists() expects parameter 2 to be array, null given" when calling build_currentdata() method's code:

      if (!array_key_exists($grouped, $this->currentdata)) {
          $a = new stdclass();
          $a->grouped = $grouped;
          $a->child = $data['path'];
          throw new progressive_parser_exception('xml_cannot_add_to_grouped', $a);
      }
      

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          This was discovered while working on MDL-32205.

          Show
          David Mudrak added a comment - This was discovered while working on MDL-32205 .
          Hide
          David Mudrak added a comment -

          This is the patch I'm about to propose to submit. I have a unit test for it. Leaving it here for a quick feedback now:

          diff --git a/backup/util/xml/parser/processors/grouped_parser_processor.class.php b/backup/util/xml/parser/processors/grouped_parser_processor.class.php
          index d8e260a..643537b 100644
          --- a/backup/util/xml/parser/processors/grouped_parser_processor.class.php
          +++ b/backup/util/xml/parser/processors/grouped_parser_processor.class.php
          @@ -166,6 +166,19 @@ abstract class grouped_parser_processor extends simplified_parser_processor {
                * grouped element for later dispatching once it is complete
                */
               protected function build_currentdata($grouped, $data) {
          +        if (!is_array($this->currentdata)) {
          +            // This is a situation when the grouped element itself does not have
          +            // any final elements but only other subpaths. Let us do a little
          +            // trick and pretend that the empty grouped element was properly
          +            // catched before.
          +            $this->currentdata = array(
          +                $grouped => array(
          +                    'path' => $grouped,
          +                    'level' => substr_count($grouped, '/') + 1,
          +                    'tags' => array(),
          +                )
          +            );
          +        }
                   // Check the grouped already exists into currentdata
                   if (!array_key_exists($grouped, $this->currentdata)) {
                       $a = new stdclass();
          
          Show
          David Mudrak added a comment - This is the patch I'm about to propose to submit. I have a unit test for it. Leaving it here for a quick feedback now: diff --git a/backup/util/xml/parser/processors/grouped_parser_processor.class.php b/backup/util/xml/parser/processors/grouped_parser_processor.class.php index d8e260a..643537b 100644 --- a/backup/util/xml/parser/processors/grouped_parser_processor.class.php +++ b/backup/util/xml/parser/processors/grouped_parser_processor.class.php @@ -166,6 +166,19 @@ abstract class grouped_parser_processor extends simplified_parser_processor { * grouped element for later dispatching once it is complete */ protected function build_currentdata($grouped, $data) { + if (!is_array($ this ->currentdata)) { + // This is a situation when the grouped element itself does not have + // any final elements but only other subpaths. Let us do a little + // trick and pretend that the empty grouped element was properly + // catched before. + $ this ->currentdata = array( + $grouped => array( + 'path' => $grouped, + 'level' => substr_count($grouped, '/') + 1, + 'tags' => array(), + ) + ); + } // Check the grouped already exists into currentdata if (!array_key_exists($grouped, $ this ->currentdata)) { $a = new stdclass();
          Hide
          Eloy Lafuente (stronk7) added a comment -

          At first sight it has sense, we just need to super-triple ensure it does not change previous behavior at all, by verifying old tests and surely introducing new ones.

          Will look to it a bit deeper later, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - At first sight it has sense, we just need to super-triple ensure it does not change previous behavior at all, by verifying old tests and surely introducing new ones. Will look to it a bit deeper later, thanks!
          Hide
          David Mudrak added a comment -

          So, after some more time spent with this (and with most valuable help from Eloy - thanks!), I am brave enough to submit this for integration. This is a blocker for another issue that is to be fixed on STABLE branches so I would need this integrated into STABLE branches as well.

          The logic of the patch is documented inline. At the end, I just decided to put the "fall back" into the before_path() method to be sure we really catch and register all paths registered as grouped ones.

          Attached unit test should document all reasonable combinations. Please note there is still this case of ignored attributes at empty grouped paths. For example, when /data/foo is registered as a grouped path and the XML looks like

          <data>
            <foo bar="baz">
            </foo>
          </data>
          

          the value of the bar attribute is not obtained. Eloy explains:

          (01:05:14) Eloy: nah, impossible. the problem comes from the parser.
          As far as it's progressive... when it reads

          <grouped id="987654321">

          it does not know in advance what's coming next, so it passes "grouped"
          as a tag of the "moodle2" chunk. Not as a chunk itself.

          Only if the "grouped" has tags or if the next "subs" has attributes then
          the grouped is sent as a chunk itself.
          (01:05:27) Eloy: So, surely... nothing to fix easily.

          So, at least the behaviour is now covered in unit tests. We do not consider it as a problem as the attributes are not massively used in Moodle XML files.

          Show
          David Mudrak added a comment - So, after some more time spent with this (and with most valuable help from Eloy - thanks!), I am brave enough to submit this for integration. This is a blocker for another issue that is to be fixed on STABLE branches so I would need this integrated into STABLE branches as well. The logic of the patch is documented inline. At the end, I just decided to put the "fall back" into the before_path() method to be sure we really catch and register all paths registered as grouped ones. Attached unit test should document all reasonable combinations. Please note there is still this case of ignored attributes at empty grouped paths. For example, when /data/foo is registered as a grouped path and the XML looks like <data> <foo bar= "baz" > </foo> </data> the value of the bar attribute is not obtained. Eloy explains: (01:05:14) Eloy: nah, impossible. the problem comes from the parser. As far as it's progressive... when it reads <grouped id="987654321"> it does not know in advance what's coming next, so it passes "grouped" as a tag of the "moodle2" chunk. Not as a chunk itself. Only if the "grouped" has tags or if the next "subs" has attributes then the grouped is sent as a chunk itself. (01:05:27) Eloy: So, surely... nothing to fix easily. So, at least the behaviour is now covered in unit tests. We do not consider it as a problem as the attributes are not massively used in Moodle XML files.
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Cheers!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Cheers!
          Hide
          Damyon Wiese added a comment -

          This looks OK,

          Unit tests pass on old versions, reverting the fix and keeping the unit test triggers the fail.

          However - I'm not confident about pushing this to stables without a thumbs up from Eloy. Putting back into Waiting for integration review list.

          Show
          Damyon Wiese added a comment - This looks OK, Unit tests pass on old versions, reverting the fix and keeping the unit test triggers the fail. However - I'm not confident about pushing this to stables without a thumbs up from Eloy. Putting back into Waiting for integration review list.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm adding one extra commit taking rid of the left "// This fails at the moment." comment, but I think it's ok, so pushing... let's break the whole world (joking, I hope).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm adding one extra commit taking rid of the left "// This fails at the moment." comment, but I think it's ok, so pushing... let's break the whole world (joking, I hope). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Andrew Davis added a comment -

          As far as I can tell this is working at least as well as before. Passing.

          Show
          Andrew Davis added a comment - As far as I can tell this is working at least as well as before. Passing.
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: