Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      6619

      Description

      Cron works for admin reports but not course reports. This is poor.

      Additionally, Tim wants me to make this change nicely so I want to make a new API function.

      1. coursereport.cron.patch
        5 kB
        Sam Marshall
      2. cron.test.patch
        3 kB
        Sam Marshall

        Activity

        Hide
        Sam Marshall added a comment -

        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.

        Show
        Sam Marshall added a comment - 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.
        Hide
        Sam Marshall added a comment - - 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.

        Show
        Sam Marshall added a comment - - 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.
        Hide
        Petr Škoda added a comment -

        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

        Show
        Petr Škoda added a comment - 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
        Hide
        Sam Marshall added a comment -

        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.

        Show
        Sam Marshall added a comment - 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.
        Hide
        Petr Škoda added a comment -

        thanks!

        Show
        Petr Škoda added a comment - thanks!
        Hide
        Sam Marshall added a comment -

        Just to note here - I did code and added pull request for it, look forward to seeing if new process works.

        Show
        Sam Marshall added a comment - Just to note here - I did code and added pull request for it, look forward to seeing if new process works.
        Hide
        Petr Škoda added a comment -

        Sorry for the confusion, I am going to create patch myself for you to review.

        Show
        Petr Škoda added a comment - Sorry for the confusion, I am going to create patch myself for you to review.
        Hide
        Petr Škoda added a comment -

        arrrggh, cron execution is such a mess! (at least 50% is my fault)

        Show
        Petr Škoda added a comment - arrrggh, cron execution is such a mess! (at least 50% is my fault)
        Hide
        Petr Škoda added a comment -

        https://github.com/skodak/moodle/commits/wip-MDL-25370-cron

        Sending untested code, please review and if you find some time please test&fix&file pull request.

        Show
        Petr Škoda added a comment - https://github.com/skodak/moodle/commits/wip-MDL-25370-cron Sending untested code, please review and if you find some time please test&fix&file pull request.
        Hide
        Sam Marshall added a comment -

        This patch is temporary code which adds cron methods to various existing reports, for testing the new API.

        Show
        Sam Marshall added a comment - This patch is temporary code which adds cron methods to various existing reports, for testing the new API.
        Hide
        Sam Marshall added a comment -

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

        Show
        Sam Marshall added a comment - 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).
        Hide
        Petr Škoda added a comment -

        thanks a lot!

        Show
        Petr Škoda added a comment - thanks a lot!
        Hide
        Petr Škoda added a comment -

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

        Show
        Petr Škoda added a comment - $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.
        Hide
        Sam Marshall added a comment -

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

        Show
        Sam Marshall added a comment - 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).
        Hide
        Petr Škoda added a comment -

        thanks!

        Show
        Petr Škoda added a comment - thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        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!

        Show
        Eloy Lafuente (stronk7) added a comment - 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!
        Hide
        Petr Škoda added a comment -

        done, thanks

        Show
        Petr Škoda added a comment - done, thanks

          People

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

            Dates

            • Created:
              Updated:
              Resolved: