r111217 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111216‎ | r111217 | r111218 >
Date:23:41, 10 February 2012
Author:werdna
Status:reverted (Comments)
Tags:gerritmigration 
Comment:
AbuseFilter: Resolve bugs 18374, 28633.
* Store the revision ID associated with a log entry if the action is successful.
* Expose this as a diff link in the UI.
* Implicitly hide log entries if their corresponding revisions are also hidden.
* Includes scope for expanding to log entries if desired.
Modified paths:
  • /trunk/extensions/AbuseFilter/AbuseFilter.class.php (modified) (history)
  • /trunk/extensions/AbuseFilter/AbuseFilter.hooks.php (modified) (history)
  • /trunk/extensions/AbuseFilter/AbuseFilter.i18n.php (modified) (history)
  • /trunk/extensions/AbuseFilter/AbuseFilter.parser.php (modified) (history)
  • /trunk/extensions/AbuseFilter/AbuseFilter.php (modified) (history)
  • /trunk/extensions/AbuseFilter/api/ApiQueryAbuseLog.php (modified) (history)
  • /trunk/extensions/AbuseFilter/db_patches/patch-afl_action_id.sql (added) (history)
  • /trunk/extensions/AbuseFilter/special/SpecialAbuseLog.php (modified) (history)

Diff [purge]

Index: trunk/extensions/AbuseFilter/special/SpecialAbuseLog.php
——@@ -252,7 +252,7 @@
253253 return;
254254 }
255255
256 - if ( $row->afl_deleted && !self::canSeeHidden() ) {
 256+ if ( $this->isHidden($row) && !self::canSeeHidden() ) {
257257 $out->addWikiMsg( 'abusefilter-log-details-hidden' );
258258 return;
259259 }
——@@ -375,10 +375,30 @@
376376
377377 $title = Title::makeTitle( $row->afl_namespace, $row->afl_title );
378378
 379+ $diffLink = false;
 380+
 381+ if ( self::isHidden($row) && ! $this->canSeeHidden() ) {
 382+ return '';
 383+ }
 384+
379385 if ( !$row->afl_wiki ) {
380386 $pageLink = $sk->link( $title );
 387+ if ( $row->afl_rev_id ) {
 388+ $diffLink = $sk->link( $title,
 389+ wfMessage('abusefilter-log-diff')->parse(), array(),
 390+ array( 'diff' => 'prev', 'oldid' => $row->afl_rev_id ) );
 391+ }
381392 } else {
382393 $pageLink = WikiMap::makeForeignLink( $row->afl_wiki, $row->afl_title );
 394+
 395+ if ( $row->afl_rev_id ) {
 396+ $diffUrl = WikiMap::getForeignURL( $row->afl_wiki, $row->afl_title );
 397+ $diffUrl = wfAppendQuery( $diffUrl,
 398+ array( 'diff' => 'prev', 'oldid' => $row->afl_rev_id ) );
 399+
 400+ $diffLink = Linker::makeExternalLink( $diffUrl,
 401+ wfMessage('abusefilter-log-diff')->parse() );
 402+ }
383403 }
384404
385405 if ( !$row->afl_wiki ) {
——@@ -433,6 +453,9 @@
434454 $actionLinks[] = $detailsLink;
435455 $actionLinks[] = $examineLink;
436456
 457+ if ($diffLink)
 458+ $actionLinks[] = $diffLink;
 459+
437460 if ( $user->isAllowed( 'abusefilter-hide-log' ) ) {
438461 $hideLink = $sk->link(
439462 $this->getTitle(),
——@@ -485,9 +508,12 @@
486509 );
487510 }
488511
489 - if ( $row->afl_deleted ) {
 512+ if ( $this->isHidden($row) === true ) {
490513 $description .= ' '.
491514 wfMsgExt( 'abusefilter-log-hidden', 'parseinline' );
 515+ } elseif ( $this->isHidden($row) === 'implicit' ) {
 516+ $description .= ' '.
 517+ wfMsgExt( 'abusefilter-log-hidden-implicit', 'parseinline' );
492518 }
493519
494520 return $li ? Xml::tags( 'li', null, $description ) : $description;
——@@ -507,6 +533,25 @@
508534
509535 return $notDeletedCond;
510536 }
 537+
 538+ /**
 539+ * Given a log entry row, decides whether or not it can be viewed by the public.
 540+ *
 541+ * @param $row The abuse_filter_log row object.
 542+ *
 543+ * @return Mixed true if the item is explicitly hidden, false if it is not.
 544+ * The string 'implicit' if it is hidden because the corresponding revision is hidden.
 545+ */
 546+ public static function isHidden( $row ) {
 547+ if ( $row->afl_rev_id ) {
 548+ $revision = Revision::newFromId( $row->afl_rev_id );
 549+ if ( $revision && $revision->getVisibility() != 0 ) {
 550+ return 'implicit';
 551+ }
 552+ }
 553+
 554+ return (bool)$row->afl_deleted;
 555+ }
511556 }
512557
513558 class AbuseLogPager extends ReverseChronologicalPager {
Index: trunk/extensions/AbuseFilter/AbuseFilter.parser.php
——@@ -338,6 +338,39 @@
339339 /** Convert shorteners */
340340
341341 /**
 342+ * @return mixed
 343+ */
 344+ public function toNative() {
 345+ switch( $this->type ) {
 346+ case self::DBool:
 347+ return $this->toBool();
 348+ break;
 349+ case self::DString:
 350+ return $this->toString();
 351+ break;
 352+ case self::DFloat:
 353+ return $this->toFloat();
 354+ break;
 355+ case self::DInt:
 356+ return $this->toInt();
 357+ break;
 358+ case self::DList:
 359+ $input = $this->toList();
 360+ $output = array();
 361+ foreach( $input as $item ) {
 362+ $output[] = $item->toNative();
 363+ }
 364+ return $output;
 365+ break;
 366+ case self::DNull:
 367+ return null;
 368+ break;
 369+ default:
 370+ throw new MWException( "Unknown type" );
 371+ }
 372+ }
 373+
 374+ /**
342375 * @return bool
343376 */
344377 public function toBool() {
Index: trunk/extensions/AbuseFilter/AbuseFilter.php
——@@ -78,6 +78,7 @@
7979 $wgHooks['ContributionsToolLinks'][] = 'AbuseFilterHooks::onContributionsToolLinks';
8080 $wgHooks['UploadVerification'][] = 'AbuseFilterHooks::onUploadVerification';
8181 $wgHooks['MakeGlobalVariablesScript'][] = 'AbuseFilterHooks::onMakeGlobalVariablesScript';
 82+$wgHooks['ArticleSaveComplete'][] = 'AbuseFilterHooks::onArticleSaveComplete';
8283
8384 $wgAvailableRights[] = 'abusefilter-modify';
8485 $wgAvailableRights[] = 'abusefilter-log-detail';
Index: trunk/extensions/AbuseFilter/db_patches/patch-afl_action_id.sql
——@@ -0,0 +1,10 @@
 2+-- Store the ID of successful actions in the abuse_filter_log table.
 3+ALTER TABLE /*_*/abuse_filter_log
 4+ ADD COLUMN afl_rev_id int unsigned;
 5+ALTER TABLE /*_*/abuse_filter_log
 6+ ADD KEY (afl_rev_id);
 7+
 8+ALTER TABLE /*_*/abuse_filter_log
 9+ ADD COLUMN afl_log_id int unsigned;
 10+ALTER TABLE /*_*/abuse_filter_log
 11+ ADD KEY (afl_log_id);
\ No newline at end of file
Property changes on: trunk/extensions/AbuseFilter/db_patches/patch-afl_action_id.sql
___________________________________________________________________
Added: svn:eol-style
112 + native
Index: trunk/extensions/AbuseFilter/AbuseFilter.class.php
——@@ -302,8 +302,12 @@
303303 return $parser->evaluateExpression( $expr );
304304 }
305305
306 - public static function checkConditions( $conds, $vars, $ignoreError = true,
307 - $keepVars = 'resetvars' ) {
 306+ public static function checkConditions(
 307+ $conds,
 308+ $vars,
 309+ $ignoreError = true,
 310+ $keepVars = 'resetvars'
 311+ ) {
308312 global $wgAbuseFilterParserClass;
309313
310314 static $parser;
——@@ -401,8 +405,12 @@
402406
403407 // Check conditions...
404408 $pattern = trim( $row->af_pattern );
405 - if ( self::checkConditions( $pattern, $vars, true /* ignore errors */,
406 - 'keepvars' ) ) {
 409+ if ( self::checkConditions(
 410+ $pattern,
 411+ $vars,
 412+ true /* ignore errors */,
 413+ 'keepvars'
 414+ ) ) {
407415 // Record match.
408416 $result = true;
409417 } else {
——@@ -682,8 +690,10 @@
683691
684692 $filter_matched = self::checkAllFilters( $vars );
685693
 694+ $matched_filters = array_keys( array_filter( $filter_matched ) );
 695+
686696 // Short-cut any remaining code if no filters were hit.
687 - if ( count( array_filter( $filter_matched ) ) == 0 ) {
 697+ if ( count( $matched_filters ) == 0 ) {
688698 wfProfileOut( __METHOD__ );
689699 return true;
690700 }
——@@ -691,7 +701,7 @@
692702 wfProfileIn( __METHOD__ . '-block' );
693703
694704 list( $actions_taken, $error_msg ) = self::executeFilterActions(
695 - array_keys( array_filter( $filter_matched ) ), $title, $vars );
 705+ $matched_filters, $title, $vars );
696706
697707 $action = $vars->getVar( 'ACTION' )->toString();
698708
——@@ -784,7 +794,11 @@
785795 // Increment trigger counter
786796 $wgMemc->incr( self::filterMatchesKey() );
787797
788 - $dbw->insert( 'abuse_filter_log', $log_rows, __METHOD__ );
 798+ $local_log_ids = array();
 799+ foreach( $log_rows as $row ) {
 800+ $dbw->insert( 'abuse_filter_log', $row, __METHOD__ );
 801+ $local_log_ids[] = $dbw->insertId();
 802+ }
789803
790804 if ( count( $logged_local_filters ) ) {
791805 // Update hit-counter.
——@@ -795,6 +809,8 @@
796810 );
797811 }
798812
 813+ $global_log_ids = array();
 814+
799815 // Global stuff
800816 if ( count( $logged_global_filters ) ) {
801817 $vars->computeDBVars();
——@@ -807,7 +823,9 @@
808824 global $wgAbuseFilterCentralDB;
809825 $fdb = wfGetDB( DB_MASTER, array(), $wgAbuseFilterCentralDB );
810826
811 - $fdb->insert( 'abuse_filter_log', $central_log_rows, __METHOD__ );
 827+ foreach( $central_log_rows as $row ) {
 828+ $fdb->insert( 'abuse_filter_log', $row, __METHOD__ );
 829+ }
812830
813831 $fdb->update( 'abuse_filter',
814832 array( 'af_hit_count=af_hit_count+1' ),
——@@ -816,6 +834,9 @@
817835 );
818836 }
819837
 838+ $vars->setVar( 'global_log_ids', $global_log_ids );
 839+ $vars->setVar( 'local_log_ids', $local_log_ids );
 840+
820841 // Check for emergency disabling.
821842 $total = $wgMemc->get( AbuseFilter::filterUsedKey() );
822843 self::checkEmergencyDisable( $logged_local_filters, $total );
Index: trunk/extensions/AbuseFilter/AbuseFilter.i18n.php
——@@ -91,6 +91,7 @@
9292 'abusefilter-log-detailedentry-global' => 'global filter $1',
9393 'abusefilter-log-detailedentry-local' => 'filter $1',
9494 'abusefilter-log-detailslink' => 'details',
 95+ 'abusefilter-log-diff' => 'diff',
9596 'abusefilter-log-hidelink' => 'adjust visibility',
9697 'abusefilter-log-details-legend' => 'Details for log entry $1',
9798 'abusefilter-log-details-var' => 'Variable',
——@@ -103,7 +104,7 @@
104105 'abusefilter-log-linkoncontribs' => 'abuse log',
105106 'abusefilter-log-linkoncontribs-text' => 'Abuse log for this user',
106107 'abusefilter-log-hidden' => '(entry hidden)',
107 - 'abusefilter-log-hide' => 'hide or unhide', // @todo FIXME: Message unused?
 108+ 'abusefilter-log-hidden-implicit' => '(hidden because revision has been deleted)',
108109 'abusefilter-log-cannot-see-details' => 'You do not have permission to see details of this entry.',
109110 'abusefilter-log-details-hidden' => 'You cannot view the details for this entry because it is hidden from public view.',
110111
——@@ -842,6 +843,8 @@
843844 * $2 is new user link or old user link. Link description is a user name',
844845 'abusefilter-diff-info' => "Header for the box containing the basic information about a user account, displayed on the 'user profile' tab of the [[Special:Preferences|user preferences]] special page.",
845846 'abusefilter-import-intro' => "Do not ''translate'' <nowiki>{{int:abusefilter-edit-export}}</nowiki>, <nowiki>{{int:abusefilter-tools-subtitle}}</nowiki>, and <nowiki>{{int:abusefilter-import-submit}}</nowiki> unless you absolute must substitute any of them.",
 847+ 'abusefilter-log-diff' => 'Diff link text to a revision associated with an AbuseFilter log entry',
 848+ 'abusefilter-log-hidden-implicit' => 'Explanatory text to be shown beside an abuse filter log entry if it cannot be viewed due to its corresponding revision being hidden',
846849 );
847850
848851 /** Faeag Rotuma (Faeag Rotuma)
Index: trunk/extensions/AbuseFilter/api/ApiQueryAbuseLog.php
——@@ -73,6 +73,7 @@
7474 $this->addFieldsIf( 'afl_var_dump', $fld_details );
7575 $this->addFieldsIf( 'afl_actions', $fld_result );
7676 $this->addFieldsIf( 'afl_deleted', $fld_hidden );
 77+ $this->addFields( 'afl_rev_id' );
7778
7879 if ( $fld_filter ) {
7980 $this->addTables( 'abuse_filter' );
——@@ -110,6 +111,11 @@
111112 $this->setContinueEnumParameter( 'start', wfTimestamp( TS_ISO_8601, $row->afl_timestamp ) );
112113 break;
113114 }
 115+ if ( SpecialAbuseLog::isHidden($row) &&
 116+ !SpecialAbuseLog::canSeeHidden( $user )
 117+ ) {
 118+ continue;
 119+ }
114120 $entry = array();
115121 if ( $fld_ids ) {
116122 $entry['id'] = intval( $row->afl_id );
——@@ -147,7 +153,10 @@
148154 }
149155
150156 if ( $fld_hidden ) {
151 - $entry['hidden'] = $row->afl_deleted;
 157+ $val = SpecialAbuseLog::isHidden($row);
 158+ if ( $val ) {
 159+ $entry['hidden'] = $val;
 160+ }
152161 }
153162
154163 if ( $entry ) {
Index: trunk/extensions/AbuseFilter/AbuseFilter.hooks.php
——@@ -4,6 +4,7 @@
55 }
66
77 class AbuseFilterHooks {
 8+ static $successful_action_vars = false;
89 // So far, all of the error message out-params for these hooks accept HTML.
910 // Hooray!
1011
——@@ -20,6 +21,8 @@
2122 // Load vars
2223 $vars = new AbuseFilterVariableHolder;
2324
 25+ self::$successful_action_vars = false;
 26+
2427 // Check for null edits.
2528 $oldtext = '';
2629
——@@ -41,9 +44,9 @@
4245
4346 global $wgUser;
4447 $vars->addHolder( AbuseFilter::generateUserVars( $wgUser ) );
45 - $vars->addHolder( AbuseFilter::generateTitleVars( $title , 'ARTICLE' ) );
46 - $vars->setVar( 'ACTION', 'edit' );
47 - $vars->setVar( 'SUMMARY', $summary );
 48+ $vars->addHolder( AbuseFilter::generateTitleVars( $title , 'article' ) );
 49+ $vars->setVar( 'action', 'edit' );
 50+ $vars->setVar( 'summary', $summary );
4851 $vars->setVar( 'minor_edit', $editor->minoredit );
4952
5053 $vars->setVar( 'old_wikitext', $oldtext );
——@@ -59,9 +62,60 @@
6063 $editor->showEditForm();
6164 return false;
6265 }
 66+
 67+ self::$successful_action_vars = $vars;
 68+
6369 return true;
6470 }
6571
 72+ public static function onArticleSaveComplete(
 73+ &$article, &$user, $text, $summary, $minoredit, $watchthis, $sectionanchor,
 74+ &$flags, $revision
 75+ ) {
 76+ if ( ! self::$successful_action_vars ) {
 77+ return true;
 78+ }
 79+
 80+ $vars = self::$successful_action_vars;
 81+
 82+ if ( $vars->getVar('article_prefixedtext')->toString() !==
 83+ $article->getTitle()->getPrefixedText()
 84+ ) {
 85+ return true;
 86+ }
 87+
 88+ if ( $vars->getVar('local_log_ids') ) {
 89+ // Now actually do our storage
 90+ $log_ids = $vars->getVar('local_log_ids')->toNative();
 91+ $dbw = wfGetDB( DB_MASTER );
 92+
 93+ if ( count($log_ids) ) {
 94+ $dbw->update( 'abuse_filter_log',
 95+ array( 'afl_rev_id' => $revision->getId() ),
 96+ array( 'afl_id' => $log_ids ),
 97+ __METHOD__
 98+ );
 99+ }
 100+ }
 101+
 102+ if ( $vars->getVar('global_log_ids') ) {
 103+ $log_ids = $vars->getVar('global_log_ids')->toNative();
 104+
 105+ global $wgAbuseFilterCentralDB;
 106+ $fdb = wfGetDB( DB_MASTER, array(), $wgAbuseFilterCentralDB );
 107+
 108+ if ( count($log_ids) ) {
 109+ $dbw->update( 'abuse_filter_log',
 110+ array( 'afl_rev_id' => $revision->getId() ),
 111+ array( 'afl_id' => $log_ids, 'afl_wiki' => wfWikiId() ),
 112+ __METHOD__
 113+ );
 114+ }
 115+ }
 116+
 117+ return true;
 118+ }
 119+
66120 public static function onGetAutoPromoteGroups( $user, &$promote ) {
67121 global $wgMemc;
68122
——@@ -201,6 +255,7 @@
202256 $updater->addExtensionUpdate( array( 'addField', 'abuse_filter', 'af_deleted', "$dir/db_patches/patch-af_deleted.sql", true ) );
203257 $updater->addExtensionUpdate( array( 'addField', 'abuse_filter', 'af_actions', "$dir/db_patches/patch-af_actions.sql", true ) );
204258 $updater->addExtensionUpdate( array( 'addField', 'abuse_filter', 'af_global', "$dir/db_patches/patch-global_filters.sql", true ) );
 259+ $updater->addExtensionUpdate( array( 'addField', 'abuse_filter_log', 'afl_rev_id', "$dir/db_patches/patch-afl_action_id.sql", true ) );
205260 if ( $updater->getDB()->getType() == 'mysql' ) {
206261 $updater->addExtensionUpdate( array( 'addIndex', 'abuse_filter_log', 'filter_timestamp', "$dir/db_patches/patch-fix-indexes.sql", true ) );
207262 } else {

Sign-offs

UserFlagDate
Nikerabbitinspected19:27, 29 February 2012
Petrbtested15:20, 20 March 2012

Follow-up revisions

RevisionCommit summaryAuthorDate
r113585Documentation...reedy20:32, 11 March 2012
r114396Revert r111217 (unreviewed rev in AbuseFilter) and its dependencies r113585, ...catrope19:41, 21 March 2012

Comments

#Comment by Reedy (talk | contribs)   16:33, 13 February 2012

bug 18374, bug 28633

#Comment by Werdna (talk | contribs)   17:09, 13 February 2012

Laaazy :)

#Comment by Nikerabbit (talk | contribs)   19:27, 29 February 2012

Instead of 

Linker::link( ..., wfMessage( ... )->parse(), ... )

we usually use ->escaped() here.

Does not exactly follow coding style conventions WRT to whitespace and braces.

Looks sane, but I don't deeply understand the functionality so I haven't tested.

Status & tagging log