r87152 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87151‎ | r87152 | r87153 >
Date:23:57, 29 April 2011
Author:brion
Status:ok
Tags:
Comment:
Revert r87129 "(bug 21196) Article::getContributors() no longer fail on PostgreSQL" -- breaks stuff under MySQL 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}


Seen trivially by going to login page while not logged in; some user check is failing and ending up with an improperly initialized object.
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,13 +157,10 @@
158158 /**
159159 * Bool Whether the cache variables have been loaded.
160160 */
161 - //@{
162 - var $mOptionsLoaded;
163 - private $mLoadedItems = array();
164 - //@}
 161+ var $mDataLoaded, $mAuthLoaded, $mOptionsLoaded;
165162
166163 /**
167 - * String Initialization data source if mLoadedItems!==true. May be one of:
 164+ * String Initialization data source if mDataLoaded==false. May be one of:
168165 * - 'defaults' anonymous user initialised from class defaults
169166 * - 'name' initialise from mName
170167 * - 'id' initialise from mId
——@@ -214,13 +211,13 @@
215212 * Load the user table data for this object from the source given by mFrom.
216213 */
217214 function load() {
218 - if ( $this->mLoadedItems === true ) {
 215+ if ( $this->mDataLoaded ) {
219216 return;
220217 }
221218 wfProfileIn( __METHOD__ );
222219
223220 # Set it now to avoid infinite recursion in accessors
224 - $this->mLoadedItems = true;
 221+ $this->mDataLoaded = true;
225222
226223 switch ( $this->mFrom ) {
227224 case 'defaults':
——@@ -339,7 +336,6 @@
340337 $u = new User;
341338 $u->mName = $name;
342339 $u->mFrom = 'name';
343 - $u->setItemLoaded( 'name' );
344340 return $u;
345341 }
346342 }
——@@ -354,7 +350,6 @@
355351 $u = new User;
356352 $u->mId = $id;
357353 $u->mFrom = 'id';
358 - $u->setItemLoaded( 'id' );
359354 return $u;
360355 }
361356
——@@ -395,14 +390,7 @@
396391
397392 /**
398393 * Create a new user object from a user row.
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 - *
 394+ * The row should have all fields from the user table in it.
407395 * @param $row Array A row from the user table
408396 * @return User
409397 */
——@@ -859,31 +847,6 @@
860848 }
861849
862850 /**
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 - /**
888851 * Load user data from the session or login cookie. If there are no valid
889852 * credentials, initialises the user as an anonymous user.
890853 * @return Bool True if the user is logged in, false otherwise.
——@@ -973,7 +936,7 @@
974937
975938 /**
976939 * Load user and user_group data from the database.
977 - * $this->mId must be set, this is how the user is identified.
 940+ * $this::mId must be set, this is how the user is identified.
978941 *
979942 * @return Bool True if the user exists, false if the user is anonymous
980943 * @private
——@@ -1013,51 +976,25 @@
1014977 * @param $row Array Row from the user table to load.
1015978 */
1016979 function loadFromRow( $row ) {
1017 - $all = true;
 980+ $this->mDataLoaded = true;
1018981
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 -
1034982 if ( isset( $row->user_id ) ) {
1035983 $this->mId = intval( $row->user_id );
1036 - $this->mFrom = 'id';
1037 - $this->setItemLoaded( 'id' );
1038 - } else {
1039 - $all = false;
1040984 }
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 - }
 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;
1062999 }
10631000
10641001 /**
——@@ -1095,7 +1032,7 @@
10961033 $this->mOptions = null;
10971034
10981035 if ( $reloadFrom ) {
1099 - $this->mLoadedItems = array();
 1036+ $this->mDataLoaded = false;
11001037 $this->mFrom = $reloadFrom;
11011038 }
11021039 }
——@@ -1524,7 +1461,7 @@
15251462 && User::isIP( $this->mName ) ) {
15261463 // Special case, we know the user is anonymous
15271464 return 0;
1528 - } elseif( !$this->isItemLoaded( 'id' ) ) {
 1465+ } elseif( $this->mId === null ) {
15291466 // Don't load if this was initialized from an ID
15301467 $this->load();
15311468 }
——@@ -1545,7 +1482,7 @@
15461483 * @return String User's name or IP address
15471484 */
15481485 function getName() {
1549 - if ( $this->isItemLoaded( 'name' ) ) {
 1486+ if ( !$this->mDataLoaded && $this->mFrom == 'name' ) {
15501487 # Special case optimisation
15511488 return $this->mName;
15521489 } else {
——@@ -1973,10 +1910,7 @@
19741911 * @return String User's real name
19751912 */
19761913 function getRealName() {
1977 - if ( !$this->isItemLoaded( 'realname' ) ) {
1978 - $this->load();
1979 - }
1980 -
 1914+ $this->load();
19811915 return $this->mRealName;
19821916 }
19831917
——@@ -2925,8 +2859,6 @@
29262860 */
29272861 function checkTemporaryPassword( $plaintext ) {
29282862 global $wgNewPasswordExpiry;
2929 -
2930 - $this->load();
29312863 if( self::comparePasswords( $this->mNewpassword, $plaintext, $this->getId() ) ) {
29322864 if ( is_null( $this->mNewpassTime ) ) {
29332865 return true;
——@@ -3242,11 +3174,9 @@
32433175 * non-existent/anonymous user accounts.
32443176 */
32453177 public function getRegistration() {
3246 - if ( $this->isAnon() ) {
3247 - return false;
3248 - }
3249 - $this->load();
3250 - return $this->mRegistration;
 3178+ return $this->getId() > 0
 3179+ ? $this->mRegistration
 3180+ : false;
32513181 }
32523182
32533183 /**
Index: trunk/phase3/includes/Article.php
——@@ -836,18 +836,11 @@
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 -
846840 $tables = array( 'revision', 'user' );
847841
848842 $fields = array(
849 - 'rev_user as user_id',
 843+ "$userTable.*",
850844 'rev_user_text AS user_name',
851 - $realNameField,
852845 'MAX(rev_timestamp) AS timestamp',
853846 );
854847
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 load $wgUser at this late stage just for statistics purposes
 393+ // Don't unstub $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->isItemLoaded( 'id' ) && $wgUser->isAnon() ) {
 395+ if ( $wgUser->mDataLoaded && $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->isItemLoaded( 'name' ) ) {
 672+ if ( is_object( $wgUser ) && $wgUser->mDataLoaded ) {
673673 $userName = $wgUser->getName();
674674 if ( mb_strlen( $userName ) > 15 ) {
675675 $userName = mb_substr( $userName, 0, 15 ) . '...';
Index: trunk/phase3/RELEASE-NOTES
——@@ -254,7 +254,6 @@
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.
259258 * (bug 28752) XCache doesn't work in CLI mode.
260259
261260 === API changes in 1.18 ===

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
r87129* (bug 21196) Article::getContributors() no longer fail on PostgreSQL...ialex18:27, 29 April 2011

Status & tagging log