Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: DEV backlog
    • Fix Version/s: 2.1
    • Component/s: Resource
    • Labels:
      None
    • Rank:
      1233

      Issue Links

        Activity

        Hide
        David Mudrak added a comment -

        As discussed in the chat, I would suggest something like:

        In new mods like mod/url/, define proper handler classes in backup/moodle1/lib.php but let their get_paths() returning empty array. Then in mod/resource/, create new instances of these handlers (libraries should be already loaded by the core) and re-dispatch relevant data to them.

        So for example in moodle1_mod_resource_handler constructor, create new "sub-handlers" for each type

        parent::__constructor(...);
        $this->filehandler = new moodle1_mod_file_handler($this->converter);
        ...
        

        and then re-dispatch the data to relevant sub-handler explicitly:

        if (....) {
            $this->filehandler->process_xxx($data, $raw);
        }
        

        This is a pattern that I use in the core handlers already and it seems to work well.

        Show
        David Mudrak added a comment - As discussed in the chat, I would suggest something like: In new mods like mod/url/, define proper handler classes in backup/moodle1/lib.php but let their get_paths() returning empty array. Then in mod/resource/, create new instances of these handlers (libraries should be already loaded by the core) and re-dispatch relevant data to them. So for example in moodle1_mod_resource_handler constructor, create new "sub-handlers" for each type parent::__constructor(...); $ this ->filehandler = new moodle1_mod_file_handler($ this ->converter); ... and then re-dispatch the data to relevant sub-handler explicitly: if (....) { $ this ->filehandler->process_xxx($data, $raw); } This is a pattern that I use in the core handlers already and it seems to work well.
        Hide
        Andrew Davis added a comment -

        Ok, I think I have something workable. One quirk I wasn't able to work out what $this->xmlwriter is null by the time you get to on_resource_end(). Not sure why. The simple work around was moving the xml close tags into process_resouce(). I still don't know what is taking out the xmlwriter however I suspect its related to instantiating additional handlers.

        Show
        Andrew Davis added a comment - Ok, I think I have something workable. One quirk I wasn't able to work out what $this->xmlwriter is null by the time you get to on_resource_end(). Not sure why. The simple work around was moving the xml close tags into process_resouce(). I still don't know what is taking out the xmlwriter however I suspect its related to instantiating additional handlers.
        Hide
        David Mudrak added a comment -

        Changes in moodle1 converter handlerlib.php were needed to support resource successors. Awaiting for the next iteration of the review.

        Show
        David Mudrak added a comment - Changes in moodle1 converter handlerlib.php were needed to support resource successors. Awaiting for the next iteration of the review.
        Hide
        David Mudrak added a comment -

        When you say "$this->xmlwriter is null", what is "$this"? Note that every instance of moodle1_xml_hanlder subclass has its own xmlwriter. So if you open a writer in the moodle1_mod_resource_handler, it is not available in the successor's moodle1_mod_page_handler automatically. There are several ways how to deal with the data flow in this case, please let me know what you are trying to achieve.

        Also note that the moodle1_converter core will have to be changed to support files conversion in modules. So for example Folder or File do not have a way how to get their files into the proper file area yet.

        Show
        David Mudrak added a comment - When you say "$this->xmlwriter is null", what is "$this"? Note that every instance of moodle1_xml_hanlder subclass has its own xmlwriter. So if you open a writer in the moodle1_mod_resource_handler, it is not available in the successor's moodle1_mod_page_handler automatically. There are several ways how to deal with the data flow in this case, please let me know what you are trying to achieve. Also note that the moodle1_converter core will have to be changed to support files conversion in modules. So for example Folder or File do not have a way how to get their files into the proper file area yet.
        Hide
        Andrew Davis added a comment - - edited

        re the xml writer being null i meant in on_resource_end() in moodle1_mod_resource_handler. However, just retested and it now seems to work. Guessing youre reworking off the process_resource() code fixed whatever I broke.

        Update: Ive actually had it a few times while running the unit tests. When it happens I refresh and it goes away....

        Show
        Andrew Davis added a comment - - edited re the xml writer being null i meant in on_resource_end() in moodle1_mod_resource_handler. However, just retested and it now seems to work. Guessing youre reworking off the process_resource() code fixed whatever I broke. Update: Ive actually had it a few times while running the unit tests. When it happens I refresh and it goes away....
        Hide
        Andrew Davis added a comment -

        Ive just pushed changes to github that remove my half baked attempts at file handling pending file fixes in core restore code. I've also added handling for 1.9 resources that need to be turned into 2.1 mod_url instances. I think the files handling is the next big missing piece.

        Show
        Andrew Davis added a comment - Ive just pushed changes to github that remove my half baked attempts at file handling pending file fixes in core restore code. I've also added handling for 1.9 resources that need to be turned into 2.1 mod_url instances. I think the files handling is the next big missing piece.
        Hide
        Andrew Davis added a comment - - edited

        I've attempted to implement file migration in the imscp module without success. In my 1.9 backup the file is at course_files/M150_2_scorm.zip when i run the unit tests (using moodle.xml out of that backup) I get the following.

        Exception: backup/converter/moodle1/simpletest/testlib.php / ► moodle1_converter_test / ► test_convert_run_convert
        Unexpected exception of type [moodle1_convert_exception] with message [Exception with missing language string {file_not_readable} from language file {error} with data {/home/andrew/Desktop/moodledata/head/temp/backup/ccc167a3ccc054ee577dd3012118ac7a/course_files/M150_2_scorm.zip}] in [/home/andrew/Desktop/git/head/backup/converter/moodle1/lib.php line 1077]
        
            * line 56 of /mod/imscp/backup/moodle1/lib.php: call to moodle1_file_manager->migrate_file()
            * line 133 of /mod/resource/backup/moodle1/lib.php: call to moodle1_mod_imscp_handler->process_resource()
            * line 272 of /backup/converter/moodle1/lib.php: call to moodle1_mod_resource_handler->process_resource()
            * line 618 of /backup/converter/moodle1/lib.php: call to moodle1_converter->process_chunk()
            * line 125 of /backup/util/xml/parser/processors/grouped_parser_processor.class.php: call to moodle1_parser_processor->dispatch_chunk()
            * line 601 of /backup/converter/moodle1/lib.php: call to grouped_parser_processor->postprocess_chunk()
            * line 148 of /backup/util/xml/parser/processors/simplified_parser_processor.class.php: call to moodle1_parser_processor->postprocess_chunk()
            * line 92 of /backup/util/xml/parser/processors/progressive_parser_processor.class.php: call to simplified_parser_processor->process_chunk()
            * line 169 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser_processor->receive_chunk()
            * line 253 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->publish()
            * line ? of unknownfile: call to progressive_parser->end_tag()
            * line 158 of /backup/util/xml/parser/progressive_parser.class.php: call to xml_parse()
            * line 137 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->parse()
            * line 125 of /backup/converter/moodle1/lib.php: call to progressive_parser->process()
            * line 85 of /backup/converter/convertlib.php: call to moodle1_converter->execute()
            * line 244 of /backup/converter/moodle1/simpletest/testlib.php: call to base_converter->convert()
            * line ... of ...
        
        

        Advice, suggestions etc are more than welcome.

        Show
        Andrew Davis added a comment - - edited I've attempted to implement file migration in the imscp module without success. In my 1.9 backup the file is at course_files/M150_2_scorm.zip when i run the unit tests (using moodle.xml out of that backup) I get the following. Exception: backup/converter/moodle1/simpletest/testlib.php / ► moodle1_converter_test / ► test_convert_run_convert Unexpected exception of type [moodle1_convert_exception] with message [Exception with missing language string {file_not_readable} from language file {error} with data {/home/andrew/Desktop/moodledata/head/temp/backup/ccc167a3ccc054ee577dd3012118ac7a/course_files/M150_2_scorm.zip}] in [/home/andrew/Desktop/git/head/backup/converter/moodle1/lib.php line 1077] * line 56 of /mod/imscp/backup/moodle1/lib.php: call to moodle1_file_manager->migrate_file() * line 133 of /mod/resource/backup/moodle1/lib.php: call to moodle1_mod_imscp_handler->process_resource() * line 272 of /backup/converter/moodle1/lib.php: call to moodle1_mod_resource_handler->process_resource() * line 618 of /backup/converter/moodle1/lib.php: call to moodle1_converter->process_chunk() * line 125 of /backup/util/xml/parser/processors/grouped_parser_processor.class.php: call to moodle1_parser_processor->dispatch_chunk() * line 601 of /backup/converter/moodle1/lib.php: call to grouped_parser_processor->postprocess_chunk() * line 148 of /backup/util/xml/parser/processors/simplified_parser_processor.class.php: call to moodle1_parser_processor->postprocess_chunk() * line 92 of /backup/util/xml/parser/processors/progressive_parser_processor.class.php: call to simplified_parser_processor->process_chunk() * line 169 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser_processor->receive_chunk() * line 253 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->publish() * line ? of unknownfile: call to progressive_parser->end_tag() * line 158 of /backup/util/xml/parser/progressive_parser.class.php: call to xml_parse() * line 137 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->parse() * line 125 of /backup/converter/moodle1/lib.php: call to progressive_parser->process() * line 85 of /backup/converter/convertlib.php: call to moodle1_converter->execute() * line 244 of /backup/converter/moodle1/simpletest/testlib.php: call to base_converter->convert() * line ... of ... Advice, suggestions etc are more than welcome.
        Hide
        Aparup Banerjee added a comment - - edited

        Andrew, you can run an actual restore. then right before phase 3, your convertor's debugging can output on the page and/or moodledata/temp/backup will have files generated.

        Show
        Aparup Banerjee added a comment - - edited Andrew, you can run an actual restore. then right before phase 3, your convertor's debugging can output on the page and/or moodledata/temp/backup will have files generated.
        Hide
        David Mudrak added a comment -

        Andrew, do I understand it correctly that you are getting this error when running the unit test after replacing just simpletest/files/moodle.xml ? If so then it is expected, indeed. Because your moodle.xml refers to a file in

        {backup temp dir}

        /course_files/ but that file does not exist in the directory used for the unittest.

        As Apu stated above, you may want to test the code by actually going to the Restore page, pick the 1.9 ZIP (a have them uploaded into my Private files so the picking is pretty quick) and going thru the restore process. Between restore phases "2. Destination" and "3. Settings" the actual conversion happens. So when the restore wizards reaches the stage 3, you can go to the moodledata/temp/backup/ and you would find the converted directory there.

        And BTW exceptions without language string are OK - I refuse to introduce strings for them as I believe that translating a message that is not expected to ever appear is wasting our good translators' time. That is why I name the strings so that they should be self-descriptive.

        Show
        David Mudrak added a comment - Andrew, do I understand it correctly that you are getting this error when running the unit test after replacing just simpletest/files/moodle.xml ? If so then it is expected, indeed. Because your moodle.xml refers to a file in {backup temp dir} /course_files/ but that file does not exist in the directory used for the unittest. As Apu stated above, you may want to test the code by actually going to the Restore page, pick the 1.9 ZIP (a have them uploaded into my Private files so the picking is pretty quick) and going thru the restore process. Between restore phases "2. Destination" and "3. Settings" the actual conversion happens. So when the restore wizards reaches the stage 3, you can go to the moodledata/temp/backup/ and you would find the converted directory there. And BTW exceptions without language string are OK - I refuse to introduce strings for them as I believe that translating a message that is not expected to ever appear is wasting our good translators' time. That is why I name the strings so that they should be self-descriptive.
        Hide
        David Mudrak added a comment -

        Hi Andrew, here are some comments on your current resource conversion code at github:

        (1) Firstly, after reading mod/resource/backuplib.php and
        mod/resource/type/ims/deploy.php in 1.9 and imscp_20_migrate() function in 2.0
        mod/imscp/db/upgradelib.php, I'm afraid that your current file migration code
        is not correct. When the IMS content package is being deployed, it is copied
        into

        moddata/resource/{resourceid}/filename.zip
        

        and unzipped there. During the upgrade to 2.0, the original filename.zip is
        migrated into a file in the filearea 'backup' with the itemid '1'. The
        contents of that zip file are migrated into the filearea 'content' with the
        itemid '1'.

        In your code, you are trying to convert some course_files, setting the itemid
        to the id of resource. I don't understand how did you come to that.

        (2) The instance variables $cminfo and $fileman are not declared in the class
        header. Even though PHP itself does not require it, I would prefer explicit
        declaration like

        class moodle1_mod_imscp_handler extends moodle1_mod_handler {
        
            /** @var array in-memory cache for the course module information for the current imscp  */
            protected $currentcminfo = null;
        
            /** @var moodle1_file_manager the file manager instance used for the current imscp instance */
            protected $fileman = null;
        

        (3) Please note that if the itemid is known in advance and does not change, it
        can be passed to the file_manager constructor.

        (4) In moodle1_mod_resource_handler, are you sure that

        $data['tobemigrated'] = 0;
        

        is correct? IIRC using this flag was the only way how to convert all needed
        files into the 2.0 framework. Not sure if that applies to the restore
        though...

        Show
        David Mudrak added a comment - Hi Andrew, here are some comments on your current resource conversion code at github: (1) Firstly, after reading mod/resource/backuplib.php and mod/resource/type/ims/deploy.php in 1.9 and imscp_20_migrate() function in 2.0 mod/imscp/db/upgradelib.php, I'm afraid that your current file migration code is not correct. When the IMS content package is being deployed, it is copied into moddata/resource/{resourceid}/filename.zip and unzipped there. During the upgrade to 2.0, the original filename.zip is migrated into a file in the filearea 'backup' with the itemid '1'. The contents of that zip file are migrated into the filearea 'content' with the itemid '1'. In your code, you are trying to convert some course_files, setting the itemid to the id of resource. I don't understand how did you come to that. (2) The instance variables $cminfo and $fileman are not declared in the class header. Even though PHP itself does not require it, I would prefer explicit declaration like class moodle1_mod_imscp_handler extends moodle1_mod_handler { /** @ var array in-memory cache for the course module information for the current imscp */ protected $currentcminfo = null ; /** @ var moodle1_file_manager the file manager instance used for the current imscp instance */ protected $fileman = null ; (3) Please note that if the itemid is known in advance and does not change, it can be passed to the file_manager constructor. (4) In moodle1_mod_resource_handler, are you sure that $data['tobemigrated'] = 0; is correct? IIRC using this flag was the only way how to convert all needed files into the 2.0 framework. Not sure if that applies to the restore though...
        Hide
        Andrew Davis added a comment -

        2 and 3 are done. will deal with 1 and 4 after scrum meeting.

        Show
        Andrew Davis added a comment - 2 and 3 are done. will deal with 1 and 4 after scrum meeting.
        Hide
        Andrew Davis added a comment -

        "Andrew, do I understand it correctly that you are getting this error when running the unit test after replacing just simpletest/files/moodle.xml ? If so then it is expected, indeed. Because your moodle.xml refers to a file in

        {backup temp dir}

        /course_files/ but that file does not exist in the directory used for the unittest."

        duh, of course the unit tests wont work. Silly me Of course that does somewhat limit the usefulness of the unit tests if we can't include and modules that require files.

        Show
        Andrew Davis added a comment - "Andrew, do I understand it correctly that you are getting this error when running the unit test after replacing just simpletest/files/moodle.xml ? If so then it is expected, indeed. Because your moodle.xml refers to a file in {backup temp dir} /course_files/ but that file does not exist in the directory used for the unittest." duh, of course the unit tests wont work. Silly me Of course that does somewhat limit the usefulness of the unit tests if we can't include and modules that require files.
        Hide
        David Mudrak added a comment -

        Of course that does somewhat limit the usefulness of the unit tests

        That's not true. Look at the unit test source code. It actually includes moodle1_file_manager tests. Note that the test that actually runs convert() is a temporary helper and will be removed, replaced with smaller test cases focused on particular cases.

        Show
        David Mudrak added a comment - Of course that does somewhat limit the usefulness of the unit tests That's not true. Look at the unit test source code. It actually includes moodle1_file_manager tests. Note that the test that actually runs convert() is a temporary helper and will be removed, replaced with smaller test cases focused on particular cases.
        Hide
        Andrew Davis added a comment -

        re #4, the tobemigrated column on resources. It probably shouldn't be there however it is still in the database as it was never dropped. Im guessing we just forgot to come back and drop it.

        Show
        Andrew Davis added a comment - re #4, the tobemigrated column on resources. It probably shouldn't be there however it is still in the database as it was never dropped. Im guessing we just forgot to come back and drop it.
        Hide
        David Mudrak added a comment -

        Hmm interesting. I lived in impression that the flag is still needed because of how the files migration was done on 1.9 => 2.0 upgrade. But apparently not - grepping for "tobemigrated" does not suggest any special handling of such resources. I must have mismatched this with the $resource->legacyfiles, RESOURCELIB_LEGACYFILES_ACTIVE and resourcelib_try_file_migration() system.

        Show
        David Mudrak added a comment - Hmm interesting. I lived in impression that the flag is still needed because of how the files migration was done on 1.9 => 2.0 upgrade. But apparently not - grepping for "tobemigrated" does not suggest any special handling of such resources. I must have mismatched this with the $resource->legacyfiles, RESOURCELIB_LEGACYFILES_ACTIVE and resourcelib_try_file_migration() system.
        Hide
        Andrew Davis added a comment - - edited

        Ive pushed the above changes to github.

        re #1 its become apparent that I dont understand either the moodle 2.0 file system or the imscp module. The reason I was referencing the course_files area in the code was because that is where the imscp zip file is stored in the 1.9 backup zip. Im afraid I dont know enough to make sense of the rest of what you're saying at this time. I'm fairly ill right now. I'll come back to it tomorrow. Hopefully it will make more sense then.

        Show
        Andrew Davis added a comment - - edited Ive pushed the above changes to github. re #1 its become apparent that I dont understand either the moodle 2.0 file system or the imscp module. The reason I was referencing the course_files area in the code was because that is where the imscp zip file is stored in the 1.9 backup zip. Im afraid I dont know enough to make sense of the rest of what you're saying at this time. I'm fairly ill right now. I'll come back to it tomorrow. Hopefully it will make more sense then.
        Hide
        Andrew Davis added a comment -

        This is all making a bit more sense today. I've pushed changes to github that get most of the resource types working. The only one that isnt is imscp. The problem is the structure field. In 2.x it contains a serialized php array that is built up by parsing the ims package. Once I figure out how to do that in the context of a restore we should be good to go.

        Show
        Andrew Davis added a comment - This is all making a bit more sense today. I've pushed changes to github that get most of the resource types working. The only one that isnt is imscp. The problem is the structure field. In 2.x it contains a serialized php array that is built up by parsing the ims package. Once I figure out how to do that in the context of a restore we should be good to go.
        Hide
        Andrew Davis added a comment -

        Ive pushed changes to mod/imscp/view.php that allows it to do parsing on the fly however its not working. That would seem to suggest my file migration code is still incorrect.

        Show
        Andrew Davis added a comment - Ive pushed changes to mod/imscp/view.php that allows it to do parsing on the fly however its not working. That would seem to suggest my file migration code is still incorrect.
        Hide
        Andrew Davis added a comment -

        Ive pushed changes that I think makes this all work I think.

        My commit history will need cleaning up and I suspect there is something a bit funny going on with the intro and content format in mod_page instances. It all more or less seems to work though.

        Show
        Andrew Davis added a comment - Ive pushed changes that I think makes this all work I think. My commit history will need cleaning up and I suspect there is something a bit funny going on with the intro and content format in mod_page instances. It all more or less seems to work though.
        Hide
        David Mudrak added a comment -

        How can mod/imscp/backup/moodle1/lib.php refer to some $context->id? There is not $context here.

        I believe that extract_to_storage() is not an option here - the conversion must happen within the temp directory, without affecting the $DB. Note that the context used during the conversion is not the context at the site where the conversion happens.

        The change in imscp_parse_structure() is not that bad. Maybe it would be better to move the shared logic into one lower level function and have two interfaces for it - one for the upgrade, one for the conversion. That would help to avoid changes in the upgrade.

        Show
        David Mudrak added a comment - How can mod/imscp/backup/moodle1/lib.php refer to some $context->id? There is not $context here. I believe that extract_to_storage() is not an option here - the conversion must happen within the temp directory, without affecting the $DB. Note that the context used during the conversion is not the context at the site where the conversion happens. The change in imscp_parse_structure() is not that bad. Maybe it would be better to move the shared logic into one lower level function and have two interfaces for it - one for the upgrade, one for the conversion. That would help to avoid changes in the upgrade.
        Hide
        Andrew Davis added a comment -

        I'm attaching a patch file for all of the work done on resource restore as I am currently experiencing some git related technical difficulties.

        Normal git based services will resume as soon as possible.

        Show
        Andrew Davis added a comment - I'm attaching a patch file for all of the work done on resource restore as I am currently experiencing some git related technical difficulties. Normal git based services will resume as soon as possible.
        Hide
        Andrew Davis added a comment -

        David, I believe I have now dealt with those issues. Integrate at will.

        Show
        Andrew Davis added a comment - David, I believe I have now dealt with those issues. Integrate at will.
        Hide
        Andrew Davis added a comment -

        Removing branch and diff URL information as it is currently not correct.

        Show
        Andrew Davis added a comment - Removing branch and diff URL information as it is currently not correct.
        Hide
        David Mudrak added a comment -

        Andrew, thanks for your work on this issue. I did a detailed review today and fixed/improved some bits. For your information:

        • I fixed the usage of the static array of successor handlers (that was my code and was not correct. Handlers can't be stored in static arrays because then they are shared by all instances of moodle1_converter. That might be also the reason of the weird behaviour you were experiencing during the unit tests).
        • Added a check in imscp that the manifest file really exists before trying to read its content (produced PHP warning) and refactorized the structure parsing.
        • Checking for URLs to file.php based on $CFG->wwwroot was completely wrong for two reasons. (1) Backup file already has these links encoded using $@FILEPHP@$ placeholder (2) Even if it did not have them encoded, we must support backups coming from other sites too so you couldn't compare with $CFG->wwwroot but the original_wwwroot stored in the backup.
        • There is no moddata/page/ to migrate, I was quite confused by that part
        • You used the original $data and dumped it into the XML files. We should respect the list of fields produced by normal 2.0 backup and convert to the same structures without legacy fields.
        • Some the parameters for the file conversion (like the file itemid) was not correct, fixed.
        • Fixed incorrect comments and copyright notices, copied without a change from other files

        Beside that, all resources now convert their files referenced from the intro field.

        Show
        David Mudrak added a comment - Andrew, thanks for your work on this issue. I did a detailed review today and fixed/improved some bits. For your information: I fixed the usage of the static array of successor handlers (that was my code and was not correct. Handlers can't be stored in static arrays because then they are shared by all instances of moodle1_converter. That might be also the reason of the weird behaviour you were experiencing during the unit tests). Added a check in imscp that the manifest file really exists before trying to read its content (produced PHP warning) and refactorized the structure parsing. Checking for URLs to file.php based on $CFG->wwwroot was completely wrong for two reasons. (1) Backup file already has these links encoded using $@FILEPHP@$ placeholder (2) Even if it did not have them encoded, we must support backups coming from other sites too so you couldn't compare with $CFG->wwwroot but the original_wwwroot stored in the backup. There is no moddata/page/ to migrate, I was quite confused by that part You used the original $data and dumped it into the XML files. We should respect the list of fields produced by normal 2.0 backup and convert to the same structures without legacy fields. Some the parameters for the file conversion (like the file itemid) was not correct, fixed. Fixed incorrect comments and copyright notices, copied without a change from other files Beside that, all resources now convert their files referenced from the intro field.
        Hide
        David Mudrak added a comment -

        Merged into the pre-integration branch.

        Show
        David Mudrak added a comment - Merged into the pre-integration branch.
        Hide
        Petr Škoda added a comment -

        this caused a fatal regression for imscp module, see MDL-29145

        Show
        Petr Škoda added a comment - this caused a fatal regression for imscp module, see MDL-29145

          People

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

            Dates

            • Created:
              Updated:
              Resolved: