From e0094618b577a82209b9cc52cd15dda5e1deb784 Mon Sep 17 00:00:00 2001
From: "Alexia E. Smith" <washuu@gmail.com>
Date: Tue, 16 Dec 2014 16:34:12 -0600
Subject: [PATCH] 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
---
 includes/User.php                       | 66 +++++++++++++++----------
 includes/specials/SpecialUserrights.php | 18 ++++---
 2 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/includes/User.php b/includes/User.php
index ad4ce60a45..dd199ee5b5 100644
--- 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 36a31bd8a7..3bf75a0e01 100644
--- 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 );
 
-- 
2.20.1