Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39778

delete_course() includes badges function without including library

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      Follow the steps to reproduce in bug description. When hit cancel at the last step, there should be no error.

      Show
      Follow the steps to reproduce in bug description. When hit cancel at the last step, there should be no error.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE

      Description

      delete_course() has badges_handle_course_deletion(), but in some cases badgelib may not be included leading to a fatal error.

      This can be reproduced by

      1. Enabling badges
      2. Doing restore of backup to a new course
      3. On "5. Review" step, hit cancel

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            damyon Damyon Wiese added a comment -

            Fatal error: Call to undefined function badges_handle_course_deletion() in /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/lib/moodlelib.php on line 4728 Call Stack: 0.0003 707024 1.

            {main}

            () /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/restore.php:0 0.2618 80936120 2. restore_ui->process() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/restore.php:42 0.2618 80936120 3. restore_ui_stage_review->process() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui.class.php:100 0.2851 82452728 4. restore_ui->cancel_process() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui_stage.class.php:542 0.2851 82452728 5. restore_ui->cleanup() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui.class.php:233 0.2860 82452808 6. delete_course() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui.class.php:159

            Show
            damyon Damyon Wiese added a comment - Fatal error: Call to undefined function badges_handle_course_deletion() in /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/lib/moodlelib.php on line 4728 Call Stack: 0.0003 707024 1. {main} () /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/restore.php:0 0.2618 80936120 2. restore_ui->process() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/restore.php:42 0.2618 80936120 3. restore_ui_stage_review->process() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui.class.php:100 0.2851 82452728 4. restore_ui->cancel_process() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui_stage.class.php:542 0.2851 82452728 5. restore_ui->cleanup() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui.class.php:233 0.2860 82452808 6. delete_course() /home/damyonw/Documents/Moodle/integration/MOODLE_25_STABLE/moodle/backup/util/ui/restore_ui.class.php:159
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Can please anyone let me know if I need to submit a patch for master branch as well?

            Show
            ybozhko Yuliya Bozhko added a comment - Can please anyone let me know if I need to submit a patch for master branch as well?
            Hide
            damyon Damyon Wiese added a comment -

            Hi Yuliya,

            We are in the "on-sync" period where we do not let 2.6 and 2.5 diverge until 2.5.1 is released - so you do not need to provide a separate patch.

            Show
            damyon Damyon Wiese added a comment - Hi Yuliya, We are in the "on-sync" period where we do not let 2.6 and 2.5 diverge until 2.5.1 is released - so you do not need to provide a separate patch.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Yuliya,

            I think that 'Moodle style' would be to the require at the top of the function and I just checked the coding style doc and indeed we do specify that: http://docs.moodle.org/dev/Coding_style#Require_.2F_include

            Otherwise it makes sense, but..

            We were discussing this internally yesterday and this issue brought to our attention the fact the badges_handle_course_deletion() work seems misplaced in delete_course() and would actually be better placed in remove_course_contents(), which is called by delete_course() but also is used for course reset.

            Unfortunately that then makes the name of badges_handle_course_deletion() name slightly weird, but from the looks of it, this would be straight forward.

            (Sorry I didn't pick that one up in peer review)

            Show
            poltawski Dan Poltawski added a comment - Hi Yuliya, I think that 'Moodle style' would be to the require at the top of the function and I just checked the coding style doc and indeed we do specify that: http://docs.moodle.org/dev/Coding_style#Require_.2F_include Otherwise it makes sense, but.. We were discussing this internally yesterday and this issue brought to our attention the fact the badges_handle_course_deletion() work seems misplaced in delete_course() and would actually be better placed in remove_course_contents(), which is called by delete_course() but also is used for course reset. Unfortunately that then makes the name of badges_handle_course_deletion() name slightly weird, but from the looks of it, this would be straight forward. (Sorry I didn't pick that one up in peer review)
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Hi Dan,

            Sure, makes sense I can have a look at that and move it to remove_course_contents.

            Show
            ybozhko Yuliya Bozhko added a comment - Hi Dan, Sure, makes sense I can have a look at that and move it to remove_course_contents.
            Hide
            ybozhko Yuliya Bozhko added a comment -

            I put another patch on top of my 2.5 branch and submitted diff URL.

            Show
            ybozhko Yuliya Bozhko added a comment - I put another patch on top of my 2.5 branch and submitted diff URL.
            Hide
            poltawski Dan Poltawski added a comment -

            +1, sending for integration.

            Strictly we probably should keep a backwards compatible function, but given this is so new I think its better to avoid it.

            Show
            poltawski Dan Poltawski added a comment - +1, sending for integration. Strictly we probably should keep a backwards compatible function, but given this is so new I think its better to avoid it.
            Hide
            poltawski Dan Poltawski added a comment -

            Bah sorry, it was just pointed out that you've dropped the badges_ prefix from the name of the function.

            Also, nitpicking, the phpdoc could mention course reset in make it clear its not just course deletion.

            Show
            poltawski Dan Poltawski added a comment - Bah sorry, it was just pointed out that you've dropped the badges_ prefix from the name of the function. Also, nitpicking, the phpdoc could mention course reset in make it clear its not just course deletion.
            Hide
            ybozhko Yuliya Bozhko added a comment -

            I am not sure I understand your comment about phpdoc. This method is never called when course is reset...

            Show
            ybozhko Yuliya Bozhko added a comment - I am not sure I understand your comment about phpdoc. This method is never called when course is reset...
            Hide
            damyon Damyon Wiese added a comment -

            I haven't checked reset course - but it definitely called when restoring a backup into an existing course (and overwriting).

            Show
            damyon Damyon Wiese added a comment - I haven't checked reset course - but it definitely called when restoring a backup into an existing course (and overwriting).
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Now, thinking about that, I am not sure what users would expect should happen to badges on course reset... We can't revoke them if they have been already issued, but clearing user data will potentially trigger issuing the same badges twice...

            @Dan: I have put badges_ prefix back Didn't change phpdoc as I am still not sure this method is called when course is reset.

            Show
            ybozhko Yuliya Bozhko added a comment - Now, thinking about that, I am not sure what users would expect should happen to badges on course reset... We can't revoke them if they have been already issued, but clearing user data will potentially trigger issuing the same badges twice... @Dan: I have put badges_ prefix back Didn't change phpdoc as I am still not sure this method is called when course is reset.
            Hide
            poltawski Dan Poltawski added a comment -

            Yeah, you are right, sorry - i've confused the issue again. Thats reset_course_userdata() which does that (and we can consider that functionality in a new issue..).

            So now I question myself and wonder if it is worth renaming that function (considering that it breaks backwards compatibility, and usually we'd try hard to avoid that on the stable branches).

            Show
            poltawski Dan Poltawski added a comment - Yeah, you are right, sorry - i've confused the issue again. Thats reset_course_userdata() which does that (and we can consider that functionality in a new issue..). So now I question myself and wonder if it is worth renaming that function (considering that it breaks backwards compatibility, and usually we'd try hard to avoid that on the stable branches).
            Hide
            ybozhko Yuliya Bozhko added a comment -

            I am saving you from this hard decision and renaming everything back

            Show
            ybozhko Yuliya Bozhko added a comment - I am saving you from this hard decision and renaming everything back
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks, and sorry for the conflicting advice

            Show
            poltawski Dan Poltawski added a comment - Thanks, and sorry for the conflicting advice
            Hide
            damyon Damyon Wiese added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            damyon Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Yuliya,

            I have integrated this to master and 25 branches. Note - I squashed the commits as in this case I the commit history is clearer.

            Show
            damyon Damyon Wiese added a comment - Thanks Yuliya, I have integrated this to master and 25 branches. Note - I squashed the commits as in this case I the commit history is clearer.
            Hide
            skodak Petr Skoda added a comment -

            Works fine, thanks.

            Show
            skodak Petr Skoda added a comment - Works fine, thanks.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for your hard work. This issue has now been sent upstream and will soon be downloaded by millions of Moodlers!

            Regards, Damyon

            Show
            damyon Damyon Wiese added a comment - Thanks for your hard work. This issue has now been sent upstream and will soon be downloaded by millions of Moodlers! Regards, Damyon

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13