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:

      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?

        Gliffy Diagrams

          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: