r61818 MediaWiki - Code Review archive

+		
+		//get the total count across all pages
+		$dbr =& wfGetDB(DB_SLAVE);
+		$revCountRes = $this->getRevCountQuery($dbr);
+		$revCount = 0;		 
+		if ($dbr->numRows($revCountRes) > 0){
+			$row = $dbr->fetchObject($revCountRes);			
+			$revCount = $row->rev_count;
+		}

Please read Manual:Coding conventions, and as a general rule, follow the coding style of the file you're in. There's other instances of inconsistent coding/whitespace style in this commit as well.

Also, there's a selectRow() function in the DatabaseBase class, which is probably what you're looking for here. See the docs at http://svn.wikimedia.org/doc/classDatabaseBase.html

 			$pager->getLimitForm() .
+			'</td><td style="padding-left: 2em;">' . 
+			' Total number of results: <span style="font-weight: bold;">' . $revCount . '</span>' . 
+			'</td></tr></table>' .  

Don't hardcode English text ever. Add a message in CodeReview.i18n.php and use wfMsg( 'messagekey' ) to get the translated message. Messages can be parameterized, so I recommend making the message contain something like "Total number of results: 1" and using wfMsgWikiHtml( 'messagekey', $revCount ); in the code. Also, building HTML with Html:: functions (Xml:: is its predecessor) is preferred.

+	function getRevCountQuery($dbr) {
+		$sql = 'SELECT COUNT(cr_id) AS rev_count' . 
+				' FROM '  . $dbr->tableName('code_rev') . ' ';
+		// count if code_rev where path matches
+		if ( $this->mPath ) {
+			$sql .= ' WHERE cr_id in' .
+						' (SELECT cp_rev_id from code_paths' .
+						' WHERE' .
+						' cp_repo_id = ' . $this->mRepo->getId(). ' AND' .
+						' cp_path LIKE ' . $dbr->addQuotes( $this->mPath . '%') . ' AND' .
+						// performance
+						' cp_rev_id > ' . ($this->mRepo->getLastStoredRev() - 20000) . 
+						$this->getSpecializedWhereClause($dbr) .
+						')';
+		// No path; count of code_rev
+		} else {
+			$sql .= ' WHERE cr_repo_id = ' . $this->mRepo->getId() .
+					$this->getSpecializedWhereClause($dbr);
+		}
+		return $dbr->query($sql);
+	}
+	
+	function getSpecializedWhereClause($dbr) {
+		return '';
+	}
+	

Please don't use raw SQL in your code, unless you need it for stuff like subqueries. I don't see why you need a subquery here, a join should work just fine. The preferred format for doing queries is by calling $dbr->select() (see the DatabaseBase docs I mentioned above) which accepts a bunch of associative arrays as parameters describing the query, and does most escaping for you. Examples of usage are all over the codebase (both core and extensions). getSpecializedWhereClause() can then simply return something like array( 'cr_status' => $this->mStatus ) which you would array_merge() into the WHERE clauses array.

You've also added trailing whitespace here and there.

The code style and hardcoded message issue should be fixed now.

I could not eliminate the subselect because of the nature of the join, each rev_id has multiple paths :

mysql> SELECT COUNT(cr_id) AS rev_count FROM `code_rev` WHERE cr_id in (SELECT cp_rev_id from code_paths WHERE cp_repo_id = 1 AND cp_path LIKE '/trunk/extensions/UsabilityInitiative%' AND cp_rev_id > -19987); +-----------+ | rev_count | +-----------+ | 1277 | +-----------+ 1 row in set (0.00 sec)

mysql> SELECT COUNT(cr_id) AS rev_count FROM code_rev, code_paths WHERE cr_id = cp_rev_id AND cp_repo_id = 1 AND cp_path LIKE '/trunk/extensions/UsabilityInitiative%' AND cr_id > -19987; +-----------+ | rev_count | +-----------+ | 5333 | +-----------+ 1 row in set (0.58 sec)

Don't mark your own commits as resolved, resetting to new.

+		if ( $this->mPath ) {
+			$tables[] = 'code_paths';

Inappropriate conversion from string to boolean, will convert "0" etc. to false. Similarly:

+		$pathQuery = (empty($this->mView->mPath)) ? array() : array('path' => $this->mView->mPath);

Inappropriate use of empty(), should probably be !strlen($this->mView->mPath) like in getDefaultSort().

Remarks on this revision appear to no longer apply to HEAD; marking as new.