r51014 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r51013‎ \| r51014 \| r51015 >
Date:	13:08, 26 May 2009
Author:	werdna
Status:	ok (Comments)
Tags:
Comment:	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.
Modified paths:	/trunk/extensions/AbuseFilter/AbuseFilter.class.php (modified) (history) /trunk/extensions/AbuseFilter/AbuseFilter.parser.php (modified) (history) /trunk/extensions/AbuseFilter/AbuseFilterVariableHolder.php (modified) (history) /trunk/extensions/AbuseFilter/SpecialAbuseFilter.php (modified) (history) /trunk/extensions/AbuseFilter/SpecialAbuseLog.php (modified) (history) /trunk/extensions/AbuseFilter/Views/AbuseFilterViewEdit.php (modified) (history)

Diff [purge]

Index: trunk/extensions/AbuseFilter/AbuseFilter.parser.php
—	—	@@ -121,10 +121,10 @@
122	122	return new AFPData( self::DInt, intval( count( $orig->data ) ) );
123	123	}
124	124	if( $target == self::DString ) {
125		~~- $lines = array();~~
	125	+ $s = '';
126	126	foreach( $orig->data as $item )
127		~~- $lines[] = $item->toString();~~
128		~~- return new AFPData( self::DString, implode( "\n", $lines ) );~~
	127	+ $s .= $item->toString()."\n";
	128	+ return new AFPData( self::DString, $s );
129	129	}
130	130	}
131	131
—	—	@@ -588,7 +588,7 @@
589	589	if( $this->mCur->type == AFPToken::TOp && $this->mCur->value == ':=' ) {
590	590	$this->move();
591	591	$this->doLevelSet( $result );
592		~~- if( $idx == 'new' )~~
	592	+ if( $idx === 'new' )
593	593	$list[] = $result;
594	594	else
595	595	$list[$idx] = $result;
—	—	@@ -1425,20 +1425,16 @@
1426	1426
1427	1427	protected function ccnorm( $s ) {
1428	1428	static $equivset = null;
	1429	+ static $replacementArray = null;
1429	1430
1430		~~- if ( is_null( $equivset ) ) {~~
	1431	+ if ( is_null( $equivset ) \|\| is_null( $replacementArray ) ) {
1431	1432	global $IP;
1432	1433	require( "$IP/extensions/AntiSpoof/equivset.php" );
	1434	+ $replacementArray = new ReplacementArray( $equivset );
1433	1435	}
1434	1436
1435		~~- if (function_exists('fss_prep_replace')) {~~
1436		~~- $fss = fss_prep_replace( $equivset );~~
1437		-
1438		~~- return fss_exec_replace( $fss, $s );~~
1439		~~- } else {~~
1440		~~- return strtr( $s, $equivset );~~
1441		~~- }~~
1442		~~- }~~
	1437	+ return $replacementArray->replace( $s );
	1438	+ }
1443	1439
1444	1440	protected function rmspecials( $s ) {
1445	1441	$orig = $s;
Index: trunk/extensions/AbuseFilter/SpecialAbuseLog.php
—	—	@@ -99,10 +99,11 @@
100	100	// Generate conditions list.
101	101	$conds = array();
102	102
103		~~- if ( !empty( $this->mSearchUser ) ) {~~
	103	+ if ( $this->mSearchUser ) {
104	104	$conds['afl_user_text'] = $this->mSearchUser;
105	105	}
106		~~- if ( !empty( $this->mSearchFilter ) ) {~~
	106	+
	107	+ if ( $this->mSearchFilter ) {
107	108	$conds['afl_filter'] = $this->mSearchFilter;
108	109	}
109	110
—	—	@@ -145,36 +146,28 @@
146	147
147	148	// Diff, if available
148	149	if ( $vars->getVar( 'action' )->toString() == 'edit' ) {
149		~~- ## Stolen from DifferenceEngine.php~~
150		~~- global $wgStylePath, $wgStyleVersion, $wgOut;~~
151		~~- $wgOut->addStyle( 'common/diff.css' );~~
152		-
153		~~- // JS is needed to detect old versions of Mozilla to work around an annoyance bug.~~
154		~~- $wgOut->addScript( "<script type=\"text/javascript\" src=\"$wgStylePath/common/diff.js?$wgStyleVersion\"></script>" );~~
155		-
156	150	$old_wikitext = $vars->getVar('old_wikitext')->toString();
157	151	$new_wikitext = $vars->getVar('new_wikitext')->toString();
	152	+
	153	+ $diffEngine = new DifferenceEngine();
158	154
159		~~- $old_lines = explode( "\n", $old_wikitext );~~
160		~~- $new_lines = explode( "\n", $new_wikitext );~~
	155	+ $diffEngine->showDiffStyle();
	156	+ $formattedDiff = $diffEngine->generateDiffBody( $old_wikitext, $new_wikitext );
161	157
162		~~- $diff = new Diff( $old_lines, $new_lines );~~
163		~~- $formatter = new TableDiffFormatter;~~
164		-
165		~~- static $colDescriptions = "<col class='diff-marker' />~~
166		~~- <col class='diff-content' />~~
167		~~- <col class='diff-marker' />~~
168		~~- <col class='diff-content' />";~~
169		-
170		~~- $formattedDiff = $formatter->format( $diff );~~
171		~~- $formattedDiff =~~
172		~~- "<table class='diff'>$colDescriptions<tbody>$formattedDiff</tbody></table>";~~
173		-
174		~~- $output .=~~
175		~~- Xml::tags( 'h3',~~
176		~~- null,~~
177		~~- wfMsgExt( 'abusefilter-log-details-diff', 'parseinline' )~~
178		~~- );~~
	158	+ static $colDescriptions = "<col class='diff-marker' />
	159	+ <col class='diff-content' />
	160	+ <col class='diff-marker' />
	161	+ <col class='diff-content' />";
	162	+
	163	+ $formattedDiff =
	164	+ "<table class='diff'>$colDescriptions<tbody>$formattedDiff</tbody></table>";
	165	+
	166	+ $output .=
	167	+ Xml::tags( 'h3',
	168	+ null,
	169	+ wfMsgExt( 'abusefilter-log-details-diff', 'parseinline' )
	170	+ );
	171	+
179	172	$output .= $formattedDiff;
180	173	}
181	174
Index: trunk/extensions/AbuseFilter/SpecialAbuseFilter.php
—	—	@@ -22,9 +22,6 @@
23	23
24	24	$this->loadParameters( $subpage );
25	25	$wgOut->setPageTitle( wfMsg( 'abusefilter-management' ) );
26		~~- $wgOut->setRobotPolicy( "noindex,nofollow" );~~
27		~~- $wgOut->setArticleRelated( false );~~
28		~~- $wgOut->enableClientCache( false );~~
29	26
30	27	// Are we allowed?
31	28	if ( !$wgUser->isAllowed( 'abusefilter-view' ) ) {
—	—	@@ -42,8 +39,16 @@
43	40	$this->mHistoryID = null;
44	41	$pageType = 'home';
45	42
46		~~- $params = array_filter( explode( '/', $subpage ) );~~
	43	+ $params = explode( '/', $subpage );
47	44
	45	+ // Filter by removing blanks.
	46	+ foreach( $params as $index => $param ) {
	47	+ if ($param === '') {
	48	+ unset( $params[$index] );
	49	+ }
	50	+ }
	51	+ $params = array_values( $params );
	52	+
48	53	if ($subpage == 'tools') {
49	54	$view = 'AbuseFilterViewTools';
50	55	$pageType = 'tools';
Index: trunk/extensions/AbuseFilter/Views/AbuseFilterViewEdit.php
—	—	@@ -391,7 +391,7 @@
392	392	$form = Xml::tags( 'form',
393	393	array(
394	394	'action' => $this->getTitle( $filter )->getFullURL(),
395		~~- 'method' => 'POST'~~
	395	+ 'method' => 'post'
396	396	),
397	397	$form );
398	398
Index: trunk/extensions/AbuseFilter/AbuseFilter.class.php
—	—	@@ -387,10 +387,6 @@
388	388	public static function checkAllFilters( $vars ) {
389	389	// Fetch from the database.
390	390	wfProfileIn( __METHOD__ );
391		-
392		~~- // Sampling profiler~~
393		~~- $profile = rand(0,50);~~
394		~~- $profile = ($profile == 1) ? true : false;~~
395	391
396	392	$filter_matched = array();
397	393
—	—	@@ -398,7 +394,7 @@
399	395	$res = $dbr->select( 'abuse_filter', '*', array( 'af_enabled' => 1, 'af_deleted' => 0 ) );
400	396
401	397	while ( $row = $dbr->fetchObject( $res ) ) {
402		~~- $filter_matched[$row->af_id] = self::checkFilter( $row, $vars, $profile );~~
	398	+ $filter_matched[$row->af_id] = self::checkFilter( $row, $vars, true );
403	399	}
404	400
405	401	global $wgAbuseFilterCentralDB, $wgAbuseFilterIsCentral;
—	—	@@ -411,7 +407,7 @@
412	408
413	409	while ( $row = $fdb->fetchObject( $res ) ) {
414	410	$filter_matched["global-".$row->af_id] =
415		~~- self::checkFilter( $row, $vars, $profile, 'global-' );~~
	411	+ self::checkFilter( $row, $vars, true, 'global-' );
416	412	}
417	413	}
418	414
Index: trunk/extensions/AbuseFilter/AbuseFilterVariableHolder.php
—	—	@@ -128,6 +128,10 @@
129	129
130	130	wfDebug( "Couldn't find user $username in cache\n" );
131	131
	132	+ if ( count( self::$userCache ) > 1000 ) {
	133	+ self::$userCache = array();
	134	+ }
	135	+
132	136	if ( IP::isIPAddress( $username ) ) {
133	137	$u = new User;
134	138	$u->setName( $username );
—	—	@@ -147,6 +151,10 @@
148	152	return self::$articleCache["$namespace:$title"];
149	153	}
150	154
	155	+ if ( count( self::$articleCache ) > 1000 ) {
	156	+ self::$articleCache = array();
	157	+ }
	158	+
151	159	wfDebug( "Creating article object for $namespace:$title in cache\n" );
152	160
153	161	$t = Title::makeTitle( $namespace, $title );
—	—	@@ -182,7 +190,7 @@
183	191	$text1 = $vars->getVar( $text1Var )->toString();
184	192	$text2 = $vars->getVar( $text2Var )->toString();
185	193	$result = wfDiff( $text1, $text2 );
186		~~- $result = trim( str_replace( '\No newline at end of file', '', $result ) );~~
	194	+ $result = trim( str_replace( "\ No newline at end of file\n", '', $result ) );
187	195	break;
188	196	case 'diff-split':
189	197	$diff = $vars->getVar( $parameters['diff-var'] )->toString();
—	—	@@ -190,21 +198,31 @@
191	199	$diff_lines = explode( "\n", $diff );
192	200	$interest_lines = array();
193	201	foreach( $diff_lines as $line ) {
194		~~- if (strpos( $line, $line_prefix )===0) {~~
	202	+ if ( substr( $line, 0, 1 ) === $line_prefix) {
195	203	$interest_lines[] = substr( $line, strlen($line_prefix) );
196	204	}
197	205	}
198	206	$result = $interest_lines;
199	207	break;
200	208	case 'links-from-wikitext':
	209	+ // This should ONLY be used when sharing a parse operation with the edit.
	210	+ global $wgArticle;
	211	+
201	212	$article = self::articleFromTitle( $parameters['namespace'],
202	213	$parameters['title'] );
203		~~- $textVar = $parameters['text-var'];~~
204	214
205		~~- $new_text = $vars->getVar( $textVar )->toString();~~
206		~~- $editInfo = $article->prepareTextForEdit( $new_text );~~
207		~~- $links = array_keys( $editInfo->output->getExternalLinks() );~~
208		~~- $result = $links;~~
	215	+ if ( $wgArticle && $article->getTitle() === $wgArticle->getTitle() ) {
	216	+ $textVar = $parameters['text-var'];
	217	+
	218	+ $new_text = $vars->getVar( $textVar )->toString();
	219	+ $editInfo = $article->prepareTextForEdit( $new_text );
	220	+ $links = array_keys( $editInfo->output->getExternalLinks() );
	221	+ $result = $links;
	222	+ } else {
	223	+ // Change to links-from-wikitext-nonedit.
	224	+ $this->mMethod = 'links-from-wikitext-nonedit';
	225	+ $result = $this->compute( $vars );
	226	+ }
209	227	break;
210	228	case 'links-from-wikitext-nonedit':
211	229	case 'links-from-wikitext-or-database':
—	—	@@ -226,18 +244,6 @@
227	245	$result = $links;
228	246	break;
229	247	case 'link-diff-added':
230		~~- $oldLinkVar = $parameters['oldlink-var'];~~
231		~~- $newLinkVar = $parameters['newlink-var'];~~
232		-
233		~~- $oldLinks = $vars->getVar( $oldLinkVar )->toString();~~
234		~~- $newLinks = $vars->getVar( $newLinkVar )->toString();~~
235		-
236		~~- $oldLinks = explode( "\n", $oldLinks );~~
237		~~- $newLinks = explode( "\n", $newLinks );~~
238		-
239		~~- $added = array_diff( $newLinks, $oldLinks );~~
240		~~- $result = $added;~~
241		~~- break;~~
242	248	case 'link-diff-removed':
243	249	$oldLinkVar = $parameters['oldlink-var'];
244	250	$newLinkVar = $parameters['newlink-var'];
—	—	@@ -248,19 +254,33 @@
249	255	$oldLinks = explode( "\n", $oldLinks );
250	256	$newLinks = explode( "\n", $newLinks );
251	257
252		~~- $removed = array_diff( $oldLinks, $newLinks );~~
253		~~- $result = $removed;~~
	258	+ if ($this->mMethod == 'link-diff-added') {
	259	+ $result = array_diff( $newLinks, $oldLinks );
	260	+ }
	261	+ if ($this->mMethod == 'link-diff-removed') {
	262	+ $result = array_diff( $oldLinks, $newLinks );
	263	+ }
254	264	break;
255	265	case 'parse-wikitext':
	266	+
	267	+ // Should ONLY be used when sharing a parse operation with the edit.
	268	+ global $wgArticle;
256	269	$article = self::articleFromTitle( $parameters['namespace'], $parameters['title'] );
257		~~- $textVar = $parameters['wikitext-var'];~~
258	270
259		~~- $new_text = $vars->getVar( $textVar )->toString();~~
260		~~- $editInfo = $article->prepareTextForEdit( $new_text );~~
261		~~- $newHTML = $editInfo->output->getText();~~
262		~~- // Kill the PP limit comments. Ideally we'd just remove these by not setting the~~
263		~~- // parser option, but then we can't share a parse operation with the edit, which is bad.~~
264		~~- $result = preg_replace( '/<!--\sNewPP limit report[^>]-->\s*$/si', '', $newHTML );~~
	271	+ if ($wgArticle && $article->getTitle() === $wgArticle->getTitle()) {
	272	+ $textVar = $parameters['wikitext-var'];
	273	+
	274	+ $new_text = $vars->getVar( $textVar )->toString();
	275	+ $editInfo = $article->prepareTextForEdit( $new_text );
	276	+ $newHTML = $editInfo->output->getText();
	277	+ // Kill the PP limit comments. Ideally we'd just remove these by not setting the
	278	+ // parser option, but then we can't share a parse operation with the edit, which is bad.
	279	+ $result = preg_replace( '/<!--\sNewPP limit report[^>]-->\s*$/si', '', $newHTML );
	280	+ } else {
	281	+ // Change to parse-wikitext-nonedit.
	282	+ $this->mMethod = 'parse-wikitext-nonedit';
	283	+ $result = $this->compute( $vars );
	284	+ }
265	285	break;
266	286	case 'parse-wikitext-nonedit':
267	287	$article = self::articleFromTitle( $parameters['namespace'], $parameters['title'] );
—	—	@@ -274,7 +294,7 @@
275	295	case 'strip-html':
276	296	$htmlVar = $parameters['html-var'];
277	297	$html = $vars->getVar( $htmlVar )->toString();
278		~~- $result = preg_replace( '/<[^>]+>/', '', $html );~~
	298	+ $result = StringUtils::delimiterReplace( '<', '>', '', $html );
279	299	break;
280	300	case 'load-recent-authors':
281	301	$cutOff = $parameters['cutoff'];
—	—	@@ -375,6 +395,7 @@
376	396	}
377	397	}
378	398
379		~~- return AFPData::newFromPHPVar( $result );~~
	399	+ return $result instanceof AFPData
	400	+ ? $result : AFPData::newFromPHPVar( $result );
380	401	}
381	402	}

Follow-up revisions

Revision	Commit summary	Author	Date
r51498	Minor bugfixes to r51014 suggested on codereview by Tim.	werdna	10:32, 5 June 2009

Comments

#Comment by Tim Starling (talk | contribs) 10:28, 5 June 2009

Excellent, thanks for that.

$result = trim( str_replace( "\ No newline at end of file\n", '', $result ) );

Still seems to be wrong. Maybe...

$result = trim( str_replace( "\n\\ No newline at end of file\n", "\n", $result ) );

I think that would more or less cover everything. Presumably that string can't appear as the first line in the diff, but if it could, this should work:

$result = trim( preg_replace( "/^\\\\ No newline at end of file\n/m", '', $result ) );

It's the backslash at the start of the line which marks it as a special sequence from the diff command rather than something the user has typed into the article.

if ( $wgArticle && $article->getTitle() === $wgArticle->getTitle() ) {

You are relying on the title cache here? It looks like it would work more reliably if you compared the contents of the titles instead of the object IDs:

if ( $wgArticle && $article->getTitle()->equals( $wgArticle->getTitle() ) ) {

#Comment by Werdna (talk | contribs) 10:34, 5 June 2009

All fixed in r51498.

#Comment by Liangent (talk | contribs) 18:22, 14 January 2013

What's the reason for not adding newlines to text instead of trimming warnings? See gerrit:43851.

Status & tagging log

16:53, 5 June 2009 Tim Starling (talk | contribs) changed the status of r51014 [removed: new added: ok]