Moodle
  1. Moodle
  2. MDL-32880

Make 1.9 blocks restorable in 2.3

    Details

    • Testing Instructions:
      Hide
      • Attempt to restore the attached Moodle 1.9 backup (backup-allblocks-20121120-1246.zip).
      • Confirm that it restores with no errors, and that all blocks (which exist in both 1.9 and 2.x) are present in the restored course.
      • Confirm that the Moodle logo image in the HTML block has been migrated (i.e. uses pluginfile.php rather than file.php).
      • Note that any blocks which store data outside the block config will be missing any such data (i.e. the rss_client block will not contain any feeds). This is because Moodle 1.9 does not back up that information, so it's not present in order to be restored. This behaviour is therefore consistent with restoring the 1.9 backup to a different instance of Moodle 1.9.
      Show
      Attempt to restore the attached Moodle 1.9 backup (backup-allblocks-20121120-1246.zip). Confirm that it restores with no errors, and that all blocks (which exist in both 1.9 and 2.x) are present in the restored course. Confirm that the Moodle logo image in the HTML block has been migrated (i.e. uses pluginfile.php rather than file.php). Note that any blocks which store data outside the block config will be missing any such data (i.e. the rss_client block will not contain any feeds). This is because Moodle 1.9 does not back up that information, so it's not present in order to be restored. This behaviour is therefore consistent with restoring the 1.9 backup to a different instance of Moodle 1.9.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32880_master_3
    • Rank:
      39948

      Description

      Blocks from a 1.9+ backup are not restored in Moodle 2.1 and higher (1.9 restore was unsupported previously). This is an extension of MDL-22414 and a narrowing of the now-closed MDL-32244. The roadmap (http://docs.moodle.org/dev/Roadmap) sort-of mentions this as a part of user data restoration but I haven't found a tracker item. Upgrading current production isn't an option for everyone, nor is cloning current production, upgrading that, and then doing a backup/restore.

      1. Course1.9-1of2.png
        343 kB
      2. course1.9-2of2.png
        221 kB
      3. restore-2.3-course.png
        217 kB
      4. restore2.3-selection_screen.png
        168 kB

        Issue Links

          Activity

          Hide
          Filip Benčo added a comment - - edited

          Can I ask you, in what stage is this issue, because I would like to help with its fixing.

          Show
          Filip Benčo added a comment - - edited Can I ask you, in what stage is this issue, because I would like to help with its fixing.
          Hide
          Mike Churchward added a comment -

          Is there a main Moodle tracker item for getting 1.9 block restores into Moodle 2?

          Show
          Mike Churchward added a comment - Is there a main Moodle tracker item for getting 1.9 block restores into Moodle 2?
          Hide
          Martin Dougiamas added a comment -

          I'm told that the 1.9 converter from MDL-22414 DOES actually already support blocks, but that core blocks need to have restore code written for each of them to make them work properly. From memory I don't think very many of the blocks actually have custom data (HTML block and RSS blocks being a big exception) so is the main problem that the block settings are not being restored?

          Can we have some more info from some of the watchers here about the exact scope of the problem?

          Show
          Martin Dougiamas added a comment - I'm told that the 1.9 converter from MDL-22414 DOES actually already support blocks, but that core blocks need to have restore code written for each of them to make them work properly. From memory I don't think very many of the blocks actually have custom data (HTML block and RSS blocks being a big exception) so is the main problem that the block settings are not being restored? Can we have some more info from some of the watchers here about the exact scope of the problem?
          Hide
          Mike Churchward added a comment -

          I will run another test, but when I last restored a 1.9 backup in the latest M2 code, no blocks appeared. The course just was restored with all course content but no blocks at all - not even the admin block.

          Show
          Mike Churchward added a comment - I will run another test, but when I last restored a 1.9 backup in the latest M2 code, no blocks appeared. The course just was restored with all course content but no blocks at all - not even the admin block.
          Hide
          Mike Churchward added a comment -

          Attaching screen shots from 1.9 to 2.3...
          "Course1.9-1of2.png" and "Course1.9-2of2.png" shows course in 1.9 with blocks.
          "restore2.3-selection_screen.png" shows where activities and blocks can be selected. Blocks are selected.
          "restore-2.3-course.png" shows the restored 2.3 course with only the navigation and 2,3 admin blocks. None of the 1.9 blocks are present.

          Would expect to see at least "People", "Activities", "Calendar", the HTML blocks, "Recent activity" and "Upcoming events".

          Show
          Mike Churchward added a comment - Attaching screen shots from 1.9 to 2.3... "Course1.9-1of2.png" and "Course1.9-2of2.png" shows course in 1.9 with blocks. "restore2.3-selection_screen.png" shows where activities and blocks can be selected. Blocks are selected. "restore-2.3-course.png" shows the restored 2.3 course with only the navigation and 2,3 admin blocks. None of the 1.9 blocks are present. Would expect to see at least "People", "Activities", "Calendar", the HTML blocks, "Recent activity" and "Upcoming events".
          Hide
          Mike Churchward added a comment -

          Replacing old files.

          Show
          Mike Churchward added a comment - Replacing old files.
          Hide
          David Mudrak added a comment -

          Hi all. Unfortunately I must confirm this - blocks are not converted by moodle1 converter yet. As you can see in testing instructions of MDL-22414, this was known. The list of that issue's subtasks illustrates that we really focused on activity modules only and since then, the whole project did not get much attention.

          However, the machinery is ready to be extended to convert blocks - pretty easily. Just off my head without diving into the code too much, I would suggest to follow a pattern similar to the one we used for Question bank conversion. Questions have some common data and then each question type can optionally have its own data included in the backup. Similarly, all blocks need some common data (instances, position etc) and then, some of them (just four or so actually) may need some callbacks.

          Looking at backup/converter/moodle1/handlerlib.php is a good start to see and understand the flow of the conversion. You may even notice the class moodle1_block_handler. The underlying machinery supports blocks in that sense that their XML paths are detected during the conversion - there is just no handler to deal with the data. This is also obvious from the restore log:

          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/PARTICIPANTS
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/ACTIVITY_MODULES
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/SEARCH_FORUMS
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/ADMIN
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/COURSE_LIST
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/NEWS_ITEMS
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/CALENDAR_UPCOMING
          [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/RECENT_ACTIVITY
          

          I'll be happy to assist anybody willing to work on this. Alternatively, I can work on this myself as soon as I finish my current DEV projects for Moodle 2.4.

          Show
          David Mudrak added a comment - Hi all. Unfortunately I must confirm this - blocks are not converted by moodle1 converter yet. As you can see in testing instructions of MDL-22414 , this was known. The list of that issue's subtasks illustrates that we really focused on activity modules only and since then, the whole project did not get much attention. However, the machinery is ready to be extended to convert blocks - pretty easily. Just off my head without diving into the code too much, I would suggest to follow a pattern similar to the one we used for Question bank conversion. Questions have some common data and then each question type can optionally have its own data included in the backup. Similarly, all blocks need some common data (instances, position etc) and then, some of them (just four or so actually) may need some callbacks. Looking at backup/converter/moodle1/handlerlib.php is a good start to see and understand the flow of the conversion. You may even notice the class moodle1_block_handler. The underlying machinery supports blocks in that sense that their XML paths are detected during the conversion - there is just no handler to deal with the data. This is also obvious from the restore log: [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/PARTICIPANTS [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/ACTIVITY_MODULES [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/SEARCH_FORUMS [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/ADMIN [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/COURSE_LIST [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/NEWS_ITEMS [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/CALENDAR_UPCOMING [Wed Aug 29 15:25:32 2012] [warn] (moodle1) no handler attached /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/RECENT_ACTIVITY I'll be happy to assist anybody willing to work on this. Alternatively, I can work on this myself as soon as I finish my current DEV projects for Moodle 2.4.
          Hide
          Mike Churchward added a comment -

          Hi David. I have resources I can throw at this. But can you give us a bit more direction?
          Looking in "backup/converter/moodle1/handlerlib.php", there is a class called "moodle1_block_handler", but it contains nothing.
          You mentioned to use the same machinery as the question bank. Is this what is contained in "moodle1_question_bank_handler"?

          The first pass at this would be to restore block instances with a course restore. Do we need to look at the course restore code?

          Show
          Mike Churchward added a comment - Hi David. I have resources I can throw at this. But can you give us a bit more direction? Looking in "backup/converter/moodle1/handlerlib.php", there is a class called "moodle1_block_handler", but it contains nothing. You mentioned to use the same machinery as the question bank. Is this what is contained in "moodle1_question_bank_handler"? The first pass at this would be to restore block instances with a course restore. Do we need to look at the course restore code?
          Hide
          Mike Churchward added a comment -

          Is there any kind of developer doc, that describes the basic layout or controller scheme of the backup/restore function? Something that indicates what is called and where the data gets set? (I know that's a big wish...)

          Show
          Mike Churchward added a comment - Is there any kind of developer doc, that describes the basic layout or controller scheme of the backup/restore function? Something that indicates what is called and where the data gets set? (I know that's a big wish...)
          Hide
          David Mudrak added a comment -

          ... and sometimes even big wishes can come true. The team working on moodle1 converters used http://docs.moodle.org/dev/Backup_1.9_conversion_for_developers as a reference for developers. I believe it provides a solid overview of the machinery and even though it was taking an activity module as the example, you may find it useful for blocks, too.

          Show
          David Mudrak added a comment - ... and sometimes even big wishes can come true. The team working on moodle1 converters used http://docs.moodle.org/dev/Backup_1.9_conversion_for_developers as a reference for developers. I believe it provides a solid overview of the machinery and even though it was taking an activity module as the example, you may find it useful for blocks, too.
          Hide
          Mike Churchward added a comment -

          Thank you!!

          Show
          Mike Churchward added a comment - Thank you!!
          Hide
          Mike Churchward added a comment -

          One more question... You refer to the "restore log". Where can I find this file when I do a restore?

          Show
          Mike Churchward added a comment - One more question... You refer to the "restore log". Where can I find this file when I do a restore?
          Hide
          David Mudrak added a comment -

          In a file moodledata/temp/backup/

          {restore-controller-id}

          .log

          Show
          David Mudrak added a comment - In a file moodledata/temp/backup/ {restore-controller-id} .log
          Hide
          Mike Churchward added a comment -

          Want to bounce some ideas off of people...

          Currently, there is a moodle1_block_handler class in the handlerlib.php file. This is used as a template to extend for all blocks in the same sense as the modules are handled.

          But, blocks all have the same basic backup/restore needs, and then may have specific extra needs on a per block basis. This is somewhat different than modules.

          I think it makes sense to use the moodle1_block_handler as the initial class to handle all blocks. Instead of registering a specific handler for each block in the moodle1_handlers_factory::get_handlers() function (as is done now), I think it makes more sense to register the moodle1_block_handler itself, and then let it manage the handlers for each block. Then it would contain the basic functions for all blocks, and add specific handlers when needed by a block.

          This means that the get_plugin_handlers function wouldn't be used for blocks as it is now. Although, I imagine the moodle1_block_handler class could still use that to set up its own list.

          Thoughts? I'll provide some sample code and changes I'm proposing, but I wanted to make sure that I was not completely off base.

          Show
          Mike Churchward added a comment - Want to bounce some ideas off of people... Currently, there is a moodle1_block_handler class in the handlerlib.php file. This is used as a template to extend for all blocks in the same sense as the modules are handled. But, blocks all have the same basic backup/restore needs, and then may have specific extra needs on a per block basis. This is somewhat different than modules. I think it makes sense to use the moodle1_block_handler as the initial class to handle all blocks. Instead of registering a specific handler for each block in the moodle1_handlers_factory::get_handlers() function (as is done now), I think it makes more sense to register the moodle1_block_handler itself, and then let it manage the handlers for each block. Then it would contain the basic functions for all blocks, and add specific handlers when needed by a block. This means that the get_plugin_handlers function wouldn't be used for blocks as it is now. Although, I imagine the moodle1_block_handler class could still use that to set up its own list. Thoughts? I'll provide some sample code and changes I'm proposing, but I wanted to make sure that I was not completely off base.
          Hide
          David Mudrak added a comment -

          You are definitely on the right track Mike. This is what I meant by using the similar pattern to the question bank. I would personally start with some sort of new moodle1_blocks_handler class that would be added to the list of returned handlers at the top of moodle1_handlers_factory::get_handlers(). Then you can decide to

          • either keep moodle1_handlers_factory::get_plugin_handlers() registering eventual block-specific handlers - I'm not sure that would be a good way as you noticed (blocks are really closer to questions than to mods when it comes to their conversion)
          • or remove the support for blocks from moodle1_handlers_factory::get_plugin_handlers() and let the new class moodle1_blocks_handler to process eventual callbacks to blocks themselves. In which case, I would keep moodle1_block_handler as an abstract common superclass for all such block-specific handlers.
          • or (as you noticed too) keep support for blocks in get_plugin_handlers() but let the new moodle1_blocks_handler to actually use it.

          Let us let the code examples speak.

          Show
          David Mudrak added a comment - You are definitely on the right track Mike. This is what I meant by using the similar pattern to the question bank. I would personally start with some sort of new moodle1_blocks_handler class that would be added to the list of returned handlers at the top of moodle1_handlers_factory::get_handlers(). Then you can decide to either keep moodle1_handlers_factory::get_plugin_handlers() registering eventual block-specific handlers - I'm not sure that would be a good way as you noticed (blocks are really closer to questions than to mods when it comes to their conversion) or remove the support for blocks from moodle1_handlers_factory::get_plugin_handlers() and let the new class moodle1_blocks_handler to process eventual callbacks to blocks themselves. In which case, I would keep moodle1_block_handler as an abstract common superclass for all such block-specific handlers. or (as you noticed too) keep support for blocks in get_plugin_handlers() but let the new moodle1_blocks_handler to actually use it. Let us let the code examples speak.
          Hide
          Paul Nicholls added a comment -

          Mike, that sounds like exactly what I was about to start doing. How are you going with this? If you've got some code you're able to share, I'd love to take a look; I don't want to waste time doing the same thing in parallel, but if I can be of assistance, please let me know.

          -Paul

          Show
          Paul Nicholls added a comment - Mike, that sounds like exactly what I was about to start doing. How are you going with this? If you've got some code you're able to share, I'd love to take a look; I don't want to waste time doing the same thing in parallel, but if I can be of assistance, please let me know. -Paul
          Hide
          Paul Nicholls added a comment - - edited

          I've had a crack at this myself, and implemented a basic handler which processes the main block details (excluding any files, role assignments and role overrides). I couldn't seem to convince a single handler to catch all blocks, so I took the approach of giving the abstract moodle1_block_handler a process_block() function which individual blocks can then leverage by extending moodle1_block_handler and providing a get_paths() function which attaches the generic process_block() function to its own path. This also makes it simple to add conversion support to custom/third-party blocks without requiring any changes to core code.

          My code is currently for 2.3, but should apply cleanly to other branches. It can currently be found in my Github repository - https://github.com/pauln/moodle/tree/MDL-32880_23 (MDL-32880_23 branch of git://github.com/pauln/moodle.git) - and any feedback is appreciated. I'm happy to apply it to other branches (i.e. master) and submit it for peer review, if there's agreement that it's a reasonable approach and a suitable first step (restoring blocks but skipping role assignments and overrides seems like a more desirable approach than just not restoring any blocks at all).

          If somebody else has the time and inclination to do so, I imagine it could also be used as a basis for the approach described above (having one handler process all blocks, handing over to specialised handlers where they exist) - but this approach seems to work well, and works for all blocks that I've tested (which, admittedly, is nowhere near all of the blocks I've added the generic handler for).

          -Paul

          P.S. Mike, I found your blog posts about this - and they certainly helped me get my head around some aspects of the process. Thanks!

          Show
          Paul Nicholls added a comment - - edited I've had a crack at this myself, and implemented a basic handler which processes the main block details (excluding any files, role assignments and role overrides). I couldn't seem to convince a single handler to catch all blocks, so I took the approach of giving the abstract moodle1_block_handler a process_block() function which individual blocks can then leverage by extending moodle1_block_handler and providing a get_paths() function which attaches the generic process_block() function to its own path. This also makes it simple to add conversion support to custom/third-party blocks without requiring any changes to core code. My code is currently for 2.3, but should apply cleanly to other branches. It can currently be found in my Github repository - https://github.com/pauln/moodle/tree/MDL-32880_23 ( MDL-32880 _23 branch of git://github.com/pauln/moodle.git) - and any feedback is appreciated. I'm happy to apply it to other branches (i.e. master) and submit it for peer review, if there's agreement that it's a reasonable approach and a suitable first step (restoring blocks but skipping role assignments and overrides seems like a more desirable approach than just not restoring any blocks at all). If somebody else has the time and inclination to do so, I imagine it could also be used as a basis for the approach described above (having one handler process all blocks, handing over to specialised handlers where they exist) - but this approach seems to work well, and works for all blocks that I've tested (which, admittedly, is nowhere near all of the blocks I've added the generic handler for). -Paul P.S. Mike, I found your blog posts about this - and they certainly helped me get my head around some aspects of the process. Thanks!
          Hide
          David Mudrak added a comment -

          Wow, thanks Paul for the code. I am pretty sure Mike will be happy to be able to refer to your implementation of moodle1_block_handler::process_block() to compare it with his findings. As I read your patch, you followed the same pattern as we did for activity modules (which is how it was originally planned). Now what Mike tries to do is to implement it in a way that all blocks (in-core as well as 3rd party) would be converted even if they have no explicit code added. That is, without requiring changes introduced in https://github.com/pauln/moodle/commit/409578fe405289b861e04d4e36036ac65c4c3f2d

          We (me and Mike) believe it is possible to achieve it in a pretty clean way. The plan is to have a new handler that would attach as a listener to all block paths. It would convert all common data (as you do in https://github.com/pauln/moodle/commit/76a7c44491120a696104cd5a4755890c1968777b) and then it would eventually dispatch the data chunks to block themselves in case they need to convert some internal data structures. The point is that like 75% (guessing) of blocks do not have custom data so the core handler should be able to do all work for them.

          There are existing examples how to convert files, so the support for them should not hurt. And of course we can't implement support for role assignments and overrides as we do not convert any user related data at all.

          Show
          David Mudrak added a comment - Wow, thanks Paul for the code. I am pretty sure Mike will be happy to be able to refer to your implementation of moodle1_block_handler::process_block() to compare it with his findings. As I read your patch, you followed the same pattern as we did for activity modules (which is how it was originally planned). Now what Mike tries to do is to implement it in a way that all blocks (in-core as well as 3rd party) would be converted even if they have no explicit code added . That is, without requiring changes introduced in https://github.com/pauln/moodle/commit/409578fe405289b861e04d4e36036ac65c4c3f2d We (me and Mike) believe it is possible to achieve it in a pretty clean way. The plan is to have a new handler that would attach as a listener to all block paths. It would convert all common data (as you do in https://github.com/pauln/moodle/commit/76a7c44491120a696104cd5a4755890c1968777b ) and then it would eventually dispatch the data chunks to block themselves in case they need to convert some internal data structures. The point is that like 75% (guessing) of blocks do not have custom data so the core handler should be able to do all work for them. There are existing examples how to convert files, so the support for them should not hurt. And of course we can't implement support for role assignments and overrides as we do not convert any user related data at all.
          Hide
          Paul Nicholls added a comment -

          Hi David,
          As I said, I couldn't seem to convince a single handler to listen to all block paths - so if you're able to do so, that'd certainly make for a cleaner implementation. I don't have the time to get intimately familiar with the conversion system, so maybe I'm missing something - but in the mean time, I went for the approach I could make work

          I hadn't thought about the lack of user-related data - thanks for pointing that out. I guess that just leaves us with files, then; I've been poking about at the file conversion stuff lately as I discovered a slight bug (failing to urldecode filenames pulled out of URLs), so I'll probably have a crack at that today.

          -Paul

          Show
          Paul Nicholls added a comment - Hi David, As I said, I couldn't seem to convince a single handler to listen to all block paths - so if you're able to do so, that'd certainly make for a cleaner implementation. I don't have the time to get intimately familiar with the conversion system, so maybe I'm missing something - but in the mean time, I went for the approach I could make work I hadn't thought about the lack of user-related data - thanks for pointing that out. I guess that just leaves us with files, then; I've been poking about at the file conversion stuff lately as I discovered a slight bug (failing to urldecode filenames pulled out of URLs), so I'll probably have a crack at that today. -Paul
          Hide
          Mike Churchward added a comment -

          Hi Paul
          Because I'm at the developers conference this week I haven't been able to follow up, but commenting on the comments, we definitely want to process all blocks by default, and have hooks for blocks that need specific processes. This means that a block does not need to have its own backup code at all (unlike modules). This process is similar to how Moodle handles blocks in the M2 backup process.

          I'll post my work soon.

          Show
          Mike Churchward added a comment - Hi Paul Because I'm at the developers conference this week I haven't been able to follow up, but commenting on the comments, we definitely want to process all blocks by default, and have hooks for blocks that need specific processes. This means that a block does not need to have its own backup code at all (unlike modules). This process is similar to how Moodle handles blocks in the M2 backup process. I'll post my work soon.
          Hide
          Paul Nicholls added a comment -

          Hi Mike,
          I'm definitely in favour of that approach. The code I posted still just has one core handler which processes all the blocks, so if you haven't already covered all the basics, you might find it of some use.

          I'll hold off on any further work on this until you've had a chance to sort things out - I look forward to seeing your code.

          -Paul

          Show
          Paul Nicholls added a comment - Hi Mike, I'm definitely in favour of that approach. The code I posted still just has one core handler which processes all the blocks, so if you haven't already covered all the basics, you might find it of some use. I'll hold off on any further work on this until you've had a chance to sort things out - I look forward to seeing your code. -Paul
          Hide
          Mike Churchward added a comment - - edited

          David / Paul -

          I have made commits to combine what Paul did with a generic block handler so that specific block convert code is not needed anywhere. Please review

          https://github.com/mchurchward/moodle/commit/5036ed80ad57c65c8cc18409e0b59307e0061c52

          and

          https://github.com/mchurchward/moodle/commit/b346f7e63c9f105dc27fc47707349b90e172ad52
          (you can ignore the block/participants part; that was an earlier test).

          How does this approach seem?

          Show
          Mike Churchward added a comment - - edited David / Paul - I have made commits to combine what Paul did with a generic block handler so that specific block convert code is not needed anywhere. Please review https://github.com/mchurchward/moodle/commit/5036ed80ad57c65c8cc18409e0b59307e0061c52 and https://github.com/mchurchward/moodle/commit/b346f7e63c9f105dc27fc47707349b90e172ad52 (you can ignore the block/participants part; that was an earlier test). How does this approach seem?
          Hide
          Paul Nicholls added a comment -

          Hi Mike,
          Looks good to me - and worked perfectly well on a test course.

          David, would you like to see this ported to 2.1 and/or 2.2, or would 2.3 and master be sufficient?

          -Paul

          Show
          Paul Nicholls added a comment - Hi Mike, Looks good to me - and worked perfectly well on a test course. David, would you like to see this ported to 2.1 and/or 2.2, or would 2.3 and master be sufficient? -Paul
          Hide
          Paul Nicholls added a comment -

          Actually, it might be worth holding off for a little longer - we've just encountered an issue: if there are multiple instances of the same block (i.e. HTML block or rss_client block) in a course, attempting to restore the course now fails with a useless error about a file already existing. I'm looking into this now.

          -Paul

          Show
          Paul Nicholls added a comment - Actually, it might be worth holding off for a little longer - we've just encountered an issue: if there are multiple instances of the same block (i.e. HTML block or rss_client block) in a course, attempting to restore the course now fails with a useless error about a file already existing. I'm looking into this now. -Paul
          Hide
          Mike Churchward added a comment -

          There is still more work to be done anyway... There are specific blocks (such as rss_client) that needs specific conversion code for its non-standard data. Also, there will be a few changes to the provided code.

          But if this approach looks good, and David agrees, this should be easy to complete.

          Show
          Mike Churchward added a comment - There is still more work to be done anyway... There are specific blocks (such as rss_client) that needs specific conversion code for its non-standard data. Also, there will be a few changes to the provided code. But if this approach looks good, and David agrees, this should be easy to complete.
          Hide
          Mike Churchward added a comment -

          Found the multiple instances of blocks problem... Needed to name the XML files in process_block with a unique identifier.

          Change is in https://github.com/mchurchward/moodle/commit/6a3045e78f3b814a72181121ab748f622c88040f. There is also some fixes where "currentmod" was being used in block code instead of "currentblock". Not sure what problem it would have caused though.

          Show
          Mike Churchward added a comment - Found the multiple instances of blocks problem... Needed to name the XML files in process_block with a unique identifier. Change is in https://github.com/mchurchward/moodle/commit/6a3045e78f3b814a72181121ab748f622c88040f . There is also some fixes where "currentmod" was being used in block code instead of "currentblock". Not sure what problem it would have caused though.
          Hide
          Paul Nicholls added a comment -

          I'd just made the same change for multiple instances (though I used $data['id'] rather than $instanceid - but they're the same thing). I'm now reverting and pulling your new commit into my tree.

          Have you started on the rss_client block yet? If not, I'll dive in now - we'd rather like it to work ASAP, as we're trying to get a few courses from our 1.9 instance into our 2.3 instance, some of which include rss_client blocks...

          -Paul

          Show
          Paul Nicholls added a comment - I'd just made the same change for multiple instances (though I used $data ['id'] rather than $instanceid - but they're the same thing). I'm now reverting and pulling your new commit into my tree. Have you started on the rss_client block yet? If not, I'll dive in now - we'd rather like it to work ASAP, as we're trying to get a few courses from our 1.9 instance into our 2.3 instance, some of which include rss_client blocks... -Paul
          Hide
          Mike Churchward added a comment -

          I haven't, no... Was going to look at it tonight, but I may not get to until tomorrow.

          Show
          Mike Churchward added a comment - I haven't, no... Was going to look at it tonight, but I may not get to until tomorrow.
          Hide
          Paul Nicholls added a comment -

          Ok - I'll dive in now, then. I'll post here with any progress.

          Show
          Paul Nicholls added a comment - Ok - I'll dive in now, then. I'll post here with any progress.
          Hide
          Paul Nicholls added a comment -

          I don't think that the 1.9 backups actually contain the list of feeds; even restoring a 1.9 backup to a different 1.9 site results in a block with no feeds chosen (and does not add any feeds - either shared or local). As such, I've implemented a handler which creates a shell rss_client.xml which contains no feeds; this results in identical behaviour to restoring that which I just described.

          The new handler is in https://github.com/pauln/moodle/commit/d89ade28060ae858356ff4a42cf946a4c99878c0.

          In the process of doing this, I discovered that there was a slight flaw in your logic with the generic block handler - if a block attempted to provide its own handler, conversion would halt with a "missing handler" error, because the file wasn't being included. I've fixed this in https://github.com/pauln/moodle/commit/630756fe1063e96cc7a6057bd512aa4afa53bc61.

          I've got a branch up at https://github.com/pauln/moodle/tree/MDL-32880_23_2 which should hopefully contain everything required to get to where I'm at now - i.e. generic block handler, able to handle multiple instances of the same block, with custom rss_client handler that creates shell rss_client.xml files.

          -Paul

          Show
          Paul Nicholls added a comment - I don't think that the 1.9 backups actually contain the list of feeds; even restoring a 1.9 backup to a different 1.9 site results in a block with no feeds chosen (and does not add any feeds - either shared or local). As such, I've implemented a handler which creates a shell rss_client.xml which contains no feeds; this results in identical behaviour to restoring that which I just described. The new handler is in https://github.com/pauln/moodle/commit/d89ade28060ae858356ff4a42cf946a4c99878c0 . In the process of doing this, I discovered that there was a slight flaw in your logic with the generic block handler - if a block attempted to provide its own handler, conversion would halt with a "missing handler" error, because the file wasn't being included. I've fixed this in https://github.com/pauln/moodle/commit/630756fe1063e96cc7a6057bd512aa4afa53bc61 . I've got a branch up at https://github.com/pauln/moodle/tree/MDL-32880_23_2 which should hopefully contain everything required to get to where I'm at now - i.e. generic block handler, able to handle multiple instances of the same block, with custom rss_client handler that creates shell rss_client.xml files. -Paul
          Hide
          Mike Churchward added a comment -

          Thanks Paul. I'll look at the code you supplied. Good catch on the "missing handler" mistake I made.

          Show
          Mike Churchward added a comment - Thanks Paul. I'll look at the code you supplied. Good catch on the "missing handler" mistake I made.
          Hide
          Mike Churchward added a comment -

          It looks like there are absolutely no core blocks that backup/restore any of their own data. In 1.9, if there was, they would use backuprestore_instancedata_used() and instance_backup($bf, $preferences). I'll have to look for a contrib example to make sure this all works.

          Show
          Mike Churchward added a comment - It looks like there are absolutely no core blocks that backup/restore any of their own data. In 1.9, if there was, they would use backuprestore_instancedata_used() and instance_backup($bf, $preferences). I'll have to look for a contrib example to make sure this all works.
          Hide
          Mike Churchward added a comment -

          So I thought we should be able to do this with specific instances of "get_paths" and specific process functions. An example of what I thought should work is here: https://github.com/mchurchward/moodle/commit/1012e75e1d018d761eb0c551c5fed05e372376f9

          But, my "process_rss_client_feeds" is not being called, even though it is loaded in the paths... Any ideas?

          Show
          Mike Churchward added a comment - So I thought we should be able to do this with specific instances of "get_paths" and specific process functions. An example of what I thought should work is here: https://github.com/mchurchward/moodle/commit/1012e75e1d018d761eb0c551c5fed05e372376f9 But, my "process_rss_client_feeds" is not being called, even though it is loaded in the paths... Any ideas?
          Hide
          Paul Nicholls added a comment -

          Hi Mike,
          Your code appears to be trying to latch on to tags which 1.9 doesn't put there ("feed" tags nested within a "feed" tag within the rss_client "block" tag in moodle.xml) - have you created a 1.9 backup which contains these extra tags in order to test it, and are you definitely using that backup for your testing? I shudder to think how many times I've wondered why something's not working, only to realise I was using the wrong test data

          -Paul

          Show
          Paul Nicholls added a comment - Hi Mike, Your code appears to be trying to latch on to tags which 1.9 doesn't put there ("feed" tags nested within a "feed" tag within the rss_client "block" tag in moodle.xml) - have you created a 1.9 backup which contains these extra tags in order to test it, and are you definitely using that backup for your testing? I shudder to think how many times I've wondered why something's not working, only to realise I was using the wrong test data -Paul
          Hide
          David Mudrak added a comment -

          Hi guys, and thanks for all your work on this.

          Mike, can you please update your branch MOODLE_23_STABLE at github? It is a bit outdated at the moment and I would like to review your patchset at https://github.com/mchurchward/moodle/compare/MOODLE_23_STABLE...MOODLE_23_STABLE_MDL-32880 Thanks in advance (and yes, I know I could fetch the branch to my notebook and review it locally but for a quick review and in-line commenting, I really found github's interface useful).

          With regards to backporting - if we manage to have this ready before the 2.4 release, then this should imho go into 2.2 and 2.3 as we maintain two most recent stable branches. Integrators now accept just security fixes for 2.1.

          Show
          David Mudrak added a comment - Hi guys, and thanks for all your work on this. Mike, can you please update your branch MOODLE_23_STABLE at github? It is a bit outdated at the moment and I would like to review your patchset at https://github.com/mchurchward/moodle/compare/MOODLE_23_STABLE...MOODLE_23_STABLE_MDL-32880 Thanks in advance (and yes, I know I could fetch the branch to my notebook and review it locally but for a quick review and in-line commenting, I really found github's interface useful). With regards to backporting - if we manage to have this ready before the 2.4 release, then this should imho go into 2.2 and 2.3 as we maintain two most recent stable branches. Integrators now accept just security fixes for 2.1.
          Hide
          Mike Churchward added a comment -

          David - MOODLE_23_STABLE branch on github is now up to date.

          Paul - I know the tags aren't there, but I believe the converter should still be calling the rss handler functions. What happens now is that when I get to the restore part, it errors out because it cannot find the "blocks/rss_client_1543/rss_client.xml" file, which should have been created (empty) in the rss handler. The way you did it, that file did exist.
          It seems that the new "process_rss_client_feeds" function does not get called at conversion time - you think that is because that path doesn't exist in the backup file? If so, why does the restore process look for it?

          Show
          Mike Churchward added a comment - David - MOODLE_23_STABLE branch on github is now up to date. Paul - I know the tags aren't there, but I believe the converter should still be calling the rss handler functions. What happens now is that when I get to the restore part, it errors out because it cannot find the "blocks/rss_client_1543/rss_client.xml" file, which should have been created (empty) in the rss handler. The way you did it, that file did exist. It seems that the new "process_rss_client_feeds" function does not get called at conversion time - you think that is because that path doesn't exist in the backup file? If so, why does the restore process look for it?
          Hide
          Mike Churchward added a comment -

          Paul... So, are you saying:

          • the conversion process needs the presence of FEEDS/FEED in a 1.9 backup file in order to trigger the call to "process_rss_client_feeds"
          • the M2 restore process expects to find an "rss_client.xml" file regardless of data within it?
          Show
          Mike Churchward added a comment - Paul... So, are you saying: the conversion process needs the presence of FEEDS/FEED in a 1.9 backup file in order to trigger the call to "process_rss_client_feeds" the M2 restore process expects to find an "rss_client.xml" file regardless of data within it?
          Hide
          Mike Churchward added a comment - - edited

          I have confirmed both above statements by creating a <FEEDS><FEED></FEED></FEEDS> structure in a 1.9 backup...

          Show
          Mike Churchward added a comment - - edited I have confirmed both above statements by creating a <FEEDS><FEED></FEED></FEEDS> structure in a 1.9 backup...
          Hide
          Mike Churchward added a comment -

          Okay, so the latest code in https://github.com/mchurchward/moodle/commits/MOODLE_23_STABLE_MDL-32880 should contain all that is necessary for core block restore. There does not appear to be any core blocks in 1.9, that provide any other backup data, but we should verify by testing with all core 1.9 blocks. I'll do a test later.

          If anyone is interested, I did a version of the rss_client block that would convert data if 1.9 actually provided it. I tested this with a hand modified 1.9 XML backup file. That version is located here: https://github.com/mchurchward/moodle/commit/055612fcf5c0d52aae029a4959134ef11e8283a3

          Show
          Mike Churchward added a comment - Okay, so the latest code in https://github.com/mchurchward/moodle/commits/MOODLE_23_STABLE_MDL-32880 should contain all that is necessary for core block restore. There does not appear to be any core blocks in 1.9, that provide any other backup data, but we should verify by testing with all core 1.9 blocks. I'll do a test later. If anyone is interested, I did a version of the rss_client block that would convert data if 1.9 actually provided it. I tested this with a hand modified 1.9 XML backup file. That version is located here: https://github.com/mchurchward/moodle/commit/055612fcf5c0d52aae029a4959134ef11e8283a3
          Hide
          Paul Nicholls added a comment -

          Hi Mike,
          Just to clarify, the restore process isn't what's looking for the FEEDS/FEED path, but rather the converter (which just so happens to be running as part of the restore process). The converter wasn't finding the FEEDS/FEED path, and therefore wasn't calling process_rss_client_feeds - which meant that rss_client.xml wasn't created. The 2.x restore process then came along and ran the rss_client block's restore code, which expected rss_client.xml to be present - whether or not it contained any feeds - and halted with the message about rss_client.xml being missing.

          I also noticed that I'd somehow managed to end up with a mix of tabs and spaces for indentation in the rss_client block's handler, so I've added a commit to my branch and ported the whole lot over to MOODLE_22_STABLE and master, since we've (hopefully) reached a good point. They're a little cleaner than your branch in terms of commits (i.e. your early "playing with ideas" commits), but have the same end result (other than the indentation tweaks I just added to mine), so are probably a nicer candidate for integration.

          I've just done a very quick test of all core 1.9 blocks - I created a new course on a 1.9 instance, added all the blocks I could, configured a few (such as creating and choosing a quiz for the quiz results block and a glossary for the random glossary entry block), backed it up and restored it into a copy of my MDL-32880_master_2 branch as it currently stands. Everything seems to have worked as well as can be expected - no errors, but anything like the rss_client block which has its own data that isn't stored as block config will be missing data (if there are any that do this other than rss_client, they're blocks I'm not terribly familiar with and therefore haven't tested thoroughly). As such, since it does not appear to break anything, I'm going to submit this for peer review with some very basic testing instructions (and a copy of my quick test course).

          As far as the rss_client block (and any others which store data outside the block config), I wonder if perhaps a suitable approach for them would be to publicly provide patches which can be applied to a 1.9 instance in order to make it back up the relevant data, coupled with custom block handlers which can be dropped in to a recent enough 2.x (i.e. one that has the code from this issue), which can then be applied by anyone who needs to transfer that data from 1.9 to 2.x via backups.

          -Paul

          Show
          Paul Nicholls added a comment - Hi Mike, Just to clarify, the restore process isn't what's looking for the FEEDS/FEED path, but rather the converter (which just so happens to be running as part of the restore process). The converter wasn't finding the FEEDS/FEED path, and therefore wasn't calling process_rss_client_feeds - which meant that rss_client.xml wasn't created. The 2.x restore process then came along and ran the rss_client block's restore code, which expected rss_client.xml to be present - whether or not it contained any feeds - and halted with the message about rss_client.xml being missing. I also noticed that I'd somehow managed to end up with a mix of tabs and spaces for indentation in the rss_client block's handler, so I've added a commit to my branch and ported the whole lot over to MOODLE_22_STABLE and master, since we've (hopefully) reached a good point. They're a little cleaner than your branch in terms of commits (i.e. your early "playing with ideas" commits), but have the same end result (other than the indentation tweaks I just added to mine), so are probably a nicer candidate for integration. I've just done a very quick test of all core 1.9 blocks - I created a new course on a 1.9 instance, added all the blocks I could, configured a few (such as creating and choosing a quiz for the quiz results block and a glossary for the random glossary entry block), backed it up and restored it into a copy of my MDL-32880 _master_2 branch as it currently stands. Everything seems to have worked as well as can be expected - no errors, but anything like the rss_client block which has its own data that isn't stored as block config will be missing data (if there are any that do this other than rss_client, they're blocks I'm not terribly familiar with and therefore haven't tested thoroughly). As such, since it does not appear to break anything, I'm going to submit this for peer review with some very basic testing instructions (and a copy of my quick test course). As far as the rss_client block (and any others which store data outside the block config), I wonder if perhaps a suitable approach for them would be to publicly provide patches which can be applied to a 1.9 instance in order to make it back up the relevant data, coupled with custom block handlers which can be dropped in to a recent enough 2.x (i.e. one that has the code from this issue), which can then be applied by anyone who needs to transfer that data from 1.9 to 2.x via backups. -Paul
          Hide
          Paul Nicholls added a comment -

          Assigned to me so that I could submit it for peer review.

          Show
          Paul Nicholls added a comment - Assigned to me so that I could submit it for peer review.
          Hide
          Mike Churchward added a comment - - edited

          Don't know if its any easier, but I squashed everything down to one commit here: https://github.com/mchurchward/moodle/commits/MDL-32880_2.3

          (edit - don't use this one as it has removed all of the contributors' credits... Paul's name would be missing)

          Show
          Mike Churchward added a comment - - edited Don't know if its any easier, but I squashed everything down to one commit here: https://github.com/mchurchward/moodle/commits/MDL-32880_2.3 (edit - don't use this one as it has removed all of the contributors' credits... Paul's name would be missing)
          Hide
          Dan Poltawski added a comment -

          David, are you able to review this one?

          Show
          Dan Poltawski added a comment - David, are you able to review this one?
          Hide
          David Mudrak added a comment -

          I'm on it

          Show
          David Mudrak added a comment - I'm on it
          Hide
          David Mudrak added a comment -

          Thanks for you work Mike and Paul. It's looking good.

          (1) So, if I understand it correctly: Imagine there is a custom block_foo that adds its custom data in Moodle 1.x ZIP format in a path like /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/FOO/INSTANCEDATA/BARS/BAR (generated via instance_backup() method in 1.x). The correct procedure for writing a converter for such a block would be to override the get_paths() method so that it records both 'block' path and the custom 'bar' path. And then implement both process_block() and process_bar() given that it would probably use the parent::process_block() method. Am I right?

          (2) I believe that the

          $newdata['subpagepattern'] = '$@NULL@$';
          

          could (and should) read

          $newdata['subpagepattern'] = null;
          

          as the actual representation of null values in MBZ is an implementation detail (and the XML writer might on day eventually even escape such string so that the literal '$@NULL@$' string and not the null value would be restored).

          (3) When converting the HTML block with embedded images, Legacy course files are used (the image is still referred via file.php and not pluginfile.php). What's worse is that the block itself does not have a chance to do it properly (unless I'm missing something). There seems to be no easy way*) how to generate the inforef.xml as the parent::process_block() does it. What about refactoring moodle1_block_handler class a bit yet to support this?

          (4) In 1.9, the value of <CONFIGDATA> element are obtained via get_backup_encoded_config(). So blocks are those who decide about the format and the interpretation of the value. If there was a hypothetical block that would change the format between 1.x and 2.x, it again has no chance*) to fix the value. Again, I can imagine a small refactoring that would give subclasses a chance to override the default behaviour.

          *) They would need to override whole process_block() which sucks obviously. What about splitting process_block() into a set of protected methods so that subclasses could easily override just bits they are interested in? E.g.

          $newdata = $this->convert_common_block_data($data);
          $this->write_block_xml($newdata, $data);
          $this->write_inforef_xml($newdata, $data);
          $this->write_roles_xml($newdata, $data);
          

          where the convert_common_block_data() method would have something like

          $newdata['configdata'] = $this->convert_configdata($data);
          

          and moodle1_block_handler::convert_configdata() would simply do the default

          protected function convert_configdata(array $olddata) {
              return $olddata['configdata'];
          }
          

          ... or something like that. I hope you see the point.

          I'll be happy to discuss eventual contra-proposals or objections against these suggestions. Once more, thanks for your work on this!

          Show
          David Mudrak added a comment - Thanks for you work Mike and Paul. It's looking good. (1) So, if I understand it correctly: Imagine there is a custom block_foo that adds its custom data in Moodle 1.x ZIP format in a path like /MOODLE_BACKUP/COURSE/BLOCKS/BLOCK/FOO/INSTANCEDATA/BARS/BAR (generated via instance_backup() method in 1.x). The correct procedure for writing a converter for such a block would be to override the get_paths() method so that it records both 'block' path and the custom 'bar' path. And then implement both process_block() and process_bar() given that it would probably use the parent::process_block() method. Am I right? (2) I believe that the $newdata['subpagepattern'] = '$@NULL@$'; could (and should) read $newdata['subpagepattern'] = null ; as the actual representation of null values in MBZ is an implementation detail (and the XML writer might on day eventually even escape such string so that the literal '$@NULL@$' string and not the null value would be restored). (3) When converting the HTML block with embedded images, Legacy course files are used (the image is still referred via file.php and not pluginfile.php). What's worse is that the block itself does not have a chance to do it properly (unless I'm missing something). There seems to be no easy way*) how to generate the inforef.xml as the parent::process_block() does it. What about refactoring moodle1_block_handler class a bit yet to support this? (4) In 1.9, the value of <CONFIGDATA> element are obtained via get_backup_encoded_config(). So blocks are those who decide about the format and the interpretation of the value. If there was a hypothetical block that would change the format between 1.x and 2.x, it again has no chance*) to fix the value. Again, I can imagine a small refactoring that would give subclasses a chance to override the default behaviour. *) They would need to override whole process_block() which sucks obviously. What about splitting process_block() into a set of protected methods so that subclasses could easily override just bits they are interested in? E.g. $newdata = $ this ->convert_common_block_data($data); $ this ->write_block_xml($newdata, $data); $ this ->write_inforef_xml($newdata, $data); $ this ->write_roles_xml($newdata, $data); where the convert_common_block_data() method would have something like $newdata['configdata'] = $ this ->convert_configdata($data); and moodle1_block_handler::convert_configdata() would simply do the default protected function convert_configdata(array $olddata) { return $olddata['configdata']; } ... or something like that. I hope you see the point. I'll be happy to discuss eventual contra-proposals or objections against these suggestions. Once more, thanks for your work on this!
          Hide
          Mike Churchward added a comment - - edited

          Hi David -

          1. Regarding your first question about a converter for a block that has its own instance data, I created an example here: https://github.com/mchurchward/moodle/commit/055612fcf5c0d52aae029a4959134ef11e8283a3
          2. I tested the 'null' suggestion, and it works.
          3. I think we convinced ourselves that file conversion wasn't necessary... I'll look into this.
          4. I see your point, but will there actually be any examples of blocks doing this? I'd hate to make it more complicated if there are no examples that would actually do that. Have you seen any contributed blocks that have changes their configuration data from one method to another?
          Show
          Mike Churchward added a comment - - edited Hi David - Regarding your first question about a converter for a block that has its own instance data, I created an example here: https://github.com/mchurchward/moodle/commit/055612fcf5c0d52aae029a4959134ef11e8283a3 I tested the 'null' suggestion, and it works. I think we convinced ourselves that file conversion wasn't necessary... I'll look into this. I see your point, but will there actually be any examples of blocks doing this? I'd hate to make it more complicated if there are no examples that would actually do that. Have you seen any contributed blocks that have changes their configuration data from one method to another?
          Hide
          David Mudrak added a comment -

          Hi Mike

          (1) Thanks for the confirmation. However, I can see some discrepancies in your example. Firstly, the method on_rss_client_feeds_start() tries to open the file rss_client.xml. I can't remember the behaviour of the underlying xml_writer class at the moment but it would either throw exception or overwrite the file previously generated by parent::process_block(). In both cases, this is not what we want. Yet another reason to refactor process_block() yet imho.

          (2) Yes, and it is how it should be used.

          (3) Well, in that case, blocks would be sort of exception. Elsewhere in moodle1 code, we tried hard to convert files properly. The point is that the moodle1 converter should produce same MBZ that moodle2 backup code would produce, matching as much as possible. There are pretty clever and useful methods for this file handling so I believe it would not be hard. Please let me know if you want to do it or whether I should take over your patchset and add the support for files conversion on top of it.

          (4) Right. I appreciate your pragmatic attitude. Here we have a patch that does what is needed for this issue. Cool. But you are also designing an API here. An API that is supposed to be robust, solid and stable. Surely we do not want to change the interface of these methods here just because we realize that block_html or some other common block can't convert its data. I believe that when it comes to designing APIs, developers must be really picky and perfectionists. Another (and very important!) point is that currently, process_block() would be difficult to unit-test. If it is split into a chain of protected methods, we can easily prepare unit tests for various scenarios. The case I described about the change format was not probably the best one, but still ...

          Show
          David Mudrak added a comment - Hi Mike (1) Thanks for the confirmation. However, I can see some discrepancies in your example. Firstly, the method on_rss_client_feeds_start() tries to open the file rss_client.xml. I can't remember the behaviour of the underlying xml_writer class at the moment but it would either throw exception or overwrite the file previously generated by parent::process_block(). In both cases, this is not what we want. Yet another reason to refactor process_block() yet imho. (2) Yes, and it is how it should be used. (3) Well, in that case, blocks would be sort of exception. Elsewhere in moodle1 code, we tried hard to convert files properly. The point is that the moodle1 converter should produce same MBZ that moodle2 backup code would produce, matching as much as possible. There are pretty clever and useful methods for this file handling so I believe it would not be hard. Please let me know if you want to do it or whether I should take over your patchset and add the support for files conversion on top of it. (4) Right. I appreciate your pragmatic attitude. Here we have a patch that does what is needed for this issue. Cool. But you are also designing an API here. An API that is supposed to be robust, solid and stable. Surely we do not want to change the interface of these methods here just because we realize that block_html or some other common block can't convert its data. I believe that when it comes to designing APIs, developers must be really picky and perfectionists. Another (and very important!) point is that currently, process_block() would be difficult to unit-test. If it is split into a chain of protected methods, we can easily prepare unit tests for various scenarios. The case I described about the change format was not probably the best one, but still ...
          Hide
          Paul Nicholls added a comment -

          Hi David,
          Just on the point of the rss_client converter, note that rss_client.xml is a file specific to the (2.x) rss_client block - so it can't conflict with the file created by parent::process_block() as the main file generated by that method is always called block.xml (it's not named after the specific block). If the file already existed, an exception would indeed be thrown.

          As for the flexibility, I think it wouldn't hurt to refactor it a little - but let's not go overboard. As Mike suggested, there's no point in going further than necessary to support real-world blocks; if we tried to code for every hypothetical case, we'd never get to a point where we actually released this feature!

          -Paul

          Show
          Paul Nicholls added a comment - Hi David, Just on the point of the rss_client converter, note that rss_client.xml is a file specific to the (2.x) rss_client block - so it can't conflict with the file created by parent::process_block() as the main file generated by that method is always called block.xml (it's not named after the specific block). If the file already existed, an exception would indeed be thrown. As for the flexibility, I think it wouldn't hurt to refactor it a little - but let's not go overboard. As Mike suggested, there's no point in going further than necessary to support real-world blocks; if we tried to code for every hypothetical case, we'd never get to a point where we actually released this feature! -Paul
          Hide
          David Mudrak added a comment -

          (1) Ah! Right! Sorry I had to look at the wrong place for the name of the file. Now it's clear. Thanks.

          (2) I definitely agree. Let me suggest that we finish the file conversion yet (with block_html using it) and introduce a way how blocks might have control over their configdata format. Once that is done, I'll be very happy to give this +1 and pass it for integration. Would you like to work on it yet?

          Show
          David Mudrak added a comment - (1) Ah! Right! Sorry I had to look at the wrong place for the name of the file. Now it's clear. Thanks. (2) I definitely agree. Let me suggest that we finish the file conversion yet (with block_html using it) and introduce a way how blocks might have control over their configdata format. Once that is done, I'll be very happy to give this +1 and pass it for integration. Would you like to work on it yet?
          Hide
          Paul Nicholls added a comment - - edited

          That sounds good to me. I'll work on file conversion and configdata today.

          Update: As is so often the way, I got caught up with other (higher priority) work. I've managed to refactor the handler, but I don't expect to have enough time to do the file conversion stuff until tomorrow (it doesn't help that my dev site takes quite a while to restore the course I'm using for testing).

          Show
          Paul Nicholls added a comment - - edited That sounds good to me. I'll work on file conversion and configdata today. Update: As is so often the way, I got caught up with other (higher priority) work. I've managed to refactor the handler, but I don't expect to have enough time to do the file conversion stuff until tomorrow (it doesn't help that my dev site takes quite a while to restore the course I'm using for testing).
          Hide
          Paul Nicholls added a comment -

          I've got a working block_html handler, which migrates referenced files. I'm now working on tidying things up a little, and it'd be nice if I could squash it down to three commits (one for the core handler, plus one for each of the rss_client and html block handlers) - Mike, would you mind if I squash the remaining commits, mentioning you in the commit message as a contributor?

          Show
          Paul Nicholls added a comment - I've got a working block_html handler, which migrates referenced files. I'm now working on tidying things up a little, and it'd be nice if I could squash it down to three commits (one for the core handler, plus one for each of the rss_client and html block handlers) - Mike, would you mind if I squash the remaining commits, mentioning you in the commit message as a contributor?
          Hide
          Mike Churchward added a comment -

          Paul - go ahead and squash.

          Show
          Mike Churchward added a comment - Paul - go ahead and squash.
          Hide
          Paul Nicholls added a comment -

          New branches with commits squashed down to 3 (generic handler / supporting code, block_html handler, block_rss_client handler).

          Show
          Paul Nicholls added a comment - New branches with commits squashed down to 3 (generic handler / supporting code, block_html handler, block_rss_client handler).
          Hide
          Paul Nicholls added a comment -

          Updated testing instructions to cover HTML block file migration.

          Show
          Paul Nicholls added a comment - Updated testing instructions to cover HTML block file migration.
          Hide
          David Mudrak added a comment -

          Yay! Congrats and thanks for the new patch. I think it's OK now. I am going to submit it for integration.

          Show
          David Mudrak added a comment - Yay! Congrats and thanks for the new patch. I think it's OK now. I am going to submit it for integration.
          Hide
          David Mudrak added a comment -

          DEAR INTEGRATORS,

          There are some formal tiny details that could be improved yet (most notably missing phpDocs blocks for methods) but I will create a separate issue for that and will eventually fix it someday. I know that the integration team is (and should be!) pretty picky when it comes to following guidelines but I believe that for demanded features like this, with a patch provided by community members (outside the HQ) we should tolerate such imperfectnesses. Paul and Mike did a good job and we should integrate this IMHO.

          Show
          David Mudrak added a comment - DEAR INTEGRATORS, There are some formal tiny details that could be improved yet (most notably missing phpDocs blocks for methods) but I will create a separate issue for that and will eventually fix it someday. I know that the integration team is (and should be!) pretty picky when it comes to following guidelines but I believe that for demanded features like this, with a patch provided by community members (outside the HQ) we should tolerate such imperfectnesses. Paul and Mike did a good job and we should integrate this IMHO.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24 and master - and, exceptionally, also to 23 as far as the solution is 100% the same). No way to back port this to 1-year old stable.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24 and master - and, exceptionally, also to 23 as far as the solution is 100% the same). No way to back port this to 1-year old stable. Thanks!
          Hide
          Mark Nelson added a comment -

          Works as expected. Thanks for your contribution!

          Show
          Mark Nelson added a comment - Works as expected. Thanks for your contribution!
          Hide
          Mark Nelson added a comment -

          There was a warning that displayed when restoring the attached file, but it is not a regression caused by this patch, so created a separate issue and linked it.

          Show
          Mark Nelson added a comment - There was a warning that displayed when restoring the attached file, but it is not a regression caused by this patch, so created a separate issue and linked it.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

            People

            • Votes:
              23 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: