Moodle
  1. Moodle
  2. MDL-39778

delete_course() includes badges function without including library

    Details

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

      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

        Issue Links

          Activity

          Hide
          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 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
          Yuliya Bozhko added a comment -

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

          Show
          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 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 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
          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
          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
          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
          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
          Yuliya Bozhko added a comment -

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

          Show
          Yuliya Bozhko added a comment - I put another patch on top of my 2.5 branch and submitted diff URL.
          Hide
          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
          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
          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
          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
          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
          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 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 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
          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
          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
          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
          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
          Yuliya Bozhko added a comment -

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

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

          Thanks, and sorry for the conflicting advice

          Show
          Dan Poltawski added a comment - Thanks, and sorry for the conflicting advice
          Hide
          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 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 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 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
          Petr Škoda added a comment -

          Works fine, thanks.

          Show
          Petr Škoda added a comment - Works fine, thanks.
          Hide
          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 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: