Cron setting not updated when plugins are upgraded

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

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

Doh, just noticed the db table you are talking about

Added some watchers here..

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

(affects activity mods, not blocks)

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...

+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 .

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.

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.

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

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

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.

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

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).

I suck! Please cherry-pick https://github.com/timhunt/moodle/commit/ff05bb31664fc60db14dafd62195ad4ca9291556 to all four branches.

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...

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.

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

Closing, ciao