Use: addGroup() and removeGroup() should return boolean

Have User::addGroup() and User::removeGroup() methoids return a
boolean when their respective hooks return the respective boolean.

Fix SpecialUserrights to respect this return vale and update the
add/remove arrays accordingly.

This resolves an issue where a hook that prevents a group from
being added or removed still shows that group being changed in
the Userrights log.

Change-Id: I7621cc22b04ff41cf67bd434a1f89d31bdc2cffd

diff --git a/includes/User.php b/includes/User.php
index ad4ce60..dd199ee 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -3008,20 +3008,24 @@ class User implements IDBAccessObject {
         * Add the user to the given group.
         * This takes immediate effect.
         * @param string $group Name of the group to add
+        * @return bool
         */
        public function addGroup( $group ) {
-               if ( Hooks::run( 'UserAddGroup', array( $this, &$group ) ) ) {
-                       $dbw = wfGetDB( DB_MASTER );
-                       if ( $this->getId() ) {
-                               $dbw->insert( 'user_groups',
-                                       array(
-                                               'ug_user' => $this->getID(),
-                                               'ug_group' => $group,
-                                       ),
-                                       __METHOD__,
-                                       array( 'IGNORE' ) );
-                       }
+               if ( !Hooks::run( 'UserAddGroup', array( $this, &$group ) ) ) {
+                       return false;
+               }
+
+               $dbw = wfGetDB( DB_MASTER );
+               if ( $this->getId() ) {
+                       $dbw->insert( 'user_groups',
+                               array(
+                                       'ug_user' => $this->getID(),
+                                       'ug_group' => $group,
+                               ),
+                               __METHOD__,
+                               array( 'IGNORE' ) );
                }
+
                $this->loadGroups();
                $this->mGroups[] = $group;
                // In case loadGroups was not called before, we now have the right twice.
@@ -3034,31 +3038,39 @@ class User implements IDBAccessObject {
                $this->mRights = null;
 
                $this->invalidateCache();
+
+               return true;
        }
 
        /**
         * Remove the user from the given group.
         * This takes immediate effect.
         * @param string $group Name of the group to remove
+        * @return bool
         */
        public function removeGroup( $group ) {
                $this->load();
-               if ( Hooks::run( 'UserRemoveGroup', array( $this, &$group ) ) ) {
-                       $dbw = wfGetDB( DB_MASTER );
-                       $dbw->delete( 'user_groups',
-                               array(
-                                       'ug_user' => $this->getID(),
-                                       'ug_group' => $group,
-                               ), __METHOD__ );
-                       // Remember that the user was in this group
-                       $dbw->insert( 'user_former_groups',
-                               array(
-                                       'ufg_user' => $this->getID(),
-                                       'ufg_group' => $group,
-                               ),
-                               __METHOD__,
-                               array( 'IGNORE' ) );
+               if ( !Hooks::run( 'UserRemoveGroup', array( $this, &$group ) ) ) {
+                       return false;
                }
+
+               $dbw = wfGetDB( DB_MASTER );
+               $dbw->delete( 'user_groups',
+                       array(
+                               'ug_user' => $this->getID(),
+                               'ug_group' => $group,
+                       ), __METHOD__
+               );
+               // Remember that the user was in this group
+               $dbw->insert( 'user_former_groups',
+                       array(
+                               'ufg_user' => $this->getID(),
+                               'ufg_group' => $group,
+                       ),
+                       __METHOD__,
+                       array( 'IGNORE' )
+               );
+
                $this->loadGroups();
                $this->mGroups = array_diff( $this->mGroups, array( $group ) );
 
@@ -3068,6 +3080,8 @@ class User implements IDBAccessObject {
                $this->mRights = null;
 
                $this->invalidateCache();
+
+               return true;
        }
 
        /**

diff --git a/includes/specials/SpecialUserrights.php b/includes/specials/SpecialUserrights.php
index 36a31bd..3bf75a0 100644 (file)
--- a/includes/specials/SpecialUserrights.php
+++ b/includes/specials/SpecialUserrights.php
@@ -244,18 +244,22 @@ class UserrightsPage extends SpecialPage {
                $oldGroups = $user->getGroups();
                $newGroups = $oldGroups;
 
-               // remove then add groups
+               // Remove then add groups
                if ( $remove ) {
-                       $newGroups = array_diff( $newGroups, $remove );
-                       foreach ( $remove as $group ) {
-                               $user->removeGroup( $group );
+                       foreach ( $remove as $index => $group ) {
+                               if ( !$user->removeGroup( $group ) ) {
+                                       unset($remove[$index]);
+                               }
                        }
+                       $newGroups = array_diff( $newGroups, $remove );
                }
                if ( $add ) {
-                       $newGroups = array_merge( $newGroups, $add );
-                       foreach ( $add as $group ) {
-                               $user->addGroup( $group );
+                       foreach ( $add as $index => $group ) {
+                               if ( !$user->addGroup( $group ) ) {
+                                       unset($add[$index]);
+                               }
                        }
+                       $newGroups = array_merge( $newGroups, $add );
                }
                $newGroups = array_unique( $newGroups );