MediaWiki r100286 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:< r100285‎ | r100286 (on ViewVC)‎ | r100287 >
Date:22:25, 19 October 2011
Author:aaron
Status:fixme (Comments)
Tags:core 
Comment:
* Tweaked Revision class to handle loading the current user name instead of rev_user_text. It still falls back to rev_user_text when building objects from DB rows for b/c.
* Moved JOIN conds to fetchFromConds() as that's where the tables are specified. This lets us avoid the same page_id=rev_page join conds plastered all over the code. Also, we can't mix WHERE and JOIN style join conds.
* Removed duplication in fetchFromConds() by using selectPageFields().
* Removed duplicate rev_parent_id field from contribs SELECT.

Yo make use of these changes, Pagers and lists still need to be updated to use Revision::selectUserFields() and join on the user table.
Modified paths:

Diff [purge]

Index: trunk/phase3/includes/Revision.php
——@@ -23,9 +23,7 @@
2424 * @return Revision or null
2525 */
2626 public static function newFromId( $id ) {
27 - return Revision::newFromConds(
28 - array( 'page_id=rev_page',
29 - 'rev_id' => intval( $id ) ) );
 27+ return Revision::newFromConds( array( 'rev_id' => intval( $id ) ) );
3028 }
3129
3230 /**
——@@ -57,7 +55,6 @@
5856 // Use a join to get the latest revision
5957 $conds[] = 'rev_id=page_latest';
6058 }
61 - $conds[] = 'page_id=rev_page';
6259 return Revision::newFromConds( $conds );
6360 }
6461
——@@ -85,7 +82,6 @@
8683 } else {
8784 $conds[] = 'rev_id = page_latest';
8885 }
89 - $conds[] = 'page_id=rev_page';
9086 return Revision::newFromConds( $conds );
9187 }
9288
——@@ -141,9 +137,7 @@
142138 * @return Revision or null
143139 */
144140 public static function loadFromId( $db, $id ) {
145 - return Revision::loadFromConds( $db,
146 - array( 'page_id=rev_page',
147 - 'rev_id' => intval( $id ) ) );
 141+ return Revision::loadFromConds( $db, array( 'rev_id' => intval( $id ) ) );
148142 }
149143
150144 /**
——@@ -157,7 +151,7 @@
158152 * @return Revision or null
159153 */
160154 public static function loadFromPageId( $db, $pageid, $id = 0 ) {
161 - $conds = array( 'page_id=rev_page','rev_page' => intval( $pageid ), 'page_id'=>intval( $pageid ) );
 155+ $conds = array( 'rev_page' => intval( $pageid ), 'page_id' => intval( $pageid ) );
162156 if( $id ) {
163157 $conds['rev_id'] = intval( $id );
164158 } else {
——@@ -182,12 +176,11 @@
183177 } else {
184178 $matchId = 'page_latest';
185179 }
186 - return Revision::loadFromConds(
187 - $db,
 180+ return Revision::loadFromConds( $db,
188181 array( "rev_id=$matchId",
189 - 'page_id=rev_page',
190182 'page_namespace' => $title->getNamespace(),
191 - 'page_title' => $title->getDBkey() ) );
 183+ 'page_title' => $title->getDBkey() )
 184+ );
192185 }
193186
194187 /**
——@@ -201,12 +194,11 @@
202195 * @return Revision or null
203196 */
204197 public static function loadFromTimestamp( $db, $title, $timestamp ) {
205 - return Revision::loadFromConds(
206 - $db,
 198+ return Revision::loadFromConds( $db,
207199 array( 'rev_timestamp' => $db->timestamp( $timestamp ),
208 - 'page_id=rev_page',
209200 'page_namespace' => $title->getNamespace(),
210 - 'page_title' => $title->getDBkey() ) );
 201+ 'page_title' => $title->getDBkey() )
 202+ );
211203 }
212204
213205 /**
——@@ -217,12 +209,12 @@
218210 */
219211 public static function newFromConds( $conditions ) {
220212 $db = wfGetDB( DB_SLAVE );
221 - $row = Revision::loadFromConds( $db, $conditions );
222 - if( is_null( $row ) && wfGetLB()->getServerCount() > 1 ) {
 213+ $rev = Revision::loadFromConds( $db, $conditions );
 214+ if( is_null( $rev ) && wfGetLB()->getServerCount() > 1 ) {
223215 $dbw = wfGetDB( DB_MASTER );
224 - $row = Revision::loadFromConds( $dbw, $conditions );
 216+ $rev = Revision::loadFromConds( $dbw, $conditions );
225217 }
226 - return $row;
 218+ return $rev;
227219 }
228220
229221 /**
——@@ -237,7 +229,6 @@
238230 $res = Revision::fetchFromConds( $db, $conditions );
239231 if( $res ) {
240232 $row = $res->fetchObject();
241 - $res->free();
242233 if( $row ) {
243234 $ret = new Revision( $row );
244235 return $ret;
——@@ -260,8 +251,8 @@
261252 wfGetDB( DB_SLAVE ),
262253 array( 'rev_id=page_latest',
263254 'page_namespace' => $title->getNamespace(),
264 - 'page_title' => $title->getDBkey(),
265 - 'page_id=rev_page' ) );
 255+ 'page_title' => $title->getDBkey() )
 256+ );
266257 }
267258
268259 /**
——@@ -274,16 +265,20 @@
275266 * @return ResultWrapper
276267 */
277268 private static function fetchFromConds( $db, $conditions ) {
278 - $fields = self::selectFields();
279 - $fields[] = 'page_namespace';
280 - $fields[] = 'page_title';
281 - $fields[] = 'page_latest';
 269+ $fields = array_merge(
 270+ self::selectFields(),
 271+ self::selectPageFields(),
 272+ self::selectUserFields()
 273+ );
282274 return $db->select(
283 - array( 'page', 'revision' ),
 275+ array( 'revision', 'page', 'user' ),
284276 $fields,
285277 $conditions,
286278 __METHOD__,
287 - array( 'LIMIT' => 1 ) );
 279+ array( 'LIMIT' => 1 ),
 280+ array( 'page' => array( 'INNER JOIN', 'page_id = rev_page' ),
 281+ 'user' => array( 'LEFT JOIN', 'user_id = rev_user' ) )
 282+ );
288283 }
289284
290285 /**
——@@ -329,6 +324,13 @@
330325 }
331326
332327 /**
 328+ * Return the list of user fields that should be selected from user table
 329+ */
 330+ static function selectUserFields() {
 331+ return array( 'COALESCE(user_name,rev_user_text) AS rev_user_name' );
 332+ }
 333+
 334+ /**
333335 * Constructor
334336 *
335337 * @param $row Mixed: either a database row or an array
——@@ -340,7 +342,6 @@
341343 $this->mPage = intval( $row->rev_page );
342344 $this->mTextId = intval( $row->rev_text_id );
343345 $this->mComment = $row->rev_comment;
344 - $this->mUserText = $row->rev_user_text;
345346 $this->mUser = intval( $row->rev_user );
346347 $this->mMinorEdit = intval( $row->rev_minor_edit );
347348 $this->mTimestamp = $row->rev_timestamp;
——@@ -374,6 +375,12 @@
375376 // 'text' table row entry will be lazy-loaded
376377 $this->mTextRow = null;
377378 }
 379+
 380+ if ( isset( $row->rev_user_name ) ) {
 381+ $this->mUserText = $row->rev_user_name; // current user name
 382+ } else { // fallback to rev_user_text
 383+ $this->mUserText = $row->rev_user_text; // de-normalized
 384+ }
378385 } elseif( is_array( $row ) ) {
379386 // Build a new revision to be saved...
380387 global $wgUser;

Sign-offs

UserFlagDate
Brion VIBBERinspected22:51, 19 October 2011
Brion VIBBERtested22:51, 19 October 2011

Follow-up revisions

Rev.Commit summaryAuthorDate
r100299FU r100286: Added sanity check to fetchFromConds() join condsaaron23:56, 19 October 2011
r100304FU r100300, r100286:...aaron00:41, 20 October 2011
r100326* Simplified r100286 by just using the field 'user_name' and removed the COAL...aaron03:03, 20 October 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   22:29, 19 October 2011

Should be "To make use" ;)

#Comment by Aaron Schulz (talk | contribs)   22:39, 19 October 2011

Ignore the "rev_parent_id" bit, that is from stuff I'm going to commit separately.

#Comment by Brion VIBBER (talk | contribs)   22:54, 19 October 2011

Nothing I see is exploding yet; individual calls via Revision::newFrom(Id|Title|PageId) seem to work fetching individual current or given revision ID. Moving the rev_page condition over into the join conditions seems to work ok with no side effects that I've come across.

I'm not sure we've used COALESCE() before, but it looks like it should work on MySQL, PostgreSQL, SQLite, MS SQL, and Oracle.

Various pagers that are still pulling general fields may not be seeing the user fields at this time, so will keep on using whatever's in rev_text.

A possible issue: if a user table entry for id=0 gets into the system, its username will start taking over from the data stored in rev_user_text. That shouldn't happen of course, but ....

#Comment by Aaron Schulz (talk | contribs)   22:56, 19 October 2011

In pagers I plan on adding rev_user != 0, I guess I forgot to add that sanity check/optimization here.

#Comment by Brion VIBBER (talk | contribs)   22:56, 19 October 2011

On a more future-focused view as well; good user-info components in big table lists etc may at some point want to pull in much more user info, such as real name, avatar, user groups, etc. Longer-term it's maybe better to encourage a batch-lookup and common output system for user references pulled from whatever sources, so every query doesn't have to have multiple fields added to it with a user join.

#Comment by Platonides (talk | contribs)   12:38, 19 May 2012

What's the point of this? For instance you're now joining two tables in action=history when you have all the needed data in a single one.

Status & tagging log

  • 14:32, 19 May 2012 Platonides (talk | contribs) changed the status of r100286 [removed: resolved added: fixme]
  • 19:19, 2 February 2012 Catrope (talk | contribs) changed the status of r100286 [removed: new added: resolved]
  • 05:47, 5 January 2012 RobLa-WMF (talk | contribs) changed the tags for r100286 [added: core]
Retrieved from "http://www.mediawiki.org/wiki/Special:Code/MediaWiki/100286"