r102290 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r102289‎ | r102290 | r102291 >
Date:15:37, 7 November 2011
Author:ashley
Status:deferred
Tags:socialprofile 
Comment:
SocialProfile: as per Markus' in-depth review:
*var -> public
*use DB_SLAVE for reads
*move inline CSS to the CSS file
*avoid leaking variables to global scope in JS
*made SpecialUpdateProfile's formatBirthday functions public and static in preparation to converting the UserProfile class to use those; did not unify UserProfile's and SpecialUpdateProfile's birthday functions yet
*removed some dead code
*moved some duplicated code in UserProfilePage into a function
Modified paths:
  • /trunk/extensions/SocialProfile/UserProfile/AvatarClass.php (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/SpecialEditProfile.php (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/SpecialToggleUserPageType.php (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/SpecialUpdateProfile.php (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/UpdateProfile.js (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/UserProfile.css (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/UserProfileClass.php (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/UserProfilePage.js (modified) (history)
  • /trunk/extensions/SocialProfile/UserProfile/UserProfilePage.php (modified) (history)

Diff [purge]

Index: trunk/extensions/SocialProfile/UserProfile/UserProfilePage.php
@@ -11,12 +11,12 @@
1212
1313 class UserProfilePage extends Article {
1414
15 - var $title = null;
 15+ public $title = null;
1616
1717 /**
1818 * Constructor
1919 */
20 - function __construct( &$title ) {
 20+ function __construct( $title ) {
2121 global $wgUser;
2222 parent::__construct( $title );
2323 $this->user_name = $title->getText();
@@ -219,7 +219,7 @@
220220 array(
221221 'page_title', 'UNIX_TIMESTAMP(poll_date) AS poll_date'
222222 ),
223 - /*where*/ array( 'poll_user_id' => $this->user_id ),
 223+ /* WHERE */array( 'poll_user_id' => $this->user_id ),
224224 __METHOD__,
225225 array( 'ORDER BY' => 'poll_id DESC', 'LIMIT' => 3 ),
226226 array( 'page' => array( 'INNER JOIN', 'page_id = poll_page_id' ) )
@@ -516,10 +516,7 @@
517517 $user_level = new UserLevel( $stats_data['points'] );
518518 $level_link = Title::makeTitle( NS_HELP, wfMsgForContent( 'user-profile-userlevels-link' ) );
519519
520 - if ( !$this->profile_data ) {
521 - $profile = new UserProfile( $user_name );
522 - $this->profile_data = $profile->getProfile();
523 - }
 520+ $this->initializeProfileData( $user_name );
524521 $profile_data = $this->profile_data;
525522
526523 $defaultCountry = wfMsgForContent( 'user-profile-default-country' );
@@ -636,10 +633,7 @@
637634 return '';
638635 }
639636
640 - if ( !$this->profile_data ) {
641 - $profile = new UserProfile( $user_name );
642 - $this->profile_data = $profile->getProfile();
643 - }
 637+ $this->initializeProfileData( $user_name );
644638
645639 $profile_data = $this->profile_data;
646640
@@ -707,10 +701,7 @@
708702 return '';
709703 }
710704
711 - if ( !$this->profile_data ) {
712 - $profile = new UserProfile( $user_name );
713 - $this->profile_data = $profile->getProfile();
714 - }
 705+ $this->initializeProfileData( $user_name );
715706
716707 $profile_data = $this->profile_data;
717708 $joined_data = $profile_data['movies'] . $profile_data['tv'] .
@@ -747,7 +738,7 @@
748739 $this->getProfileSection( wfMsg( 'other-info-snacks' ), $profile_data['snacks'], false ) .
749740 $this->getProfileSection( wfMsg( 'other-info-drinks' ), $profile_data['drinks'], false ) .
750741 '</div>';
751 - } elseif ( $wgUser->getName() == $user_name ) {
 742+ } elseif ( $this->isOwner() ) {
752743 $output .= '<div class="user-section-heading">
753744 <div class="user-section-title">' .
754745 wfMsg( 'other-info-title' ) .
@@ -786,10 +777,7 @@
787778 $user_level = new UserLevel( $stats_data['points'] );
788779 $level_link = Title::makeTitle( NS_HELP, wfMsgForContent( 'user-profile-userlevels-link' ) );
789780
790 - if ( !$this->profile_data ) {
791 - $profile = new UserProfile( $user_name );
792 - $this->profile_data = $profile->getProfile();
793 - }
 781+ $this->initializeProfileData( $user_name );
794782 $profile_data = $this->profile_data;
795783
796784 // Variables and other crap
@@ -942,7 +930,7 @@
943931 $avatar = new wAvatar( $this->user_id, 'l' );
944932 $avatarTitle = SpecialPage::getTitleFor( 'UploadAvatar' );
945933
946 - $output .= '<div class="profile-image">';
 934+ $output = '<div class="profile-image">';
947935 if ( $wgUser->getName() == $this->user_name ) {
948936 if ( strpos( $avatar->getAvatarImage(), 'default_' ) != false ) {
949937 $caption = 'upload image';
@@ -1234,63 +1222,6 @@
12351223 $by_type .= $item;
12361224 }
12371225 $output .= "<div id=\"recent-all\">$by_type</div>";
1238 -
1239 - $by_type = '';
1240 - if ( isset( $items_html_type['edit'] ) && is_array( $items_html_type['edit'] ) ) {
1241 - foreach ( $items_html_type['edit'] as $item ) {
1242 - $by_type .= $item;
1243 - }
1244 - }
1245 -
1246 - $by_type = '';
1247 - if ( isset( $items_html_type['comment'] ) && is_array( $items_html_type['comment'] ) ) {
1248 - foreach ( $items_html_type['comment'] as $item ) {
1249 - $by_type .= $item;
1250 - }
1251 - }
1252 -
1253 - $by_type = '';
1254 - if ( isset( $items_html_type['gift-sent'] ) && is_array( $items_html_type['gift-sent'] ) ) {
1255 - foreach ( $items_html_type['gift-sent'] as $item ) {
1256 - $by_type .= $item;
1257 - }
1258 - }
1259 -
1260 - $by_type = '';
1261 - if ( isset( $items_html_type['gift-rec'] ) && is_array( $items_html_type['gift-rec'] ) ) {
1262 - foreach ( $items_html_type['gift-rec'] as $item ) {
1263 - $by_type .= $item;
1264 - }
1265 - }
1266 -
1267 - $by_type = '';
1268 - if ( isset( $items_html_type['system_gift'] ) && is_array( $items_html_type['system_gift'] ) ) {
1269 - foreach ( $items_html_type['system_gift'] as $item ) {
1270 - $by_type .= $item;
1271 - }
1272 - }
1273 -
1274 - $by_type = '';
1275 - if ( isset( $items_html_type['friend'] ) && is_array( $items_html_type['friend'] ) ) {
1276 - foreach ( $items_html_type['friend'] as $item ) {
1277 - $by_type .= $item;
1278 - }
1279 - }
1280 -
1281 - $by_type = '';
1282 - if ( isset( $items_html_type['foe'] ) && is_array( $items_html_type['foe'] ) ) {
1283 - foreach ( $items_html_type['foe'] as $item ) {
1284 - $by_type .= $item;
1285 - }
1286 - }
1287 -
1288 - $by_type = '';
1289 - if ( isset( $items_html_type['system_message'] ) && is_array( $items_html_type['system_message'] ) ) {
1290 - foreach ( $items_html_type['system_message'] as $item ) {
1291 - $by_type .= $item;
1292 - }
1293 - }
1294 -
12951226 }
12961227
12971228 return $output;
@@ -1548,7 +1479,7 @@
15491480 if ( $wgUser->isLoggedIn() && !$wgUser->isBlocked() ) {
15501481 $output .= '<div class="user-page-message-form">
15511482 <input type="hidden" id="user_name_to" name="user_name_to" value="' . addslashes( $user_name ) . '" />
1552 - <span style="color:#797979;">' .
 1483+ <span class="profile-board-message-type">' .
15531484 wfMsgHtml( 'userboard_messagetype' ) .
15541485 '</span>
15551486 <select id="message_type">
@@ -1782,4 +1713,10 @@
17831714 return $output;
17841715 }
17851716
 1717+ private function initializeProfileData( $username ) {
 1718+ if ( !$this->profile_data ) {
 1719+ $profile = new UserProfile( $username );
 1720+ $this->profile_data = $profile->getProfile();
 1721+ }
 1722+ }
17861723 }
Index: trunk/extensions/SocialProfile/UserProfile/UserProfile.css
@@ -652,3 +652,8 @@
653653 color: #fff;
654654 font-weight: bold;
655655 }
 656+
 657+/* The text "Message type" on the left side of the message type selector on profile page */
 658+.profile-board-message-type {
 659+ color: #797979;
 660+}
\ No newline at end of file
Index: trunk/extensions/SocialProfile/UserProfile/AvatarClass.php
@@ -12,9 +12,9 @@
1313 * @ingroup Extensions
1414 */
1515 class wAvatar {
16 - var $user_name = null;
17 - var $user_id;
18 - var $avatar_type = 0;
 16+ public $user_name = null;
 17+ public $user_id;
 18+ public $avatar_type = 0;
1919
2020 /**
2121 * Constructor
Index: trunk/extensions/SocialProfile/UserProfile/SpecialUpdateProfile.php
@@ -39,7 +39,6 @@
4040 __METHOD__
4141 );
4242 if ( $s === false ) {
43 - $dbw = wfGetDB( DB_MASTER );
4443 $dbw->insert(
4544 'user_profile',
4645 array( 'up_user_id' => $user->getID() ),
@@ -222,7 +221,7 @@
223222 wfRunHooks( 'SpecialUpdateProfile::saveSettings_pref', array( $this, $wgRequest ) );
224223 }
225224
226 - function formatBirthdayDB( $birthday ) {
 225+ public static function formatBirthdayDB( $birthday ) {
227226 $dob = explode( '/', $birthday );
228227 if ( count( $dob ) == 2 || count( $dob ) == 3 ) {
229228 $year = isset( $dob[2] ) ? $dob[2] : 2007;
@@ -235,7 +234,7 @@
236235 return ( $birthday_date );
237236 }
238237
239 - function formatBirthday( $birthday, $showYOB = false ) {
 238+ public static function formatBirthday( $birthday, $showYOB = false ) {
240239 $dob = explode( '-', $birthday );
241240 if ( count( $dob ) == 3 ) {
242241 $month = $dob[1];
@@ -275,7 +274,7 @@
276275 'up_hometown_state' => $wgRequest->getVal( 'hometown_state' ),
277276 'up_hometown_country' => $wgRequest->getVal( 'hometown_country' ),
278277
279 - 'up_birthday' => $this->formatBirthdayDB( $wgRequest->getVal( 'birthday' ) ),
 278+ 'up_birthday' => self::formatBirthdayDB( $wgRequest->getVal( 'birthday' ) ),
280279 'up_about' => $wgRequest->getVal( 'about' ),
281280 'up_occupation' => $wgRequest->getVal( 'occupation' ),
282281 'up_schools' => $wgRequest->getVal( 'schools' ),
@@ -375,7 +374,7 @@
376375 function displayBasicForm( $user ) {
377376 global $wgRequest, $wgUser, $wgOut;
378377
379 - $dbr = wfGetDB( DB_MASTER );
 378+ $dbr = wfGetDB( DB_SLAVE );
380379 $s = $dbr->selectRow( 'user_profile',
381380 array(
382381 'up_location_city', 'up_location_state', 'up_location_country',
@@ -398,7 +397,7 @@
399398 $hometown_state = $s->up_hometown_state;
400399 $hometown_country = $s->up_hometown_country;
401400 $showYOB = $wgUser->getIntOption( 'showyearofbirth', !isset( $s->up_birthday ) ) == 1;
402 - $birthday = $this->formatBirthday( $s->up_birthday, $showYOB );
 401+ $birthday = self::formatBirthday( $s->up_birthday, $showYOB );
403402 $schools = $s->up_schools;
404403 $places = $s->up_places_lived;
405404 $websites = $s->up_websites;
@@ -473,7 +472,7 @@
474473 }
475474
476475 $form .= '</select>';
477 - $form .= '</p>
 476+ $form .= '</p>
478477 <div class="cleared"></div>
479478 </div>
480479 <div class="cleared"></div>';
@@ -582,8 +581,9 @@
583582 function displayPersonalForm( $user ) {
584583 global $wgRequest, $wgUser, $wgOut;
585584
586 - $dbr = wfGetDB( DB_MASTER );
587 - $s = $dbr->selectRow( 'user_profile',
 585+ $dbr = wfGetDB( DB_SLAVE );
 586+ $s = $dbr->selectRow(
 587+ 'user_profile',
588588 array(
589589 'up_about', 'up_places_lived', 'up_websites', 'up_relationship',
590590 'up_occupation', 'up_companies', 'up_schools', 'up_movies',
@@ -676,7 +676,7 @@
677677 function displayPreferencesForm() {
678678 global $wgRequest, $wgUser, $wgOut;
679679
680 - $dbr = wfGetDB( DB_MASTER );
 680+ $dbr = wfGetDB( DB_SLAVE );
681681 $s = $dbr->selectRow(
682682 'user_profile',
683683 array( 'up_birthday' ),
Index: trunk/extensions/SocialProfile/UserProfile/SpecialEditProfile.php
@@ -158,7 +158,7 @@
159159 function displayBasicForm( $tar ) {
160160 global $wgRequest, $wgUser, $wgOut;
161161
162 - $dbr = wfGetDB( DB_MASTER );
 162+ $dbr = wfGetDB( DB_SLAVE );
163163 $s = $dbr->selectRow( 'user_profile',
164164 array(
165165 'up_location_city', 'up_location_state', 'up_location_country',
@@ -179,7 +179,7 @@
180180 $hometown_city = $s->up_hometown_city;
181181 $hometown_state = $s->up_hometown_state;
182182 $hometown_country = $s->up_hometown_country;
183 - $birthday = $this->formatBirthday( $s->up_birthday, true );
 183+ $birthday = self::formatBirthday( $s->up_birthday, true );
184184 $schools = $s->up_schools;
185185 $places = $s->up_places_lived;
186186 $websites = $s->up_websites;
@@ -254,7 +254,7 @@
255255 }
256256
257257 $form .= '</select>';
258 - $form .= '</p>
 258+ $form .= '</p>
259259 <div class="cleared"></div>
260260 </div>
261261 <div class="cleared"></div>';
@@ -349,7 +349,7 @@
350350 function displayPersonalForm( $tar ) {
351351 global $wgRequest, $wgUser, $wgOut;
352352
353 - $dbr = wfGetDB( DB_MASTER );
 353+ $dbr = wfGetDB( DB_SLAVE );
354354 $s = $dbr->selectRow( 'user_profile',
355355 array(
356356 'up_about', 'up_places_lived', 'up_websites', 'up_relationship',
@@ -438,7 +438,7 @@
439439 function displayCustomForm( $tar ) {
440440 global $wgRequest, $wgUser, $wgOut;
441441
442 - $dbr = wfGetDB( DB_MASTER );
 442+ $dbr = wfGetDB( DB_SLAVE );
443443 $s = $dbr->selectRow(
444444 'user_profile',
445445 array(
Index: trunk/extensions/SocialProfile/UserProfile/SpecialToggleUserPageType.php
@@ -38,19 +38,18 @@
3939 return;
4040 }
4141
42 - $dbr = wfGetDB( DB_MASTER );
43 - $s = $dbr->selectRow(
 42+ $dbw = wfGetDB( DB_MASTER );
 43+ $s = $dbw->selectRow(
4444 'user_profile',
4545 array( 'up_user_id' ),
4646 array( 'up_user_id' => $wgUser->getID() ),
4747 __METHOD__
4848 );
4949 if ( $s === false ) {
50 - $dbw = wfGetDB( DB_MASTER );
51 - $dbw->insert( 'user_profile',
52 - array(
53 - 'up_user_id' => $wgUser->getID()
54 - ), __METHOD__
 50+ $dbw->insert(
 51+ 'user_profile',
 52+ array( 'up_user_id' => $wgUser->getID() ),
 53+ __METHOD__
5554 );
5655 }
5756
@@ -59,8 +58,8 @@
6059
6160 $user_page_type = ( ( $profile_data['user_page_type'] == 1 ) ? 0 : 1 );
6261
63 - $dbw = wfGetDB( DB_MASTER );
64 - $dbw->update( 'user_profile',
 62+ $dbw->update(
 63+ 'user_profile',
6564 /* SET */array(
6665 'up_type' => $user_page_type
6766 ),
Index: trunk/extensions/SocialProfile/UserProfile/UserProfilePage.js
@@ -58,7 +58,7 @@
5959 document.getElementById( 'upload-frame-errors' ).innerHTML = '';
6060 oldHtml = document.getElementById( 'mini-gallery-' + replaceID ).innerHTML;
6161
62 - for( x = 7; x > 0; x-- ) {
 62+ for( var x = 7; x > 0; x-- ) {
6363 document.getElementById( 'mini-gallery-' + ( x ) ).innerHTML =
6464 document.getElementById( 'mini-gallery-' + ( x - 1 ) ).innerHTML.replace( 'slideShowLink(' + ( x - 1 ) + ')','slideShowLink(' + ( x ) + ')' );
6565 }
Index: trunk/extensions/SocialProfile/UserProfile/UserProfileClass.php
@@ -6,25 +6,25 @@
77 /**
88 * @var Integer: the current user's user ID. Set in the constructor.
99 */
10 - var $user_id;
 10+ public $user_id;
1111
1212 /**
1313 * @var String: the current user's user name. Set in the constructor.
1414 */
15 - var $user_name;
 15+ public $user_name;
1616
1717 /** unused, remove me? */
18 - var $profile;
 18+ public $profile;
1919
2020 /**
2121 * @var Integer: used in getProfileComplete()
2222 */
23 - var $profile_fields_count;
 23+ public $profile_fields_count;
2424
2525 /**
2626 * @var Array: array of valid profile fields; used in getProfileComplete()
2727 */
28 - var $profile_fields = array(
 28+ public $profile_fields = array(
2929 'real_name',
3030 'location_city',
3131 'hometown_city',
@@ -51,7 +51,7 @@
5252 /**
5353 * @var Array: unused, remove me?
5454 */
55 - var $profile_missing = array();
 55+ public $profile_missing = array();
5656
5757 /**
5858 * Constructor
@@ -118,7 +118,7 @@
119119 $profile['hometown_city'] = isset( $row->up_hometown_city ) ? $row->up_hometown_city : '';
120120 $profile['hometown_state'] = isset( $row->up_hometown_state ) ? $row->up_hometown_state : '';
121121 $profile['hometown_country'] = isset( $row->up_hometown_country ) ? $row->up_hometown_country : '';
122 - $profile['birthday'] = $this->formatBirthday( $issetUpBirthday, $showYOB);
 122+ $profile['birthday'] = $this->formatBirthday( $issetUpBirthday, $showYOB );
123123
124124 $profile['about'] = isset( $row->up_about ) ? $row->up_about : '';
125125 $profile['places_lived'] = isset( $row->up_places_lived ) ? $row->up_places_lived : '';
Index: trunk/extensions/SocialProfile/UserProfile/UpdateProfile.js
@@ -22,13 +22,13 @@
2323
2424 function displaySection( id, country, section ) {
2525 country_id = -1;
26 - for( x = 0; x <= countries.length-1; x++ ) {
 26+ for( var x = 0; x <= countries.length-1; x++ ) {
2727 if( country == countries[x].country ) {
2828 country_id = x;
2929 }
3030 }
3131
32 - section_select = '';
 32+ var section_select = '';
3333 if( countries[country_id] ) {
3434 document.getElementById( id + '_label' ).innerHTML = countries[country_id].name;
3535 section_select += '<select class="profile-form" name="' + id + '" id="' + id + '"><option></option>';

Status & tagging log