r87129 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87128‎ | r87129 | r87130 >
Date:18:27, 29 April 2011
Author:ialex
Status:reverted (Comments)
Tags:
Comment:
* (bug 21196) Article::getContributors() no longer fail on PostgreSQL

Changed User::loadFromRow() to allow partial row (user_id, user_name, user_real_name can be set independently from other fields) userand removed User::$mDataLoaded, replaced by User::$mLoadedItems (marked as private) that can be an array with already loaded items or true when all data has been loaded. Changed GlobalFunctions.php and Database.php accordingly, no use of User::$mDataLoaded in extensions.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/User.php (modified) (history)
  • /trunk/phase3/includes/db/Database.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/User.php
——@@ -157,10 +157,13 @@
158158 /**
159159 * Bool Whether the cache variables have been loaded.
160160 */
161 - var $mDataLoaded, $mAuthLoaded, $mOptionsLoaded;
 161+ //@{
 162+ var $mOptionsLoaded;
 163+ private $mLoadedItems = array();
 164+ //@}
162165
163166 /**
164 - * String Initialization data source if mDataLoaded==false. May be one of:
 167+ * String Initialization data source if mLoadedItems!==true. May be one of:
165168 * - 'defaults' anonymous user initialised from class defaults
166169 * - 'name' initialise from mName
167170 * - 'id' initialise from mId
——@@ -211,13 +214,13 @@
212215 * Load the user table data for this object from the source given by mFrom.
213216 */
214217 function load() {
215 - if ( $this->mDataLoaded ) {
 218+ if ( $this->mLoadedItems === true ) {
216219 return;
217220 }
218221 wfProfileIn( __METHOD__ );
219222
220223 # Set it now to avoid infinite recursion in accessors
221 - $this->mDataLoaded = true;
 224+ $this->mLoadedItems = true;
222225
223226 switch ( $this->mFrom ) {
224227 case 'defaults':
——@@ -336,6 +339,7 @@
337340 $u = new User;
338341 $u->mName = $name;
339342 $u->mFrom = 'name';
 343+ $u->setItemLoaded( 'name' );
340344 return $u;
341345 }
342346 }
——@@ -350,6 +354,7 @@
351355 $u = new User;
352356 $u->mId = $id;
353357 $u->mFrom = 'id';
 358+ $u->setItemLoaded( 'id' );
354359 return $u;
355360 }
356361
——@@ -390,7 +395,14 @@
391396
392397 /**
393398 * Create a new user object from a user row.
394 - * The row should have all fields from the user table in it.
 399+ * The row should have the following fields from the user table in it:
 400+ * - either user_name or user_id to load further data if needed (or both)
 401+ * - user_real_name
 402+ * - all other fields (email, password, etc.)
 403+ * It is useless to provide the remaining fields if either user_id,
 404+ * user_name and user_real_name are not provided because the whole row
 405+ * will be loaded once more from the database when accessing them.
 406+ *
395407 * @param $row Array A row from the user table
396408 * @return User
397409 */
——@@ -847,6 +859,31 @@
848860 }
849861
850862 /**
 863+ * Return whether an item has been loaded.
 864+ *
 865+ * @param $item String: item to check. Current possibilities:
 866+ * - 'id'
 867+ * - name
 868+ * - realname
 869+ * @return Boolean
 870+ */
 871+ public function isItemLoaded( $item ) {
 872+ return $this->mLoadedItems === true || ( isset( $this->mLoadedItems[$item] )
 873+ && $this->mLoadedItems[$item] === true );
 874+ }
 875+
 876+ /**
 877+ * Set that an item has been loaded
 878+ *
 879+ * @param $item String
 880+ */
 881+ private function setItemLoaded( $item ) {
 882+ if ( is_array( $this->mLoadedItems ) ) {
 883+ $this->mLoadedItems[$item] = true;
 884+ }
 885+ }
 886+
 887+ /**
851888 * Load user data from the session or login cookie. If there are no valid
852889 * credentials, initialises the user as an anonymous user.
853890 * @return Bool True if the user is logged in, false otherwise.
——@@ -936,7 +973,7 @@
937974
938975 /**
939976 * Load user and user_group data from the database.
940 - * $this::mId must be set, this is how the user is identified.
 977+ * $this->mId must be set, this is how the user is identified.
941978 *
942979 * @return Bool True if the user exists, false if the user is anonymous
943980 * @private
——@@ -976,25 +1013,51 @@
9771014 * @param $row Array Row from the user table to load.
9781015 */
9791016 function loadFromRow( $row ) {
980 - $this->mDataLoaded = true;
 1017+ $all = true;
9811018
 1019+ if ( isset( $row->user_name ) ) {
 1020+ $this->mName = $row->user_name;
 1021+ $this->mFrom = 'name';
 1022+ $this->setItemLoaded( 'name' );
 1023+ } else {
 1024+ $all = false;
 1025+ }
 1026+
 1027+ if ( isset( $row->user_name ) ) {
 1028+ $this->mRealName = $row->user_real_name;
 1029+ $this->setItemLoaded( 'realname' );
 1030+ } else {
 1031+ $all = false;
 1032+ }
 1033+
9821034 if ( isset( $row->user_id ) ) {
9831035 $this->mId = intval( $row->user_id );
 1036+ $this->mFrom = 'id';
 1037+ $this->setItemLoaded( 'id' );
 1038+ } else {
 1039+ $all = false;
9841040 }
985 - $this->mName = $row->user_name;
986 - $this->mRealName = $row->user_real_name;
987 - $this->mPassword = $row->user_password;
988 - $this->mNewpassword = $row->user_newpassword;
989 - $this->mNewpassTime = wfTimestampOrNull( TS_MW, $row->user_newpass_time );
990 - $this->mEmail = $row->user_email;
991 - $this->decodeOptions( $row->user_options );
992 - $this->mTouched = wfTimestamp(TS_MW,$row->user_touched);
993 - $this->mToken = $row->user_token;
994 - $this->mEmailAuthenticated = wfTimestampOrNull( TS_MW, $row->user_email_authenticated );
995 - $this->mEmailToken = $row->user_email_token;
996 - $this->mEmailTokenExpires = wfTimestampOrNull( TS_MW, $row->user_email_token_expires );
997 - $this->mRegistration = wfTimestampOrNull( TS_MW, $row->user_registration );
998 - $this->mEditCount = $row->user_editcount;
 1041+
 1042+ if ( isset( $row->user_password ) ) {
 1043+ $this->mPassword = $row->user_password;
 1044+ $this->mNewpassword = $row->user_newpassword;
 1045+ $this->mNewpassTime = wfTimestampOrNull( TS_MW, $row->user_newpass_time );
 1046+ $this->mEmail = $row->user_email;
 1047+ $this->decodeOptions( $row->user_options );
 1048+ $this->mTouched = wfTimestamp(TS_MW,$row->user_touched);
 1049+ $this->mToken = $row->user_token;
 1050+ $this->mEmailAuthenticated = wfTimestampOrNull( TS_MW, $row->user_email_authenticated );
 1051+ $this->mEmailToken = $row->user_email_token;
 1052+ $this->mEmailTokenExpires = wfTimestampOrNull( TS_MW, $row->user_email_token_expires );
 1053+ $this->mRegistration = wfTimestampOrNull( TS_MW, $row->user_registration );
 1054+ $this->mEditCount = $row->user_editcount;
 1055+ } else {
 1056+ $all = false;
 1057+ }
 1058+
 1059+ if ( $all ) {
 1060+ $this->mLoadedItems = true;
 1061+ }
9991062 }
10001063
10011064 /**
——@@ -1032,7 +1095,7 @@
10331096 $this->mOptions = null;
10341097
10351098 if ( $reloadFrom ) {
1036 - $this->mDataLoaded = false;
 1099+ $this->mLoadedItems = array();
10371100 $this->mFrom = $reloadFrom;
10381101 }
10391102 }
——@@ -1461,7 +1524,7 @@
14621525 && User::isIP( $this->mName ) ) {
14631526 // Special case, we know the user is anonymous
14641527 return 0;
1465 - } elseif( $this->mId === null ) {
 1528+ } elseif( !$this->isItemLoaded( 'id' ) ) {
14661529 // Don't load if this was initialized from an ID
14671530 $this->load();
14681531 }
——@@ -1482,7 +1545,7 @@
14831546 * @return String User's name or IP address
14841547 */
14851548 function getName() {
1486 - if ( !$this->mDataLoaded && $this->mFrom == 'name' ) {
 1549+ if ( $this->isItemLoaded( 'name' ) ) {
14871550 # Special case optimisation
14881551 return $this->mName;
14891552 } else {
——@@ -1910,7 +1973,10 @@
19111974 * @return String User's real name
19121975 */
19131976 function getRealName() {
1914 - $this->load();
 1977+ if ( !$this->isItemLoaded( 'realname' ) ) {
 1978+ $this->load();
 1979+ }
 1980+
19151981 return $this->mRealName;
19161982 }
19171983
——@@ -2859,6 +2925,8 @@
28602926 */
28612927 function checkTemporaryPassword( $plaintext ) {
28622928 global $wgNewPasswordExpiry;
 2929+
 2930+ $this->load();
28632931 if( self::comparePasswords( $this->mNewpassword, $plaintext, $this->getId() ) ) {
28642932 if ( is_null( $this->mNewpassTime ) ) {
28652933 return true;
——@@ -3174,9 +3242,11 @@
31753243 * non-existent/anonymous user accounts.
31763244 */
31773245 public function getRegistration() {
3178 - return $this->getId() > 0
3179 - ? $this->mRegistration
3180 - : false;
 3246+ if ( $this->isAnon() ) {
 3247+ return false;
 3248+ }
 3249+ $this->load();
 3250+ return $this->mRegistration;
31813251 }
31823252
31833253 /**
Index: trunk/phase3/includes/Article.php
——@@ -836,11 +836,18 @@
837837 $dbr = wfGetDB( DB_SLAVE );
838838 $userTable = $dbr->tableName( 'user' );
839839
 840+ if ( $dbr->implicitGroupby() ) {
 841+ $realNameField = 'user_real_name';
 842+ } else {
 843+ $realNameField = 'FIRST(user_real_name) AS user_real_name';
 844+ }
 845+
840846 $tables = array( 'revision', 'user' );
841847
842848 $fields = array(
843 - "$userTable.*",
 849+ 'rev_user as user_id',
844850 'rev_user_text AS user_name',
 851+ $realNameField,
845852 'MAX(rev_timestamp) AS timestamp',
846853 );
847854
Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -389,9 +389,9 @@
390390 if ( $forward ) {
391391 $forward = "\t(proxied via {$_SERVER['REMOTE_ADDR']}{$forward})";
392392 }
393 - // Don't unstub $wgUser at this late stage just for statistics purposes
 393+ // Don't load $wgUser at this late stage just for statistics purposes
394394 // FIXME: We can detect some anons even if it is not loaded. See User::getId()
395 - if ( $wgUser->mDataLoaded && $wgUser->isAnon() ) {
 395+ if ( $wgUser->isItemLoaded( 'id' ) && $wgUser->isAnon() ) {
396396 $forward .= ' anon';
397397 }
398398 $log = sprintf( "%s\t%04.3f\t%s\n",
Index: trunk/phase3/includes/db/Database.php
——@@ -668,7 +668,7 @@
669669 # Add a comment for easy SHOW PROCESSLIST interpretation
670670 # if ( $fname ) {
671671 global $wgUser;
672 - if ( is_object( $wgUser ) && $wgUser->mDataLoaded ) {
 672+ if ( is_object( $wgUser ) && $wgUser->isItemLoaded( 'name' ) ) {
673673 $userName = $wgUser->getName();
674674 if ( mb_strlen( $userName ) > 15 ) {
675675 $userName = mb_substr( $userName, 0, 15 ) . '...';
Index: trunk/phase3/RELEASE-NOTES
——@@ -254,6 +254,7 @@
255255 * (bug 27249) "Installed software" table in Special:Version should always be
256256 left-to-right.
257257 * (bug 28719) Do not call mLinkHolders __destruct explicitly
 258+* (bug 21196) Article::getContributors() no longer fail on PostgreSQL.
258259
259260 === API changes in 1.18 ===
260261 * (bug 26339) Throw warning when truncating an overlarge API result.

Follow-up revisions

RevisionCommit summaryAuthorDate
r87143Followup to r85907, correctly quote table names....overlordq22:27, 29 April 2011
r87152Revert r87129 "(bug 21196) Article::getContributors() no longer fail on Postg...brion23:57, 29 April 2011
r87164Recommit r87129 and follow-ups but with a fix for the bug Brion found (sorry)ialex14:08, 30 April 2011
r90162Document from r87129 summaryplatonides21:23, 15 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r59123Allow $wgMaxCredits to work for database backends with non-magic group by beh...greg23:31, 15 November 2009

Comments

#Comment by OverlordQ (talk | contribs)   22:21, 29 April 2011

Can't find FIRST in MySQL or PG docu. Likely be able to cheat with MIN().

#Comment by IAlex (talk | contribs)   14:05, 30 April 2011

I found FIRST() is SQLite, and worked arround the non-presence in MySQL with the implicitGroupby() check, but I didn't check for PostgreSQL. Will MIN() work correctly for strings? (sometimes I would like ANY_VALUE_BECAUSE_I_KNOW_THEY_ARE_IDENTICAL()).

#Comment by Brion VIBBER (talk | contribs)   23:58, 29 April 2011

Causes wild breakage like this: 

SkinTemplate::makeTalkUrlDetails given invalid pagename User:

Backtrace:

#0 /var/www/trunk/includes/SkinTemplate.php(691): SkinTemplate->makeTalkUrlDetails('User:')
#1 /var/www/trunk/includes/SkinTemplate.php(495): SkinTemplate->buildPersonalUrls(Object(OutputPage))
#2 /var/www/trunk/includes/OutputPage.php(1906): SkinTemplate->outputPage(Object(OutputPage))
#3 /var/www/trunk/includes/Wiki.php(402): OutputPage->output()
#4 /var/www/trunk/index.php(146): MediaWiki->finalCleanup()
#5 {main}

Reverted in r87152.

Status & tagging log