Moodle
  1. Moodle
  2. MDL-27822

Module version number from backup file should be available in restore_activity_structure_step::define_structure()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.0.4
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      Just need to test that this does not cause any fatal errors on restore of anything.

      If you really want to test this, add debug code like
      print_object($this->task->get_old_module_version()); // DONOTCOMMIT
      to restore_quiz_activity_structure_step::define_structure()

      Show
      Just need to test that this does not cause any fatal errors on restore of anything. If you really want to test this, add debug code like print_object($this->task->get_old_module_version()); // DONOTCOMMIT to restore_quiz_activity_structure_step::define_structure()
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17499

      Description

      Obviously, as time passes, activity modules can have their database structure changed. In this situation, when the module it restored, it is useful to know which version of the module wrote the backup file.

      For this reason, the version number of the module is written into the backup file into a location like activities/quiz_123/module.xml. Brilliant!

      Unfortunately, this information is not made available in restore_activity_structure_step::define_structure(), which is where it is needed

      What are the issues with making it available there?

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I thought I was on the trail of solving this, because most of the attributes from module.xml seemed to be in restore_quiz_activity_task->info.

          However, it turns out that that data is loaded from the top level moodle_backup.xml - which does not contain the version information

          Show
          Tim Hunt added a comment - I thought I was on the trail of solving this, because most of the attributes from module.xml seemed to be in restore_quiz_activity_task->info. However, it turns out that that data is loaded from the top level moodle_backup.xml - which does not contain the version information
          Hide
          Tim Hunt added a comment -

          OK, am I a bloody genius or what?

          With this code, $this->task->get_old_module_version() in restore_quiz_activity_structure_step::define_structure() gets the information I want.

          Hopefully, this is an OK way to achieve this outcome.

          Show
          Tim Hunt added a comment - OK, am I a bloody genius or what? With this code, $this->task->get_old_module_version() in restore_quiz_activity_structure_step::define_structure() gets the information I want. Hopefully, this is an OK way to achieve this outcome.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          You got it, that's the way 100%!

          The only point of discussion could be where it should be:

          [get/set]_old_versionnumber() instead, just to be consistent with the way other methods have been named there.

          Ciao

          PS: This causes me to think what happens with plugins... we are not including that information with them and perhaps it would be interesting too to have it in a central way. That leads me to think that ALL plugins should have versions... uhm... surely one thing to debate in another issue.

          Show
          Eloy Lafuente (stronk7) added a comment - You got it, that's the way 100%! The only point of discussion could be where it should be: [get/set] _old_versionnumber() instead, just to be consistent with the way other methods have been named there. Ciao PS: This causes me to think what happens with plugins... we are not including that information with them and perhaps it would be interesting too to have it in a central way. That leads me to think that ALL plugins should have versions... uhm... surely one thing to debate in another issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          PS2: I'd backport this to 2.0, should apply clean and doesn't hurt at all, IMO

          Show
          Eloy Lafuente (stronk7) added a comment - PS2: I'd backport this to 2.0, should apply clean and doesn't hurt at all, IMO
          Hide
          Tim Hunt added a comment -

          Back-ported to 2.0.

          I did not rename the method. You can do that if you like.

          Show
          Tim Hunt added a comment - Back-ported to 2.0. I did not rename the method. You can do that if you like.
          Hide
          David Mudrak added a comment -

          +1 for this being fixed (thanks Tim)
          +1 for all plugins are versioned, even if they use neither DB nor capabilities

          Show
          David Mudrak added a comment - +1 for this being fixed (thanks Tim) +1 for all plugins are versioned, even if they use neither DB nor capabilities
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated, thanks!

          Notes:

          1) It seems your branch (master) has some incorrect commit (related to some essay stuff). So I just have cherry-picked 989f18be and used it both for master and 20_STABLE.

          2) I've renamed the methods to get/set_old_moduleversion()

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks! Notes: 1) It seems your branch (master) has some incorrect commit (related to some essay stuff). So I just have cherry-picked 989f18be and used it both for master and 20_STABLE. 2) I've renamed the methods to get/set_old_moduleversion() Ciao
          Hide
          Tim Hunt added a comment -

          Look at the diff URL I gave. My branch was not starting with master, but with another necessary bug-fix. So, yet, cherry-picking was correct, if you just wanted this fix.

          Thanks for integrating this. Pity about the re-name. I will have to update the code I wrote yesterday. It does depress me that code written as recently as Moodle 2.0, by one of the integrators responsible for maintaining the coding standards, does not follow the coding standards. Still I can see it is better to be consistent now.

          Show
          Tim Hunt added a comment - Look at the diff URL I gave. My branch was not starting with master, but with another necessary bug-fix. So, yet, cherry-picking was correct, if you just wanted this fix. Thanks for integrating this. Pity about the re-name. I will have to update the code I wrote yesterday. It does depress me that code written as recently as Moodle 2.0, by one of the integrators responsible for maintaining the coding standards, does not follow the coding standards. Still I can see it is better to be consistent now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          lol, in my mind "moduleversion" or "activityid" or "contextid" are one word, hence I used the the "together" approach".

          Surely if they part of something else, say one object of class "module", then I agree set_module_old_version() or get_module_id() have more sense, but I think this case is not really so clear.

          And, of course, standards are to break them, specially if they try to standarise the un-standarisable IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - lol, in my mind "moduleversion" or "activityid" or "contextid" are one word, hence I used the the "together" approach". Surely if they part of something else, say one object of class "module", then I agree set_module_old_version() or get_module_id() have more sense, but I think this case is not really so clear. And, of course, standards are to break them, specially if they try to standarise the un-standarisable IMO. Ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks guys looking good

          Show
          Sam Hemelryk added a comment - Thanks guys looking good
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Upstream, upstream, this is part of upstream, upstream... thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Upstream, upstream, this is part of upstream, upstream... thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: