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:
    • Rank:
      16062

      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.

        Issue Links

          Activity

          Davo Smith created issue -
          Tomasz Muras made changes -
          Field Original Value New Value
          Affects Version/s 2.0.2 [ 10421 ]
          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
          Martin Dougiamas made changes -
          Workflow MDL Workflow [ 68037 ] MDL Full Workflow [ 75915 ]
          Dan Poltawski made changes -
          Assignee moodle.com [ moodle.com ] Dan Poltawski [ poltawski ]
          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
          Dan Poltawski made changes -
          Assignee Dan Poltawski [ poltawski ] moodle.com [ moodle.com ]
          Dan Poltawski made changes -
          Labels triaged
          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 Škoda 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 Škoda 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 Škoda 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 Škoda 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.
          Tim Hunt made changes -
          Assignee moodle.com [ moodle.com ] Tim Hunt [ timhunt ]
          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 .
          Tim Hunt made changes -
          Status Open [ 1 ] Waiting for integration review [ 10010 ]
          Pull 2.0 Diff URL https://github.com/timhunt/moodle/compare/MOODLE_20_STABLE...MDL-26469_20
          Pull from Repository git://github.com/timhunt/moodle.git
          Pull 2.0 Branch MDL-26469_20
          Fix Version/s 2.0.8 [ 11554 ]
          Fix Version/s 2.1.5 [ 11553 ]
          Fix Version/s 2.2.2 [ 11552 ]
          Testing Instructions Test the mod_quiz upgrade in MDL-30635 and make sure modules.cron gets updated from 0 to 60.
          Pull 2.1 Branch MDL-26469_21
          Pull 2.1 Diff URL https://github.com/timhunt/moodle/compare/MOODLE_21_STABLE...MDL-26469_21
          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.
          Tim Hunt made changes -
          Link This issue will help resolve MDL-30635 [ MDL-30635 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator stronk7
          Currently in integration Yes [ 10041 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Affects Version/s 2.2.1 [ 11456 ]
          Affects Version/s 2.1.4 [ 11452 ]
          Affects Version/s 2.3 [ 10657 ]
          Affects Version/s 2.0.2 [ 10421 ]
          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...
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester stronk7
          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.
          Eloy Lafuente (stronk7) made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 12/Jan/12

            People

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

              Dates

              • Created:
                Updated:
                Resolved: