Moodle
  1. Moodle
  2. MDL-35032

book restore from moodle 1.9: Images are not imported

    Details

    • Testing Instructions:
      Hide

      Pre-requisites

      1. Create a book on a 1.9 instance
        • Add some chapters/subchaters with some files
        • Create a backup of that book

      Test

      1. Create a new course without enabling the legacy files
      2. Restore the backup from 1.9
      3. If the legacy files have been enabled after the restore, disable them again
      4. View the book
        • Make sure the images have been restored
      Show
      Pre-requisites Create a book on a 1.9 instance Add some chapters/subchaters with some files Create a backup of that book Test Create a new course without enabling the legacy files Restore the backup from 1.9 If the legacy files have been enabled after the restore, disable them again View the book Make sure the images have been restored
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
    • Pull Master Branch:
      MDL-35032-master
    • Story Points (Obsolete):
      8
    • Sprint:
      FRONTEND Sprint 7

      Description

      A linked or embedded image from the course_files in Moodle 1.9 is not converted when restoring a 1.9 backup in moodle 2.3.

      Reason: https://github.com/moodle/moodle/blob/master/mod/book/backup/moodle1/lib.php

      The moodle2-xml file is written before the images are migrated and so the @FILEPHP@ Links are not replaced by the @@PLUGINFILE@@ Links. $this->write_xml('chapter', $data, array('/chapter/id')); should be executed at the end.

      public function process_book_chapters($data) {
          $this->write_xml('chapter', $data, array('/chapter/id'));
       
          // convert chapter files
          $this->fileman->filearea = 'chapter';
          $this->fileman->itemid   = $data['id'];
          $data['content'] = moodle1_converter::migrate_referenced_files($data['content'], $this->fileman);
      }

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that and suggesting a fix.

          If you are able to turn this into a patch, or even better, to provide Git links, that would be helpful. Providing testing instructions would make this issue supremely excellent.

          Show
          Michael de Raadt added a comment - Thanks for reporting that and suggesting a fix. If you are able to turn this into a patch, or even better, to provide Git links, that would be helpful. Providing testing instructions would make this issue supremely excellent.
          Hide
          Ankit Agarwal added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Ankit Agarwal added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          Frédéric Massart added a comment -

          I have had some troubles replicating this issue, without the patch the restore works perfectly, at least it did for me (2.4 onwards). But, as the restore automatically enabled the legacy files if you happen to disable them, then the images are not displayed any more.

          The suggested fix properly converts the images to the book area and uses PLUGINFILE. That way, whether or not the legacy files are used, the images are available from the book.

          I was questioning the fact that in 1.9 you could change the image in one place and having them updated all through your course, but it does not seem that we are willing to support that. We tend to ignore the legacy files as much as we can, and convert all the legacy files to the modules respective areas.

          So there is it, up for peer review.

          Show
          Frédéric Massart added a comment - I have had some troubles replicating this issue, without the patch the restore works perfectly, at least it did for me (2.4 onwards). But, as the restore automatically enabled the legacy files if you happen to disable them, then the images are not displayed any more. The suggested fix properly converts the images to the book area and uses PLUGINFILE. That way, whether or not the legacy files are used, the images are available from the book. I was questioning the fact that in 1.9 you could change the image in one place and having them updated all through your course, but it does not seem that we are willing to support that. We tend to ignore the legacy files as much as we can, and convert all the legacy files to the modules respective areas. So there is it, up for peer review.
          Hide
          Damyon Wiese added a comment -

          Moodle 19 backup with book + images.

          Show
          Damyon Wiese added a comment - Moodle 19 backup with book + images.
          Hide
          Damyon Wiese added a comment -

          I added a 19 backup that can be used for testing this. There should be 1 image in the book description, and one on the chapter one description.

          [Y] Syntax
          [Y] Whitespace
          [-] Output
          [-] Language
          [-] Databases
          [Y] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [Y] Git - (There is a typo in the commit message - but close enough)
          [-] Third party code
          [Y] Sanity check

          Looks good Fred, sending for integration.

          Show
          Damyon Wiese added a comment - I added a 19 backup that can be used for testing this. There should be 1 image in the book description, and one on the chapter one description. [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [-] Security [-] Documentation [Y] Git - (There is a typo in the commit message - but close enough) [-] Third party code [Y] Sanity check Looks good Fred, sending for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi guys,

          this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues).

          So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet.

          Please, feel free to comment any idea/objection @ MDLSITE-2662. I'll be collecting everything there.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi guys, this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues). So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet. Please, feel free to comment any idea/objection @ MDLSITE-2662 . I'll be collecting everything there. TIA and ciao
          Hide
          CiBoT added a comment -

          Results for MDL-35032

          Show
          CiBoT added a comment - Results for MDL-35032 Branch MDL-35032 -24 to be integrated into upstream MOODLE_24_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/683 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/683/artifact/work/smurf.html Branch MDL-35032 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/684 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/684/artifact/work/smurf.html Branch MDL-35032 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/685 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/685/artifact/work/smurf.html Branch MDL-35032 -master to be integrated into upstream master Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/686 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/686/artifact/work/smurf.html
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Fred - this has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          Tested during integration review and passed. Thanks Fred.

          Show
          Sam Hemelryk added a comment - Tested during integration review and passed. Thanks Fred.
          Hide
          Sam Hemelryk added a comment -

          Thank you, your code has landed just in time for 2013.
          Merry Christmas and may your 2014 be even better than 2013.

          Kind regards with much holiday spirit
          Sam

          Show
          Sam Hemelryk added a comment - Thank you, your code has landed just in time for 2013. Merry Christmas and may your 2014 be even better than 2013. Kind regards with much holiday spirit Sam

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile