r85872 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85871‎ | r85872 | r85873 >
Date:12:09, 12 April 2011
Author:happy-melon
Status:ok (Comments)
Tags:
Comment:
Clean up some direct $db->query($sql) calls. Remove limit/offset parameters from Article::getContributors() as they weren't being used anywhere and probably didn't work properly anyway.
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/Export.php (modified) (history)
  • /trunk/phase3/includes/parser/LinkHolderArray.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Article.php
——@@ -799,46 +799,47 @@
800800 }
801801
802802 /**
803 - * FIXME: this does what?
804 - * @param $limit Integer: default 0.
805 - * @param $offset Integer: default 0.
806 - * @return UserArrayFromResult object with User objects of article contributors for requested range
 803+ * Get a list of users who have edited this article, not including the user who made
 804+ * the most recent revision, which you can get from $article->getUser() if you want it
 805+ * @return UserArray
807806 */
808 - public function getContributors( $limit = 0, $offset = 0 ) {
 807+ public function getContributors() {
809808 # FIXME: this is expensive; cache this info somewhere.
810809
811810 $dbr = wfGetDB( DB_SLAVE );
812 - $revTable = $dbr->tableName( 'revision' );
813811 $userTable = $dbr->tableName( 'user' );
814812
815 - $pageId = $this->getId();
 813+ $tables = array( 'revision', 'user' );
816814
 815+ $fields = array(
 816+ "$userTable.*",
 817+ 'rev_user_text AS user_name',
 818+ 'MAX(rev_timestamp) AS timestamp',
 819+ );
 820+
 821+ $conds = array( 'rev_page' => $this->getId() );
 822+
 823+ // The user who made the top revision gets credited as "this page was last edited by
 824+ // John, based on contributions by Tom, Dick and Harry", so don't include them twice.
817825 $user = $this->getUser();
818 -
819826 if ( $user ) {
820 - $excludeCond = "AND rev_user != $user";
 827+ $conds[] = "rev_user != $user";
821828 } else {
822 - $userText = $dbr->addQuotes( $this->getUserText() );
823 - $excludeCond = "AND rev_user_text != $userText";
 829+ $conds[] = "rev_user_text != {$dbr->addQuotes( $this->getUserText() )}";
824830 }
825831
826 - $deletedBit = $dbr->bitAnd( 'rev_deleted', Revision::DELETED_USER ); // username hidden?
 832+ $conds[] = "{$dbr->bitAnd( 'rev_deleted', Revision::DELETED_USER )} = 0"; // username hidden?
827833
828 - $sql = "SELECT {$userTable}.*, rev_user_text as user_name, MAX(rev_timestamp) as timestamp
829 - FROM $revTable LEFT JOIN $userTable ON rev_user = user_id
830 - WHERE rev_page = $pageId
831 - $excludeCond
832 - AND $deletedBit = 0
833 - GROUP BY rev_user, rev_user_text
834 - ORDER BY timestamp DESC";
 834+ $jconds = array(
 835+ 'user' => array( 'LEFT JOIN', 'rev_user = user_id' ),
 836+ );
835837
836 - if ( $limit > 0 ) {
837 - $sql = $dbr->limitResult( $sql, $limit, $offset );
838 - }
839 -
840 - $sql .= ' ' . $this->getSelectOptions();
841 - $res = $dbr->query( $sql, __METHOD__ );
842 -
 838+ $options = array(
 839+ 'GROUP BY' => array( 'rev_user', 'rev_user_text' ),
 840+ 'ORDER BY' => 'timestamp DESC',
 841+ );
 842+
 843+ $res = $dbr->select( $tables, $fields, $conds, __METHOD__, $options, $jconds );
843844 return new UserArrayFromResult( $res );
844845 }
845846
——@@ -3618,10 +3619,11 @@
36193620
36203621 $dbw = wfGetDB( DB_MASTER );
36213622 $cutoff = $dbw->timestamp( time() - $wgRCMaxAge );
3622 - $recentchanges = $dbw->tableName( 'recentchanges' );
3623 - $sql = "DELETE FROM $recentchanges WHERE rc_timestamp < '{$cutoff}'";
3624 -
3625 - $dbw->query( $sql );
 3623+ $dbw->delete(
 3624+ 'recentchanges',
 3625+ array( "rc_timestamp < '$cutoff'" ),
 3626+ __METHOD__
 3627+ );
36263628 }
36273629 }
36283630
Index: trunk/phase3/includes/parser/LinkHolderArray.php
——@@ -270,6 +270,7 @@
271271 # Generate query
272272 $query = false;
273273 $current = null;
 274+ $queries = array();
274275 foreach ( $this->internals as $ns => $entries ) {
275276 foreach ( $entries as $entry ) {
276277 $title = $entry['title'];
——@@ -294,25 +295,28 @@
295296 $colours[$pdbk] = 'new';
296297 } else {
297298 # Not in the link cache, add it to the query
298 - if ( !isset( $current ) ) {
299 - $current = $ns;
300 - $query = "SELECT page_id, page_namespace, page_title, page_is_redirect, page_len, page_latest";
301 - $query .= " FROM $page WHERE (page_namespace=$ns AND page_title IN(";
302 - } elseif ( $current != $ns ) {
303 - $current = $ns;
304 - $query .= ")) OR (page_namespace=$ns AND page_title IN(";
305 - } else {
306 - $query .= ', ';
307 - }
308 -
309 - $query .= $dbr->addQuotes( $title->getDBkey() );
 299+ $queries[$ns][] = $title->getDBkey();
310300 }
311301 }
312302 }
313 - if ( $query ) {
314 - $query .= '))';
 303+ if ( $queries ) {
 304+ $where = array();
 305+ foreach( $queries as $ns => $pages ){
 306+ $where[] = $dbr->makeList(
 307+ array(
 308+ 'page_namespace' => $ns,
 309+ 'page_title' => $pages,
 310+ ),
 311+ LIST_AND
 312+ );
 313+ }
315314
316 - $res = $dbr->query( $query, __METHOD__ );
 315+ $res = $dbr->select(
 316+ 'page',
 317+ array( 'page_id', 'page_namespace', 'page_title', 'page_is_redirect', 'page_len', 'page_latest' ),
 318+ $dbr->makeList( $where, LIST_OR ),
 319+ __METHOD__
 320+ );
317321
318322 # Fetch data and form into an associative array
319323 # non-existent = broken
Index: trunk/phase3/includes/Export.php
——@@ -157,17 +157,23 @@
158158 # Generates the distinct list of authors of an article
159159 # Not called by default (depends on $this->list_authors)
160160 # Can be set by Special:Export when not exporting whole history
161 - protected function do_list_authors( $page , $revision , $cond ) {
 161+ protected function do_list_authors( $cond ) {
162162 wfProfileIn( __METHOD__ );
163163 $this->author_list = "<contributors>";
164164 // rev_deleted
165 - $nothidden = '(' . $this->db->bitAnd( 'rev_deleted', Revision::DELETED_USER ) . ') = 0';
166165
167 - $sql = "SELECT DISTINCT rev_user_text,rev_user FROM {$page},{$revision}
168 - WHERE page_id=rev_page AND $nothidden AND " . $cond ;
169 - $result = $this->db->query( $sql, __METHOD__ );
170 - $resultset = $this->db->resultObject( $result );
171 - foreach ( $resultset as $row ) {
 166+ $res = $this->db->select(
 167+ array( 'page', 'revision' ),
 168+ array( 'DISTINCT rev_user_text', 'rev_user' ),
 169+ array(
 170+ $this->db->bitAnd( 'rev_deleted', Revision::DELETED_USER ) . ' = 0',
 171+ $cond,
 172+ 'page_id = rev_id',
 173+ ),
 174+ __METHOD__
 175+ );
 176+
 177+ foreach ( $res as $row ) {
172178 $this->author_list .= "<contributor>" .
173179 "<username>" .
174180 htmlentities( $row->rev_user_text ) .
——@@ -240,8 +246,7 @@
241247 } elseif ( $this->history & WikiExporter::CURRENT ) {
242248 # Latest revision dumps...
243249 if ( $this->list_authors && $cond != '' ) { // List authors, if so desired
244 - list( $page, $revision ) = $this->db->tableNamesN( 'page', 'revision' );
245 - $this->do_list_authors( $page, $revision, $cond );
 250+ $this->do_list_authors( $cond );
246251 }
247252 $join['revision'] = array( 'INNER JOIN', 'page_id=rev_page AND page_latest=rev_id' );
248253 } elseif ( $this->history & WikiExporter::STABLE ) {

Comments

#Comment by Aaron Schulz (talk | contribs)   00:26, 21 June 2011

In LinkHolderArray.php:

Unless mysql is really smart this will probably hurt performance somewhat. Can you keep the namespace condition clause chunked (NS = x AND (title = a OR title = b OR...)) as it was before rather than one for each title?

#Comment by Happy-melon (talk | contribs)   23:54, 26 June 2011

AFAICT, this is what it does; the query is something like 

SELECT '*' FROM `page` WHERE (
    ( `page_namespace` = 0 AND `page_title` IN ( 'foo', 'bar', 'baz' ) ) OR
    ( `page_namespace` = 1 AND `page_title` IN ( 'baz', 'nada', 'quok' ) ) OR
    ( `page_namespace` = 2 AND `page_title` = 'blat' ) )

Am I misunderstanding the problem?

#Comment by Aaron Schulz (talk | contribs)   00:05, 27 June 2011

Nevermind, it *is* what it should be. Maybe I thought that $pages in foreach( $queries as $ns => $pages ){ was singular...

Status & tagging log