Moodle
  1. Moodle
  2. MDL-26469

Cron setting not updated when plugins are upgraded

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.0.8, 2.1.5, 2.2.2
    • Component/s: General
    • Labels:
    • Testing Instructions:
      Hide

      Test the mod_quiz upgrade in MDL-30635 and make sure modules.cron gets updated from 0 to 60.

      Show
      Test the mod_quiz upgrade in MDL-30635 and make sure modules.cron gets updated from 0 to 60.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:

      Description

      1. Create a new plugin with and put the following line in version.php:
      $plugin->cron = 0

      2. Install the plugin to a Moodle server.
      3. Update version.php, changing the $plugin->version number and setting the cron line to, for example:
      $plugin->cron = 60

      4. Visit the admin/notifications page to upgrade the module

      Expected outcome:
      'mdl_modules' database table now contains the updated 'cron' value for this plugin (i.e. it should be '60')

      Actual outcome:
      'mdl_modules' database table still has the original 'cron' value ('0')

      Workaround:
      For the moment, I've added a manual upgrade to the cron value in 'mypluginname/db/upgrade.php', but would prefer not to have to do that.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tomasz Muras added a comment -

            It's quite a serious issue - especially in the case like described by Davo above: once module installs with cron disabled, it can not (in a clean way) enable cron for itself.

            It looks like the code to update the cron setting should go to:
            function upgrade_mod_savepoint

            Show
            Tomasz Muras added a comment - It's quite a serious issue - especially in the case like described by Davo above: once module installs with cron disabled, it can not (in a clean way) enable cron for itself. It looks like the code to update the cron setting should go to: function upgrade_mod_savepoint
            Hide
            Dan Poltawski added a comment -

            Is this specific to activity modules? I've just tested this with a block and don't get the same result

            Show
            Dan Poltawski added a comment - Is this specific to activity modules? I've just tested this with a block and don't get the same result
            Hide
            Dan Poltawski added a comment -

            Doh, just noticed the db table you are talking about

            Show
            Dan Poltawski added a comment - Doh, just noticed the db table you are talking about
            Hide
            Dan Poltawski added a comment -

            Added some watchers here..

            Verified this issue and its pretty nasty - we really should fix this!

            Show
            Dan Poltawski added a comment - Added some watchers here.. Verified this issue and its pretty nasty - we really should fix this!
            Hide
            Dan Poltawski added a comment -

            (affects activity mods, not blocks)

            Show
            Dan Poltawski added a comment - (affects activity mods, not blocks)
            Hide
            Petr Skoda added a comment -

            Why should we store the cron settings in DB tables when the version.php file contains it? why not parse it directly from there in each plugin type? Then if somebody needs to change this they could simply edit it - the only trouble might be that the version.php is changing frequently and merge collisions would be fatal there. In the long run in might be better to add db/cron.php to all plugins and describe the cron needs of each plugin there...

            Show
            Petr Skoda added a comment - Why should we store the cron settings in DB tables when the version.php file contains it? why not parse it directly from there in each plugin type? Then if somebody needs to change this they could simply edit it - the only trouble might be that the version.php is changing frequently and merge collisions would be fatal there. In the long run in might be better to add db/cron.php to all plugins and describe the cron needs of each plugin there...
            Hide
            Tomasz Muras added a comment -

            +1 for not storing this information in DB at all.
            It would make sense to store it in DB if we would like to make it configurable by end-users/administrators, e.g. you'd have default values in the code but then allow users to override it using UI. Since we're not doing it, storing it both in the code and DB seems to me like just asking for the troubles .

            Show
            Tomasz Muras added a comment - +1 for not storing this information in DB at all. It would make sense to store it in DB if we would like to make it configurable by end-users/administrators, e.g. you'd have default values in the code but then allow users to override it using UI. Since we're not doing it, storing it both in the code and DB seems to me like just asking for the troubles .
            Hide
            Petr Skoda added a comment -

            If we want to store it in DB then it should imo standardise on config_plugins table and use the info in version.php or db/cron.php only during install time and later no updates. Thinking a bit more, this can be viewed both as a feature and a bug - somebody might want to modify the db date to alter cron execution, plugin authors might want to fix incorrect value on the other hand. I personally believe the dev should decide in this case and if necessary add standard plugin setting for cron tweaking and implement it internally in the cron function.

            Show
            Petr Skoda added a comment - If we want to store it in DB then it should imo standardise on config_plugins table and use the info in version.php or db/cron.php only during install time and later no updates. Thinking a bit more, this can be viewed both as a feature and a bug - somebody might want to modify the db date to alter cron execution, plugin authors might want to fix incorrect value on the other hand. I personally believe the dev should decide in this case and if necessary add standard plugin setting for cron tweaking and implement it internally in the cron function.
            Hide
            Tomasz Muras added a comment -

            Petr,
            if we want to have the best of both worlds, we could implement it similar way Debian (and other distros) treat configuration files - the default value would be always in (say) cron.php. The current value is always in DB. During the upgrade, it could be automatically checked if user has edited the value from previous default. If he did, it stays as set by user. If not, then it's updated to whatever developer has set in the new version of cron.php.
            But if we go with this, I think we would need to add some UI where users can review the settings.

            Show
            Tomasz Muras added a comment - Petr, if we want to have the best of both worlds, we could implement it similar way Debian (and other distros) treat configuration files - the default value would be always in (say) cron.php. The current value is always in DB. During the upgrade, it could be automatically checked if user has edited the value from previous default. If he did, it stays as set by user. If not, then it's updated to whatever developer has set in the new version of cron.php. But if we go with this, I think we would need to add some UI where users can review the settings.
            Hide
            Tim Hunt added a comment -

            This is indeed a serious issue, and is causing me grief on MDL-30635, so I will fix this now.

            Show
            Tim Hunt added a comment - This is indeed a serious issue, and is causing me grief on MDL-30635 , so I will fix this now.
            Hide
            Tim Hunt added a comment -

            Not to integrators, commit for 22 and master branches is included in the fixup branch for MDL-30635.

            Show
            Tim Hunt added a comment - Not to integrators, commit for 22 and master branches is included in the fixup branch for MDL-30635 .
            Hide
            Tim Hunt added a comment -

            I note the comments above, about cleaning up module and block cron, to read $plugin->cron directly from version.php, like every other plugin type, instead of storing it in the database. However, I think that is a separate issue.

            Show
            Tim Hunt added a comment - I note the comments above, about cleaning up module and block cron, to read $plugin->cron directly from version.php, like every other plugin type, instead of storing it in the database. However, I think that is a separate issue.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks (20 and 21). And, once MDL-30635 is done, this will be fixed also for 22 and master.

            Note: I've nothing against table-less cron everywhere (relying exclusively in version.php one). If anybody is really interested, please fill a new issue about that, afaik, modules and blocks are the only 2 plugins using DB cron settings. Although surely it will be worth checking it.

            In the meantime, this is now fixed and module changes of cron are recognized by the system.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated, thanks (20 and 21). And, once MDL-30635 is done, this will be fixed also for 22 and master. Note: I've nothing against table-less cron everywhere (relying exclusively in version.php one). If anybody is really interested, please fill a new issue about that, afaik, modules and blocks are the only 2 plugins using DB cron settings. Although surely it will be worth checking it. In the meantime, this is now fixed and module changes of cron are recognized by the system. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Just detected that the change breaks upgrade with error:

            Default exception handler: Error writing to database Debug: Column 'cron' cannot be null
            UPDATE ciu_modules SET cron = NULL WHERE name = ?
            [array (
              0 => 'workshop',
            )]
            * line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown
            * line 1071 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
            * line 1475 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->set_field_select()
            * line 554 of /lib/upgradelib.php: call to moodle_database->set_field()
            * line 271 of /lib/upgradelib.php: call to upgrade_plugins_modules()
            * line 1471 of /lib/upgradelib.php: call to upgrade_plugins()
            * line 146 of /admin/cli/upgrade.php: call to upgrade_noncore()
            

            So we need to check for not null / isset values before setting the field. This affects ALL branches (20, 21, 22, master).

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Just detected that the change breaks upgrade with error: Default exception handler: Error writing to database Debug: Column 'cron' cannot be null UPDATE ciu_modules SET cron = NULL WHERE name = ? [array ( 0 => 'workshop', )] * line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown * line 1071 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() * line 1475 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->set_field_select() * line 554 of /lib/upgradelib.php: call to moodle_database->set_field() * line 271 of /lib/upgradelib.php: call to upgrade_plugins_modules() * line 1471 of /lib/upgradelib.php: call to upgrade_plugins() * line 146 of /admin/cli/upgrade.php: call to upgrade_noncore() So we need to check for not null / isset values before setting the field. This affects ALL branches (20, 21, 22, master).
            Hide
            Tim Hunt added a comment -
            Show
            Tim Hunt added a comment - I suck! Please cherry-pick https://github.com/timhunt/moodle/commit/ff05bb31664fc60db14dafd62195ad4ca9291556 to all four branches.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I've applied the extra commit to 20 and 21 and CI tests are back to normal, so this is ready for testing. Thanks!

            Going to integrate MDL-30635 now...

            Show
            Eloy Lafuente (stronk7) added a comment - I've applied the extra commit to 20 and 21 and CI tests are back to normal, so this is ready for testing. Thanks! Going to integrate MDL-30635 now...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Passing tests, I've tried the upgrade of MDL-30635 (22 and master), and also playing under 21 with changes in one module (setting, back to 0, undefined...) and them ended with the correct result in the modules->cron column.

            Show
            Eloy Lafuente (stronk7) added a comment - Passing tests, I've tried the upgrade of MDL-30635 (22 and master), and also playing under 21 with changes in one module (setting, back to 0, undefined...) and them ended with the correct result in the modules->cron column.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: