Moodle
  1. Moodle
  2. MDL-32008

Remove deprecated code in group/group.php

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.3
    • Component/s: Groups
    • Labels:
    • Testing Instructions:
      Hide
      • As a teacher, open a course
      • Navigate to Settings -> Course administration -> Users -> Groups
      • Create three groups
      • Select two of the groups you just created
        • Click the 'Delete selected groups' button
        • Click yes on the confirmation page
        • Confirm that the groups have been removed
      • Select the third group you created
        • Click the 'Delete selected groups' button
        • Click yes on the confirmation page
        • Confirm that this group has also been removed
      • Try visiting /groups/group.php?courseid=1&delete=1
        • Confirm that a debugging message is shown if developer mode is enabled
        • Confirm that the redirect works
      Show
      As a teacher, open a course Navigate to Settings -> Course administration -> Users -> Groups Create three groups Select two of the groups you just created Click the 'Delete selected groups' button Click yes on the confirmation page Confirm that the groups have been removed Select the third group you created Click the 'Delete selected groups' button Click yes on the confirmation page Confirm that this group has also been removed Try visiting /groups/group.php?courseid=1&delete=1 Confirm that a debugging message is shown if developer mode is enabled Confirm that the redirect works
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32008-master-2
    • Rank:
      38680

      Description

      Whilst working on MDL-32005, I came across some deprecated code in group/group.php related to deletion which is no longer hit.

        Activity

        Hide
        Andrew Nicols added a comment -

        I was originally just going to remove the second hunk from

        if ($id and $delete) {
        

        as the if ($delete) hunk above meant this condition should never be reached anyway, but a git blame and bit of a look at the history shows that the redirect itself has been presence since 1.9.4 and came from MDL-16756 so I've removed all of the delete code from group/group.php as there shouldn't be any code using that any more.

        Show
        Andrew Nicols added a comment - I was originally just going to remove the second hunk from if ($id and $delete) { as the if ($delete) hunk above meant this condition should never be reached anyway, but a git blame and bit of a look at the history shows that the redirect itself has been presence since 1.9.4 and came from MDL-16756 so I've removed all of the delete code from group/group.php as there shouldn't be any code using that any more.
        Hide
        Andrew Nicols added a comment -

        A quick git grep shows that there are no pages in core still using group.php and passing it the delete option:

        766 moodle:MDL-32008-master-1> git grep 'group.php' | grep del
        766 moodle:MDL-32008-master-1> 
        

        I've also checked the output of git grep group.php for any multi-line URLs which the group wouldn't pick up.

        Show
        Andrew Nicols added a comment - A quick git grep shows that there are no pages in core still using group.php and passing it the delete option: 766 moodle:MDL-32008-master-1> git grep 'group.php' | grep del 766 moodle:MDL-32008-master-1> I've also checked the output of git grep group.php for any multi-line URLs which the group wouldn't pick up.
        Hide
        Andrew Nicols added a comment -

        Too many similarly numbered issues open at one time!

        Show
        Andrew Nicols added a comment - Too many similarly numbered issues open at one time!
        Hide
        Dan Poltawski added a comment -

        The original was remove din MDL-16756 - 2008.

        Looks good to go for master only, anything inking there should have been converted by now.

        Show
        Dan Poltawski added a comment - The original was remove din MDL-16756 - 2008. Looks good to go for master only, anything inking there should have been converted by now.
        Hide
        Eloy Lafuente (stronk7) 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
        Eloy Lafuente (stronk7) 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
        Michael de Raadt added a comment -

        It may still be prudent to retain the if-redirect statement as we cannot know if any contributed code is using this. As there is little cost to leaving it there, I suggest we do not remove it.

        Show
        Michael de Raadt added a comment - It may still be prudent to retain the if-redirect statement as we cannot know if any contributed code is using this. As there is little cost to leaving it there, I suggest we do not remove it.
        Hide
        Andrew Nicols added a comment -

        I'm happy to do so if you think it's best.

        The original delete code was introduced on 2007-08-16 and deprecated on 2008-10-24 and it appears as though it was only ever present in four releases:

        • 1.9.0
        • 1.9.1
        • 1.9.2
        • 1.9.3
        Show
        Andrew Nicols added a comment - I'm happy to do so if you think it's best. The original delete code was introduced on 2007-08-16 and deprecated on 2008-10-24 and it appears as though it was only ever present in four releases: 1.9.0 1.9.1 1.9.2 1.9.3
        Hide
        Tim Hunt added a comment -

        I think +1 for cleaning up the mess.

        If this really does break anything, which I doubt, we can always revert it.

        Show
        Tim Hunt added a comment - I think +1 for cleaning up the mess. If this really does break anything, which I doubt, we can always revert it.
        Hide
        Andrew Nicols added a comment -

        I've updated the patch to add the redirect back, though switched it to using a moodle_url instead. I've also added a debugging message to warn of impending doom/removal.

        Show
        Andrew Nicols added a comment - I've updated the patch to add the redirect back, though switched it to using a moodle_url instead. I've also added a debugging message to warn of impending doom/removal.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, this has been integrated now!

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now!
        Hide
        Ankit Agarwal added a comment -

        Looks perfect to me!

        I get following error msg when the developer mode was on and obviously that prevented the automatic redirect

        Deleting a group through group/group.php is deprecated and will be removed soon. Please use group/delete.php instead
        
            line 39 of /group/group.php: call to debugging()
        

        And when debug is off redirect works perfectly sending to the error page saying "Incorrect group id specified"

        Passing
        Thanks

        Show
        Ankit Agarwal added a comment - Looks perfect to me! I get following error msg when the developer mode was on and obviously that prevented the automatic redirect Deleting a group through group/group.php is deprecated and will be removed soon. Please use group/delete.php instead line 39 of /group/group.php: call to debugging() And when debug is off redirect works perfectly sending to the error page saying "Incorrect group id specified" Passing Thanks
        Hide
        Sam Hemelryk added a comment -

        Congratulations are in order, you've made it, or at least your code has!
        It's now part of Moodle and both the git and cvs repositories have been updated.

        This issue is being marked as fixed and closed.

        Thank you.

        Show
        Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: