r107418 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r107417‎ | r107418 | r107419 >
Date:20:47, 27 December 2011
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Added/update some documentation

Added explicit function modifiers

Tidy up some redundant else statements at the end of functions

Explicitly return null

De-indent some code by a few levels
Modified paths:
  • /trunk/extensions/LdapAuthentication/LdapAuthentication.php (modified) (history)

Diff [purge]

Index: trunk/extensions/LdapAuthentication/LdapAuthentication.php
——@@ -504,8 +504,6 @@
505505 * @return bool
506506 */
507507 public function userExists( $username ) {
508 - global $wgLDAPAddLDAPUsers;
509 -
510508 $this->printDebug( "Entering userExists", NONSENSITIVE );
511509
512510 // If we can't add LDAP users, we don't really need to check
——@@ -550,10 +548,8 @@
551549
552550 /**
553551 * Connect to LDAP
554 - *
555 - * @access private
556552 */
557 - function connect( $domain='' ) {
 553+ private function connect( $domain='' ) {
558554 if ( $domain == '' ) {
559555 $domain = $this->getSessionDomain();
560556 }
——@@ -747,6 +743,7 @@
748744 * Modify options in the login template.
749745 *
750746 * @param UserLoginTemplate $template
 747+ * @param $type
751748 */
752749 public function modifyUITemplate( &$template, &$type ) {
753750 $this->printDebug( "Entering modifyUITemplate", NONSENSITIVE );
——@@ -764,6 +761,9 @@
765762 wfRunHooks( 'LDAPModifyUITemplate', array( &$template ) );
766763 }
767764
 765+ /**
 766+ * @return array
 767+ */
768768 function domainList() {
769769 global $wgLDAPDomainNames;
770770
——@@ -858,13 +858,10 @@
859859 if ( $success ) {
860860 $this->printDebug( "Successfully modified the user's password", NONSENSITIVE );
861861 return true;
862 - } else {
863 - $this->printDebug( "Failed to modify the user's password", NONSENSITIVE );
864 - return false;
865862 }
866 - } else {
867 - return false;
 863+ $this->printDebug( "Failed to modify the user's password", NONSENSITIVE );
868864 }
 865+ return false;
869866 }
870867
871868 /**
——@@ -918,14 +915,11 @@
919916 $this->printDebug( "Successfully modified the user's attributes", NONSENSITIVE );
920917 LdapAuthenticationPlugin::ldap_unbind( $this->ldapconn );
921918 return true;
922 - } else {
923 - $this->printDebug( "Failed to modify the user's attributes", NONSENSITIVE );
924 - LdapAuthenticationPlugin::ldap_unbind( $this->ldapconn );
925 - return false;
926919 }
927 - } else {
928 - return false;
 920+ $this->printDebug( "Failed to modify the user's attributes", NONSENSITIVE );
 921+ LdapAuthenticationPlugin::ldap_unbind( $this->ldapconn );
929922 }
 923+ return false;
930924 }
931925
932926 /**
——@@ -966,6 +960,8 @@
967961 *
968962 * @param User $user
969963 * @param string $password
 964+ * @param string $email
 965+ * @param string $realname
970966 * @return bool
971967 */
972968 public function addUser( $user, $password, $email = '', $realname = '' ) {
——@@ -1054,14 +1050,11 @@
10551051 $this->printDebug( "Successfully added user", NONSENSITIVE );
10561052 LdapAuthenticationPlugin::ldap_unbind( $this->ldapconn );
10571053 return true;
1058 - } else {
1059 - $this->printDebug( "Failed to add user", NONSENSITIVE );
1060 - LdapAuthenticationPlugin::ldap_unbind( $this->ldapconn );
1061 - return false;
10621054 }
1063 - } else {
1064 - return false;
 1055+ $this->printDebug( "Failed to add user", NONSENSITIVE );
 1056+ LdapAuthenticationPlugin::ldap_unbind( $this->ldapconn );
10651057 }
 1058+ return false;
10661059 }
10671060
10681061 /**
——@@ -1198,10 +1191,9 @@
11991192 if ( $this->getConf( 'UseLocal' ) || $this->getConf( 'MailPassword' ) ) {
12001193 $this->printDebug( "Returning false in strict().", NONSENSITIVE );
12011194 return false;
1202 - } else {
1203 - $this->printDebug( "Returning true in strict().", NONSENSITIVE );
1204 - return true;
12051195 }
 1196+ $this->printDebug( "Returning true in strict().", NONSENSITIVE );
 1197+ return true;
12061198 }
12071199
12081200 /**
——@@ -1366,6 +1358,9 @@
13671359 return $userdn;
13681360 }
13691361
 1362+ /**
 1363+ * @return array|null
 1364+ */
13701365 function getUserInfo() {
13711366 // Don't fetch the same data more than once
13721367 if ( $this->fetchedUserInfo ) {
——@@ -1375,13 +1370,17 @@
13761371 $userInfo = $this->getUserInfoStateless( $this->usernn );
13771372 if ( is_null( $userInfo ) ) {
13781373 $this->fetchedUserInfo = false;
1379 - return;
 1374+ return null;
13801375 } else {
13811376 $this->fetchedUserInfo = true;
13821377 return $userInfo;
13831378 }
13841379 }
13851380
 1381+ /**
 1382+ * @param $userdn string
 1383+ * @return array|null
 1384+ */
13861385 function getUserInfoStateless( $userdn ) {
13871386 // Don't fetch the same data more than once
13881387 // TODO: use memcached here
——@@ -1389,7 +1388,7 @@
13901389 $entry = LdapAuthenticationPlugin::ldap_read( $this->ldapconn, $userdn, "objectclass=*", array( '*', 'memberof' ) );
13911390 $userInfo = LdapAuthenticationPlugin::ldap_get_entries( $this->ldapconn, $entry );
13921391 if ( $userInfo["count"] < 1 ) {
1393 - return;
 1392+ return null;
13941393 } else {
13951394 return $userInfo;
13961395 }
——@@ -1397,11 +1396,8 @@
13981397
13991398 /**
14001399 * Retrieve user preferences from LDAP
1401 - *
1402 - * @param string $userDN
1403 - * @access private
14041400 */
1405 - function getPreferences() {
 1401+ private function getPreferences() {
14061402 $this->printDebug( "Entering getPreferences", NONSENSITIVE );
14071403
14081404 $this->userInfo = $this->getUserInfo();
——@@ -1411,34 +1407,34 @@
14121408
14131409 // Retrieve preferences
14141410 $prefs = $this->getConf( 'Preferences' );
1415 - if ( $prefs ) {
1416 - $this->printDebug( "Retrieving preferences", NONSENSITIVE );
1417 - foreach ( array_keys( $prefs ) as $key ) {
1418 - $attr = strtolower( $prefs[$key] );
1419 - if ( isset( $this->userInfo[0][$attr] ) ) {
1420 - $value = $this->userInfo[0][$attr][0];
1421 - } else {
1422 - continue;
1423 - }
1424 - switch ( $key ) {
1425 - case "email":
1426 - $this->email = $value;
1427 - $this->printDebug( "Retrieved email ($this->email) using attribute ($prefs[$key])", NONSENSITIVE );
1428 - break;
1429 - case "language":
1430 - $this->lang = $value;
1431 - $this->printDebug( "Retrieved language ($this->lang) using attribute ($prefs[$key])", NONSENSITIVE );
1432 - break;
1433 - case "nickname":
1434 - $this->nickname = $value;
1435 - $this->printDebug( "Retrieved nickname ($this->nickname) using attribute ($prefs[$key])", NONSENSITIVE );
1436 - break;
1437 - case "realname":
1438 - $this->realname = $value;
1439 - $this->printDebug( "Retrieved realname ($this->realname) using attribute ($prefs[$key])", NONSENSITIVE );
1440 - break;
1441 - }
 1411+ if ( !$prefs ) {
 1412+ return;
 1413+ }
 1414+ $this->printDebug( "Retrieving preferences", NONSENSITIVE );
 1415+ foreach ( array_keys( $prefs ) as $key ) {
 1416+ $attr = strtolower( $prefs[$key] );
 1417+ if ( !isset( $this->userInfo[0][$attr] ) ) {
 1418+ continue;
14421419 }
 1420+ $value = $this->userInfo[0][$attr][0];
 1421+ switch ( $key ) {
 1422+ case "email":
 1423+ $this->email = $value;
 1424+ $this->printDebug( "Retrieved email ($this->email) using attribute ($prefs[$key])", NONSENSITIVE );
 1425+ break;
 1426+ case "language":
 1427+ $this->lang = $value;
 1428+ $this->printDebug( "Retrieved language ($this->lang) using attribute ($prefs[$key])", NONSENSITIVE );
 1429+ break;
 1430+ case "nickname":
 1431+ $this->nickname = $value;
 1432+ $this->printDebug( "Retrieved nickname ($this->nickname) using attribute ($prefs[$key])", NONSENSITIVE );
 1433+ break;
 1434+ case "realname":
 1435+ $this->realname = $value;
 1436+ $this->printDebug( "Retrieved realname ($this->realname) using attribute ($prefs[$key])", NONSENSITIVE );
 1437+ break;
 1438+ }
14431439 }
14441440 }
14451441
——@@ -1497,10 +1493,8 @@
14981494
14991495 /**
15001496 * Function to get the user's groups.
1501 - *
1502 - * @access private
15031497 */
1504 - function getGroups( $username ) {
 1498+ private function getGroups( $username ) {
15051499 $this->printDebug( "Entering getGroups", NONSENSITIVE );
15061500
15071501 // Find groups
——@@ -1624,9 +1618,8 @@
16251619 *
16261620 * @param string $dn
16271621 * @return array
1628 - * @access private
16291622 */
1630 - function searchGroups( $dn ) {
 1623+ private function searchGroups( $dn ) {
16311624 $this->printDebug( "Entering searchGroups", NONSENSITIVE );
16321625
16331626 $base = $this->getBaseDN( GROUPDN );
——@@ -1637,8 +1630,9 @@
16381631
16391632 // We actually want to search for * not \2a, ensure we don't escape *
16401633 $value = $dn;
1641 - if ( $value != "*" )
 1634+ if ( $value != "*" ) {
16421635 $value = $this->getLdapEscapedString( $value );
 1636+ }
16431637
16441638 $proxyagent = $this->getConf( 'ProxyAgent' );
16451639 if ( $proxyagent ) {
——@@ -1662,6 +1656,7 @@
16631657 $Usid = $PGentries[0]['objectsid'][0];
16641658 $PGrid = $PGentries[0]['primarygroupid'][0];
16651659 $PGsid = bin2hex( $Usid );
 1660+ $PGSID = array();
16661661 for ( $i=0; $i < 56; $i += 2 ) {
16671662 $PGSID[] = substr( $PGsid, $i, 2 );
16681663 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r107428Simplify down, and don't fall through switch statements...reedy21:23, 27 December 2011

Comments

#Comment by Ryan lane (talk | contribs)   20:49, 27 December 2011

connect is called from OpenStackManager, and should be public.

Status & tagging log