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

Remove deprecated code in group/group.php

    Details

    • Type: Task
    • Status: Closed
    • Priority: 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

      Description

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

        Gliffy Diagrams

          Activity

          Hide
          dobedobedoh 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
          dobedobedoh 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
          dobedobedoh 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
          dobedobedoh 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
          dobedobedoh Andrew Nicols added a comment -

          Too many similarly numbered issues open at one time!

          Show
          dobedobedoh Andrew Nicols added a comment - Too many similarly numbered issues open at one time!
          Hide
          poltawski 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
          poltawski 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
          stronk7 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
          stronk7 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
          salvetore 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
          salvetore 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
          dobedobedoh 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
          dobedobedoh 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
          timhunt 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
          timhunt 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
          dobedobedoh 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
          dobedobedoh 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
          samhemelryk Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now!

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now!
          Hide
          ankit_frenz 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_frenz 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
          samhemelryk 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
          samhemelryk 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:
                Fix Release Date:
                25/Jun/12