Sam Marshall
added a comment - 23/Nov/10 1:50 AM Note that core does not actually use this feature - it is there for plugins - so changing it can't break anything (short of syntax errors etc).
I have done it properly (the Tim way). Patch to follow once Tim has reviewed it.

This combines the code that looks for 'cron.php' in all those plugins (usually lib.php) and then checks if the frankenstyle_cron function exists; it only returns a list with the functions that exist. The same logic is done in other places so we could potentially replace those with this function call too, however this patch does not change anything in other places apart from admin and course report cron.

Sam Marshall
added a comment - 23/Nov/10 2:15 AM - edited Here is my patch. If this is OK I will commit it after 2.0 is released, so that it goes into 2.0.1.
This has been reviewed by Tim and I have tested it with cron functions in both admin and course reports, and with none.
Note that the code in cronlib.php is now shorter (even though it supports course reports as well as admin reports) thanks to the new API function. The function works like this:
$reports = get_plugin_list_with_function(array('report', 'coursereport'),
'cron', 'cron.php');
This combines the code that looks for 'cron.php' in all those plugins (usually lib.php) and then checks if the frankenstyle_cron function exists; it only returns a list with the functions that exist. The same logic is done in other places so we could potentially replace those with this function call too, however this patch does not change anything in other places apart from admin and course report cron.

Hmmm, we could add support for all remaining plugins, at the end of cron we could loop all plugin types that were not already processed (we need to execute some plugins like auth and enrol first with higher priority).

I like the get_plugin_list_with_function():

$plugintype should be just string imo - I do not think there is a place where we would need the array there

it would be nice if it was optionally looking for the new frankenstyle "mod_forum_xxx" functions too, maybe get_plugin_function_name() would not be necessary for now because it would have to return arrays.

Petr Skoda
added a comment - 29/Nov/10 12:03 PM Hmmm, we could add support for all remaining plugins, at the end of cron we could loop all plugin types that were not already processed (we need to execute some plugins like auth and enrol first with higher priority).
I like the get_plugin_list_with_function():
$plugintype should be just string imo - I do not think there is a place where we would need the array there
it would be nice if it was optionally looking for the new frankenstyle "mod_forum_xxx" functions too, maybe get_plugin_function_name() would not be necessary for now because it would have to return arrays.
My +1 to get all this into 2.0.1 right after we switch to git.
Thanks a lot for the report and patch!
Petr

After switch to git I will make the following changes and then commit:

1. remove support for array

2. instead loop through plugins in the part that calls it (list of plugins initially include at least report and coursereport, in array that can easily be added once people notice others that aren't included)

3. remove get_plugin_function_name and hardcode it inside main function, including check for both function names in modules (frankenstyle first).

Sam Marshall
added a comment - 29/Nov/10 7:16 PM thanks petr!
After switch to git I will make the following changes and then commit:
1. remove support for array
2. instead loop through plugins in the part that calls it (list of plugins initially include at least report and coursereport, in array that can easily be added once people notice others that aren't included)
3. remove get_plugin_function_name and hardcode it inside main function, including check for both function names in modules (frankenstyle first).
4. move code to/near end of cron.

1. The 'lastcron' feature did not work because it never found version.php because it was looking under frankenstyle name and one of the arrays uses only the plugin name without the prefix; fixed

2. I added a whole bunch of comments/documentation where I didn't understand something initially (or for just missing phpdoc on the functions).

3. I removed 'BC' support for coursereport_ because that was not implemented before so it does not need BC (left it in for report_)

As you can see from the previous patch, I tested this with 5 different cron methods, this includes both types of BC (report_ and gradesomething_), new API methods in both areas which also support BC, and one example of using the ->cron frequency setting. It seems to work correctly! Filed new PULL-37 request for next week or whenever (no rush).

Sam Marshall
added a comment - 22/Dec/10 12:56 AM I tested petr's patch, which is a nice improvement, and:
1. The 'lastcron' feature did not work because it never found version.php because it was looking under frankenstyle name and one of the arrays uses only the plugin name without the prefix; fixed
2. I added a whole bunch of comments/documentation where I didn't understand something initially (or for just missing phpdoc on the functions).
3. I removed 'BC' support for coursereport_ because that was not implemented before so it does not need BC (left it in for report_)
As you can see from the previous patch, I tested this with 5 different cron methods, this includes both types of BC (report_ and gradesomething_), new API methods in both areas which also support BC, and one example of using the ->cron frequency setting. It seems to work correctly! Filed new PULL-37 request for next week or whenever (no rush).

Petr Skoda
added a comment - 22/Dec/10 3:31 AM $nameonly = preg_replace(' ^.*?_ ', '', $component);
hmm, maybe we could just use the get_component_directory($component), it is a bit slow, but it should not matter much in cron.

Sam Marshall
added a comment - 23/Dec/10 6:34 PM good idea, that makes it a lot cleaner. I've committed that change (also retested with the same test patch as above, appears to still work with timed stuff).

This has been integrated now, will be tested tomorrow and hopefully will land upstream past tomorrow. Setting it as in review (formerly resolved). Thanks Sam & Petr & Petr (once more because of he supporting me when I was reviewing the pull request).

Side note: Please, create one issue, related to this, about the need of taking rid of cron_bc_hack_plugin_functions(), moving all the current "wrong" uses to the new lib.php->cron() way for DEV backlog (2.1), assigning it to David and, possibly, linking it (as somehow related) to MDL-25762). TIA!

Eloy Lafuente (stronk7)
added a comment - 28/Dec/10 1:07 AM This has been integrated now, will be tested tomorrow and hopefully will land upstream past tomorrow. Setting it as in review (formerly resolved). Thanks Sam & Petr & Petr (once more because of he supporting me when I was reviewing the pull request).
Side note: Please, create one issue, related to this, about the need of taking rid of cron_bc_hack_plugin_functions(), moving all the current "wrong" uses to the new lib.php->cron() way for DEV backlog (2.1), assigning it to David and, possibly, linking it (as somehow related) to MDL-25762 ). TIA!