Moodle
  1. Moodle
  2. MDL-32104

Cannot restore backups with 'update post' forum logs

    Details

    • Testing Instructions:
      Hide
      1. This needs to be tested in ALL supported databases
      2. Goto a forum in a course
      3. update any post
      4. Goto site admin>reports>live logs and make sure the log is in the format discuss.php?d=3213#p17623&parent=17623
      5. Backup the course with Include course logs checked
      6. Restore the course
      7. Make sure you get no errors during restore and the entries in the log report for this course are still in the format discuss.php?d=3213#p17623&parent=17623
        (Note:- the urls in logs may be encoded so # appears as %23)
      Show
      This needs to be tested in ALL supported databases Goto a forum in a course update any post Goto site admin>reports>live logs and make sure the log is in the format discuss.php?d=3213#p17623&parent=17623 Backup the course with Include course logs checked Restore the course Make sure you get no errors during restore and the entries in the log report for this course are still in the format discuss.php?d=3213#p17623&parent=17623 (Note:- the urls in logs may be encoded so # appears as %23)
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-32104-master
    • Rank:
      38812

      Description

      The "update post" log action in the forum module fails to parse properly when restoring a course.
      eg:

       module |   action    |                  url                   
      --------+-------------+----------------------------------------
       forum  | update post | discuss.php?d=3237#p13137&parent=13137
       forum  | update post | discuss.php?d=1532#p6295&parent=6295
       forum  | update post | discuss.php?d=3213#p17623&parent=17623
      

      It logs to the backup XML like this:

        <log id="3806754">
          <time>1331694499</time>
          <userid>44</userid>
          <ip>129.94.52.74</ip>
          <module>forum</module>
          <action>update post</action>
          <url>discuss.php?d=9020#p31092&amp;parent=31092</url>
          <info>31092</info>
        </log>
      

      mod/forum/backup/moodle2/restore_forum_activity_task.class.php:114:

              $rules[] = new restore_log_rule('forum', 'update post', 'discuss.php?d={forum_discussion}&parent={forum_post}', '{forum_post}');
      

      Parses the 'd' parameter as a non-integer which crashes the restore:

      Default exception handler: Error reading from database Debug: ERROR:  invalid input syntax for integer: "9020#p31092" SELECT * FROM mdl_backup_ids_temp WHERE backupid = $1 AND itemname = $2 AND itemid = $3\n[array (\n  0 => '0b3f2f773e7ac2546690019215ff867a',\n  1 => 'forum_discussion',  2 => '9020#p31092',\n)]
      * line 394 of /lib/dml/moodle_database.php: dml_read_exception thrown\n* line 232 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      * line 678 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()\n* line 1297 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
      * line 1269 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
      * line 1249 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()\n* line 1261 of /backup/util/dbops/restore_dbops.class.php: call to moodle_database->get_record()
      * line 197 of /backup/util/helper/restore_log_rule.class.php: call to restore_dbops::get_backup_ids_record()\n* line 137 of /backup/util/helper/restore_log_rule.class.php: call to restore_log_rule->parse_tokens_and_matches()
      * line 84 of /backup/util/helper/restore_logs_processor.class.php: call to restore_
      log_rule->process()
      * line 1933 of /backup/moodle2/restore_stepslib.php: call to restore_logs_processor->process_log_record()\n* line 131 of /backup/util/plan/restore_structure_step.class.php: call to restore_activity_logs_structure_step->process_log()
      * line 103 of /backup/util/helper/restore_structure_parser_processor.class.php: call to restore_structure_step->process()
      * line 130 of /backup/util/xml/parser/processors/grouped_parser_processor.class.php: call to restore_structure_parser_processor->dispatch_chunk()
      * line 91 of /backup/util/helper/restore_structure_parser_processor.class.php: call to grouped_parser_processor->postprocess_chunk()\n* line 148 of /backup/util/xml/parser/processors/simplified_parser_processor.class.php: call to restore_structure_parser_processor->postprocess_chunk()
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          I noted that you didn't add a "partner" tag. You still can if you wish.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. I noted that you didn't add a "partner" tag. You still can if you wish.
          Hide
          Ankit Agarwal added a comment -

          We can either change the url format when adding logs or change the rule during restore to fix this issue.
          Changing the restore log rule makes more sense to me, so I have added a solution with that approach.
          Sending for peer-review.
          Thanks

          Show
          Ankit Agarwal added a comment - We can either change the url format when adding logs or change the rule during restore to fix this issue. Changing the restore log rule makes more sense to me, so I have added a solution with that approach. Sending for peer-review. Thanks
          Hide
          Rossiani Wijaya added a comment -

          looks good ankit.

          Show
          Rossiani Wijaya added a comment - looks good ankit.
          Hide
          Ankit Agarwal added a comment -

          Thanks Rosie for the review.
          Sending for integration
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration Thanks
          Hide
          Dan Poltawski 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.

          TIA and ciao

          Show
          Dan Poltawski 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. TIA and ciao
          Hide
          Ankit Agarwal added a comment -

          rebased!

          Show
          Ankit Agarwal added a comment - rebased!
          Hide
          Dan Poltawski added a comment -

          Thanks, that has been integrated now!

          Show
          Dan Poltawski added a comment - Thanks, that has been integrated now!
          Hide
          Michael de Raadt added a comment -

          So far I have tested this in 2.1, 2.2 and master on MySQL.

          I have also tested it on PostgreSQL and MS SQL in master.

          I'm waiting for my Oracle instance to finish PHPUnit tests before I can test it there.

          Show
          Michael de Raadt added a comment - So far I have tested this in 2.1, 2.2 and master on MySQL. I have also tested it on PostgreSQL and MS SQL in master. I'm waiting for my Oracle instance to finish PHPUnit tests before I can test it there.
          Hide
          Michael de Raadt added a comment -

          Test result: Success

          Tested in master under all DBs. Also tested in 2.1 and 2.2 under MySQL.

          Show
          Michael de Raadt added a comment - Test result: Success Tested in master under all DBs. Also tested in 2.1 and 2.2 under MySQL.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: