Various code quality fixes for AbuseFilter suggested by Tim Starling in a private email, including bugfixes, memory safeguards, performance improvements, removal of redundant code, consolidation of similar functionaality.

AbuseFilter.class.php

@@ -387,10 +387,6 @@ class AbuseFilter {
 	public static function checkAllFilters( $vars ) {
 		// Fetch from the database.
 		wfProfileIn( __METHOD__ );
-		
-		// Sampling profiler
-		$profile = rand(0,50);
-		$profile = ($profile == 1) ? true : false;
 
 		$filter_matched = array();
 		
@@ -398,7 +394,7 @@ class AbuseFilter {
 		$res = $dbr->select( 'abuse_filter', '*', array( 'af_enabled' => 1, 'af_deleted' => 0 ) );
 
 		while ( $row = $dbr->fetchObject( $res ) ) {
-			$filter_matched[$row->af_id] = self::checkFilter( $row, $vars, $profile );
+			$filter_matched[$row->af_id] = self::checkFilter( $row, $vars, true );
 		}
 		
 		global $wgAbuseFilterCentralDB, $wgAbuseFilterIsCentral;
@@ -411,7 +407,7 @@ class AbuseFilter {
 			
 			while ( $row = $fdb->fetchObject( $res ) ) {
 				$filter_matched["global-".$row->af_id] =
-					self::checkFilter( $row, $vars, $profile, 'global-' );
+					self::checkFilter( $row, $vars, true, 'global-' );
 			}
 		}
 

AbuseFilter.parser.php

@@ -121,10 +121,10 @@ class AFPData {
 				return new AFPData( self::DInt, intval( count( $orig->data ) ) );
 			}
 			if( $target == self::DString ) {
-				$lines = array();
+				$s = '';
 				foreach( $orig->data as $item )
-					$lines[] = $item->toString();
-				return new AFPData( self::DString, implode( "\n", $lines ) );
+					$s .= $item->toString()."\n";
+				return new AFPData( self::DString, $s );
 			}
 		}
 
@@ -588,7 +588,7 @@ class AbuseFilterParser {
 				if( $this->mCur->type == AFPToken::TOp && $this->mCur->value == ':=' ) {
 					$this->move();
 					$this->doLevelSet( $result );
-					if( $idx == 'new' )
+					if( $idx === 'new' )
 						$list[] = $result;
 					else
 						$list[$idx] = $result;
@@ -1425,20 +1425,16 @@ class AbuseFilterParser {
 	
 	protected function ccnorm( $s ) {
 		static $equivset = null;
+		static $replacementArray = null;
 		
-		if ( is_null( $equivset ) ) {
+		if ( is_null( $equivset ) || is_null( $replacementArray ) ) {
 			global $IP;
 			require( "$IP/extensions/AntiSpoof/equivset.php" );
+			$replacementArray = new ReplacementArray( $equivset );
 		}
 		
-		if (function_exists('fss_prep_replace')) {
-			$fss = fss_prep_replace( $equivset );
-			
-			return fss_exec_replace( $fss, $s );
-		} else {
-			return strtr( $s, $equivset );
-		}
-	}
+		return $replacementArray->replace( $s );
+	}	
 	
 	protected function rmspecials( $s ) {
 		$orig = $s;

AbuseFilterVariableHolder.php

@@ -128,6 +128,10 @@ class AFComputedVariable {
 			
 		wfDebug( "Couldn't find user $username in cache\n" );
 		
+		if ( count( self::$userCache ) > 1000 ) {
+			self::$userCache = array();
+		}
+		
 		if ( IP::isIPAddress( $username ) ) {
 			$u = new User;
 			$u->setName( $username );
@@ -147,6 +151,10 @@ class AFComputedVariable {
 			return self::$articleCache["$namespace:$title"];
 		}
 			
+		if ( count( self::$articleCache ) > 1000 ) {
+			self::$articleCache = array();
+		}
+			
 		wfDebug( "Creating article object for $namespace:$title in cache\n" );
 		
 		$t = Title::makeTitle( $namespace, $title );
@@ -182,7 +190,7 @@ class AFComputedVariable {
 				$text1 = $vars->getVar( $text1Var )->toString();
 				$text2 = $vars->getVar( $text2Var )->toString();
 				$result = wfDiff( $text1, $text2 );
-				$result = trim( str_replace( '\No newline at end of file', '', $result ) );
+				$result = trim( str_replace( "\ No newline at end of file\n", '', $result ) );
 				break;
 			case 'diff-split':
 				$diff = $vars->getVar( $parameters['diff-var'] )->toString();
@@ -190,21 +198,31 @@ class AFComputedVariable {
 				$diff_lines = explode( "\n", $diff );
 				$interest_lines = array();
 				foreach( $diff_lines as $line ) {
-					if (strpos( $line, $line_prefix )===0) {
+					if ( substr( $line, 0, 1 ) === $line_prefix) {
 						$interest_lines[] = substr( $line, strlen($line_prefix) );
 					}
 				}
 				$result = $interest_lines;
 				break;
 			case 'links-from-wikitext':
+				// This should ONLY be used when sharing a parse operation with the edit.
+				global $wgArticle;
+				
 				$article = self::articleFromTitle( $parameters['namespace'],
 													$parameters['title'] );
-				$textVar = $parameters['text-var'];
 				
-				$new_text = $vars->getVar( $textVar )->toString();
-				$editInfo = $article->prepareTextForEdit( $new_text );
-				$links = array_keys( $editInfo->output->getExternalLinks() );
-				$result = $links;
+				if ( $wgArticle && $article->getTitle() === $wgArticle->getTitle() ) {
+					$textVar = $parameters['text-var'];
+					
+					$new_text = $vars->getVar( $textVar )->toString();
+					$editInfo = $article->prepareTextForEdit( $new_text );
+					$links = array_keys( $editInfo->output->getExternalLinks() );
+					$result = $links;
+				} else {
+					// Change to links-from-wikitext-nonedit.
+					$this->mMethod = 'links-from-wikitext-nonedit';
+					$result = $this->compute( $vars );
+				}
 				break;
 			case 'links-from-wikitext-nonedit':
 			case 'links-from-wikitext-or-database':
@@ -226,18 +244,6 @@ class AFComputedVariable {
 				$result = $links;
 				break;
 			case 'link-diff-added':
-				$oldLinkVar = $parameters['oldlink-var'];
-				$newLinkVar = $parameters['newlink-var'];
-				
-				$oldLinks = $vars->getVar( $oldLinkVar )->toString();
-				$newLinks = $vars->getVar( $newLinkVar )->toString();
-				
-				$oldLinks = explode( "\n", $oldLinks );
-				$newLinks = explode( "\n", $newLinks );
-				
-				$added = array_diff( $newLinks, $oldLinks );
-				$result = $added;
-				break;
 			case 'link-diff-removed':
 				$oldLinkVar = $parameters['oldlink-var'];
 				$newLinkVar = $parameters['newlink-var'];
@@ -248,19 +254,33 @@ class AFComputedVariable {
 				$oldLinks = explode( "\n", $oldLinks );
 				$newLinks = explode( "\n", $newLinks );
 				
-				$removed = array_diff( $oldLinks, $newLinks );
-				$result = $removed;
+				if ($this->mMethod == 'link-diff-added') {
+					$result = array_diff( $newLinks, $oldLinks );
+				}
+				if ($this->mMethod == 'link-diff-removed') {
+					$result = array_diff( $oldLinks, $newLinks );
+				}
 				break;
 			case 'parse-wikitext':
-				$article = self::articleFromTitle( $parameters['namespace'], $parameters['title'] );
-				$textVar = $parameters['wikitext-var'];
 				
-				$new_text = $vars->getVar( $textVar )->toString();
-				$editInfo = $article->prepareTextForEdit( $new_text );
-				$newHTML = $editInfo->output->getText();
-				// Kill the PP limit comments. Ideally we'd just remove these by not setting the 
-				// parser option, but then we can't share a parse operation with the edit, which is bad.
-				$result = preg_replace( '/<!--\s*NewPP limit report[^>]*-->\s*$/si', '', $newHTML );
+				// Should ONLY be used when sharing a parse operation with the edit.
+				global $wgArticle;
+				$article = self::articleFromTitle( $parameters['namespace'], $parameters['title'] );
+				
+				if ($wgArticle && $article->getTitle() === $wgArticle->getTitle()) {
+					$textVar = $parameters['wikitext-var'];
+					
+					$new_text = $vars->getVar( $textVar )->toString();
+					$editInfo = $article->prepareTextForEdit( $new_text );
+					$newHTML = $editInfo->output->getText();
+					// Kill the PP limit comments. Ideally we'd just remove these by not setting the 
+					// parser option, but then we can't share a parse operation with the edit, which is bad.
+					$result = preg_replace( '/<!--\s*NewPP limit report[^>]*-->\s*$/si', '', $newHTML );
+				} else {
+					// Change to parse-wikitext-nonedit.
+					$this->mMethod = 'parse-wikitext-nonedit';
+					$result = $this->compute( $vars );
+				}
 				break;
 			case 'parse-wikitext-nonedit':
 				$article = self::articleFromTitle( $parameters['namespace'], $parameters['title'] );
@@ -274,7 +294,7 @@ class AFComputedVariable {
 			case 'strip-html':
 				$htmlVar = $parameters['html-var'];
 				$html = $vars->getVar( $htmlVar )->toString();
-				$result = preg_replace( '/<[^>]+>/', '', $html );
+				$result = StringUtils::delimiterReplace( '<', '>', '', $html );
 				break;
 			case 'load-recent-authors':
 				$cutOff = $parameters['cutoff'];
@@ -375,6 +395,7 @@ class AFComputedVariable {
 				}
 		}
 		
-		return AFPData::newFromPHPVar( $result );
+		return $result instanceof AFPData
+			? $result : AFPData::newFromPHPVar( $result );
 	}
 }

SpecialAbuseFilter.php

@@ -22,9 +22,6 @@ class SpecialAbuseFilter extends SpecialPage {
 
 		$this->loadParameters( $subpage );
 		$wgOut->setPageTitle( wfMsg( 'abusefilter-management' ) );
-		$wgOut->setRobotPolicy( "noindex,nofollow" );
-		$wgOut->setArticleRelated( false );
-		$wgOut->enableClientCache( false );
 
 		// Are we allowed?
 		if ( !$wgUser->isAllowed( 'abusefilter-view' ) ) {
@@ -42,7 +39,15 @@ class SpecialAbuseFilter extends SpecialPage {
 		$this->mHistoryID = null;
 		$pageType = 'home';
 		
-		$params = array_filter( explode( '/', $subpage ) );
+		$params = explode( '/', $subpage );
+		
+		// Filter by removing blanks.
+		foreach( $params as $index => $param ) {
+			if ($param === '') {
+				unset( $params[$index] );
+			}
+		}
+		$params = array_values( $params );
 		
 		if ($subpage == 'tools') {
 			$view = 'AbuseFilterViewTools';

SpecialAbuseLog.php

@@ -99,10 +99,11 @@ class SpecialAbuseLog extends SpecialPage {
 		// Generate conditions list.
 		$conds = array();
 		
-		if ( !empty( $this->mSearchUser ) ) {
+		if ( $this->mSearchUser ) {
 			$conds['afl_user_text'] = $this->mSearchUser;
 		}
-		if ( !empty( $this->mSearchFilter ) ) {
+
+		if ( $this->mSearchFilter ) {
 			$conds['afl_filter'] = $this->mSearchFilter;
 		}
 			
@@ -145,36 +146,28 @@ class SpecialAbuseLog extends SpecialPage {
 		
 		// Diff, if available
 		if ( $vars->getVar( 'action' )->toString() == 'edit' ) {
-			## Stolen from DifferenceEngine.php
-			global $wgStylePath, $wgStyleVersion, $wgOut;
-			$wgOut->addStyle( 'common/diff.css' );
-	
-			// JS is needed to detect old versions of Mozilla to work around an annoyance bug.
-			$wgOut->addScript( "<script type=\"text/javascript\" src=\"$wgStylePath/common/diff.js?$wgStyleVersion\"></script>" );
-			
 			$old_wikitext = $vars->getVar('old_wikitext')->toString();
 			$new_wikitext = $vars->getVar('new_wikitext')->toString();
+
+			$diffEngine = new DifferenceEngine();
 			
-			$old_lines = explode( "\n", $old_wikitext );
-			$new_lines = explode( "\n", $new_wikitext );
+			$diffEngine->showDiffStyle();
+			$formattedDiff = $diffEngine->generateDiffBody( $old_wikitext, $new_wikitext );
 			
-			$diff = new Diff( $old_lines, $new_lines );
-			$formatter = new TableDiffFormatter;
-			
-			static $colDescriptions = "<col class='diff-marker' />
-		<col class='diff-content' />
-		<col class='diff-marker' />
-		<col class='diff-content' />";
-			
-			$formattedDiff = $formatter->format( $diff );
-			$formattedDiff =
-				"<table class='diff'>$colDescriptions<tbody>$formattedDiff</tbody></table>";
-			
-			$output .=
-				Xml::tags( 'h3',
-							null,
-							wfMsgExt( 'abusefilter-log-details-diff', 'parseinline' )
-						);
+            static $colDescriptions = "<col class='diff-marker' />
+        <col class='diff-content' />
+        <col class='diff-marker' />
+        <col class='diff-content' />";
+            
+            $formattedDiff =
+                "<table class='diff'>$colDescriptions<tbody>$formattedDiff</tbody></table>";
+            
+            $output .=
+                Xml::tags( 'h3',
+                            null,
+                            wfMsgExt( 'abusefilter-log-details-diff', 'parseinline' )
+                        );
+        
 			$output .= $formattedDiff;
 		}
 		

Views/AbuseFilterViewEdit.php

@@ -391,7 +391,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		$form = Xml::tags( 'form', 
 			array( 
 				'action' => $this->getTitle( $filter )->getFullURL(), 
-				'method' => 'POST' 
+				'method' => 'post' 
 			), 
 			$form );