From d8d4750e6a62c83f95c5822c231e760fc58dd4e2 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Thu, 27 Sep 2018 15:58:55 +0200 Subject: [PATCH] Simplify action arrays The current form is awkward. They're all like [ actionname => [ 'action' => actionname, 'parameters' => params ] ] This is greatly confusing since adds a nesting level, and just duplicates the actionname information (also, we actually never retrieve it from the internal array). Instead, change all of them to be [ actionname => params ] which is a lot shorter and clearer (and easier to handle). A similar case is handled in I8134ecc41fbecdbed99faf406e9e3ca91b6123b9 (see PS 8..10). Change-Id: I34c040dbeb3ab01158fb3db22496def6ccaf72d9 --- includes/AbuseFilter.php | 49 +++----- includes/Views/AbuseFilterViewEdit.php | 22 ++-- tests/phpunit/AbuseFilterSaveTest.php | 2 +- tests/phpunit/AbuseFilterTest.php | 150 ++++++------------------- 4 files changed, 58 insertions(+), 165 deletions(-) diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 8ccb6dc69..a657f5bfe 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -2452,12 +2452,12 @@ class AbuseFilter { } // If we've activated the 'tag' option, check the arguments for validity. - if ( !empty( $actions['tag'] ) ) { - if ( count( $actions['tag']['parameters'] ) === 0 ) { + if ( isset( $actions['tag'] ) ) { + if ( count( $actions['tag'] ) === 0 ) { $validationStatus->error( 'tags-create-no-name' ); return $validationStatus; } - foreach ( $actions['tag']['parameters'] as $tag ) { + foreach ( $actions['tag'] as $tag ) { $status = self::isAllowedTag( $tag ); if ( !$status->isGood() ) { @@ -2470,17 +2470,17 @@ class AbuseFilter { } // Warning and disallow message cannot be empty - if ( !empty( $actions['warn'] ) && $actions['warn']['parameters'][0] === '' ) { + if ( isset( $actions['warn'] ) && $actions['warn'][0] === '' ) { $validationStatus->error( 'abusefilter-edit-invalid-warn-message' ); return $validationStatus; - } elseif ( !empty( $actions['disallow'] ) && $actions['disallow']['parameters'][0] === '' ) { + } elseif ( isset( $actions['disallow'] ) && $actions['disallow'][0] === '' ) { $validationStatus->error( 'abusefilter-edit-invalid-disallow-message' ); return $validationStatus; } // If 'throttle' is selected, check its parameters - if ( !empty( $actions['throttle'] ) ) { - $throttleCheck = self::checkThrottleParameters( $actions['throttle']['parameters'] ); + if ( isset( $actions['throttle'] ) ) { + $throttleCheck = self::checkThrottleParameters( $actions['throttle'] ); if ( $throttleCheck !== null ) { $validationStatus->error( $throttleCheck ); return $validationStatus; @@ -2524,10 +2524,7 @@ class AbuseFilter { $restrictions = $page->getConfig()->get( 'AbuseFilterRestrictions' ); if ( count( array_intersect_key( array_filter( $restrictions ), - array_merge( - array_filter( $actions ), - array_filter( $origActions ) - ) + array_merge( $actions, $origActions ) ) ) && !$page->getUser()->isAllowed( 'abusefilter-modify-restricted' ) ) { @@ -2604,10 +2601,10 @@ class AbuseFilter { $actionsRows = []; foreach ( array_filter( $availableActions ) as $action => $_ ) { // Check if it's set - $enabled = isset( $actions[$action] ) && (bool)$actions[$action]; + $enabled = isset( $actions[$action] ); if ( $enabled ) { - $parameters = $actions[$action]['parameters']; + $parameters = $actions[$action]; if ( $action === 'throttle' && $parameters[0] === 'new' ) { // FIXME: Do we really need to keep the filter ID inside throttle parameters? // We'd save space, keep things simpler and avoid this hack. Note: if removing @@ -2631,11 +2628,7 @@ class AbuseFilter { $afh_row[$afh_col] = $newRow[$af_col]; } - $displayActions = []; - foreach ( $actions as $action ) { - $displayActions[$action['action']] = $action['parameters']; - } - $afh_row['afh_actions'] = serialize( $displayActions ); + $afh_row['afh_actions'] = serialize( $actions ); $afh_row['afh_changed_fields'] = implode( ',', $differences ); @@ -2740,10 +2733,8 @@ class AbuseFilter { // They're both unset } elseif ( isset( $actions1[$action] ) && isset( $actions2[$action] ) ) { // They're both set. Double check needed, e.g. per T180194 - if ( array_diff( $actions1[$action]['parameters'], - $actions2[$action]['parameters'] ) || - array_diff( $actions2[$action]['parameters'], - $actions1[$action]['parameters'] ) ) { + if ( array_diff( $actions1[$action], $actions2[$action] ) || + array_diff( $actions2[$action], $actions1[$action] ) ) { // Different parameters $differences[] = 'actions'; } @@ -2782,18 +2773,10 @@ class AbuseFilter { } // Process actions - $actions_raw = unserialize( $row->afh_actions ); - $actions_output = []; - if ( is_array( $actions_raw ) ) { - foreach ( $actions_raw as $action => $parameters ) { - $actions_output[$action] = [ - 'action' => $action, - 'parameters' => $parameters - ]; - } - } + $actionsRaw = unserialize( $row->afh_actions ); + $actionsOutput = is_array( $actionsRaw ) ? $actionsRaw : []; - return [ $af_row, $actions_output ]; + return [ $af_row, $actionsOutput ]; } /** diff --git a/includes/Views/AbuseFilterViewEdit.php b/includes/Views/AbuseFilterViewEdit.php index c6ebaf2e2..995fb4857 100644 --- a/includes/Views/AbuseFilterViewEdit.php +++ b/includes/Views/AbuseFilterViewEdit.php @@ -479,9 +479,7 @@ class AbuseFilterViewEdit extends AbuseFilterView { $output = ''; foreach ( $enabledActions as $action => $_ ) { - Wikimedia\suppressWarnings(); - $params = $actions[$action]['parameters']; - Wikimedia\restoreWarnings(); + $params = $actions[$action] ?? null; $output .= $this->buildConsequenceSelector( $action, $setActions[$action], $params, $row ); } @@ -492,7 +490,7 @@ class AbuseFilterViewEdit extends AbuseFilterView { /** * @param string $action The action to build an editor for * @param bool $set Whether or not the action is activated - * @param array $parameters Action parameters + * @param array|null $parameters Action parameters * @param stdClass $row abuse_filter row object * @return string|\OOUI\FieldLayout */ @@ -1096,7 +1094,6 @@ class AbuseFilterViewEdit extends AbuseFilterView { } // Load the actions - $actions = []; $res = $dbr->select( 'abuse_filter_action', [ 'afa_consequence', 'afa_parameters' ], @@ -1104,12 +1101,10 @@ class AbuseFilterViewEdit extends AbuseFilterView { __METHOD__ ); + $actions = []; foreach ( $res as $actionRow ) { - $thisAction = []; - $thisAction['action'] = $actionRow->afa_consequence; - $thisAction['parameters'] = array_filter( explode( "\n", $actionRow->afa_parameters ) ); - - $actions[$actionRow->afa_consequence] = $thisAction; + $actions[$actionRow->afa_consequence] = + array_filter( explode( "\n", $actionRow->afa_parameters ) ); } return [ $row, $actions ]; @@ -1153,7 +1148,7 @@ class AbuseFilterViewEdit extends AbuseFilterView { $data = FormatJson::decode( $import ); $importRow = $data->row; - $actions = wfObjectToArray( $data->actions ); + $actions = get_object_vars( $data->actions ); $copy = [ 'af_public_comments', @@ -1245,13 +1240,12 @@ class AbuseFilterViewEdit extends AbuseFilterView { } } - $thisAction = [ 'action' => $action, 'parameters' => $parameters ]; - $actions[$action] = $thisAction; + $actions[$action] = $parameters; } } } - $row->af_actions = implode( ',', array_keys( array_filter( $actions ) ) ); + $row->af_actions = implode( ',', array_keys( $actions ) ); self::$mLoadedRow = $row; self::$mLoadedActions = $actions; diff --git a/tests/phpunit/AbuseFilterSaveTest.php b/tests/phpunit/AbuseFilterSaveTest.php index b1176c565..5d7af50bf 100644 --- a/tests/phpunit/AbuseFilterSaveTest.php +++ b/tests/phpunit/AbuseFilterSaveTest.php @@ -246,7 +246,7 @@ class AbuseFilterSaveTest extends MediaWikiTestCase { $filter = self::$mParameters['id']; $viewEdit = self::getViewEdit( $filter ); list( $newRow, $actions ) = $viewEdit->loadRequest( $filter ); - self::$mParameters['rowActions'] = implode( ',', array_keys( array_filter( $actions ) ) ); + self::$mParameters['rowActions'] = implode( ',', array_keys( $actions ) ); // Send data for validation and saving $status = AbuseFilter::saveFilter( $viewEdit, $filter, $newRow, $actions ); diff --git a/tests/phpunit/AbuseFilterTest.php b/tests/phpunit/AbuseFilterTest.php index 74d85ce81..0a02e45a8 100644 --- a/tests/phpunit/AbuseFilterTest.php +++ b/tests/phpunit/AbuseFilterTest.php @@ -521,10 +521,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ @@ -539,10 +536,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'flow' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ @@ -569,10 +563,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ @@ -587,10 +578,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [] @@ -608,10 +596,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ @@ -626,10 +611,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'degroup' => [ - 'action' => 'degroup', - 'parameters' => [] - ] + 'degroup' => [] ] ], [ 'actions' ] @@ -647,10 +629,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ @@ -665,10 +644,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'flow' ], [ - 'blockautopromote' => [ - 'action' => 'blockautopromote', - 'parameters' => [] - ] + 'blockautopromote' => [] ] ], [ @@ -696,10 +672,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ @@ -715,10 +688,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-warning' - ] + 'abusefilter-warning' ] ] ], @@ -738,10 +708,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-warning' - ] + 'abusefilter-warning' ] ] ], @@ -757,10 +724,7 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_group' => 'default' ], [ - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ], [ 'actions' ] @@ -779,10 +743,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-warning' - ] + 'abusefilter-warning' ] ] ], @@ -799,15 +760,9 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-my-best-warning' - ] + 'abusefilter-my-best-warning' ], - 'degroup' => [ - 'action' => 'degroup', - 'parameters' => [] - ] + 'degroup' => [] ] ], [ 'actions' ] @@ -826,10 +781,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-warning' - ] + 'abusefilter-warning' ] ] ], @@ -846,10 +798,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-my-best-warning' - ] + 'abusefilter-my-best-warning' ] ] ], @@ -874,10 +823,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-beautiful-warning' - ] + 'abusefilter-beautiful-warning' ] ] ], @@ -894,10 +840,7 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-my-best-warning' - ] + 'abusefilter-my-best-warning' ] ] ], @@ -966,14 +909,8 @@ class AbuseFilterTest extends MediaWikiTestCase { 'af_enabled' => 1 ], [ - 'degroup' => [ - 'action' => 'degroup', - 'parameters' => [] - ], - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'degroup' => [], + 'disallow' => [] ] ] ], @@ -1014,16 +951,10 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ - 'abusefilter-warning', - '' - ] + 'abusefilter-warning', + '' ], - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] - ] + 'disallow' => [] ] ] ], @@ -1069,23 +1000,14 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'warn' => [ - 'action' => 'warn', - 'parameters' => [ 'abusefilter-warning', '' - ] - ], - 'disallow' => [ - 'action' => 'disallow', - 'parameters' => [] ], + 'disallow' => [], 'block' => [ - 'action' => 'block', - 'parameters' => [ - 'blocktalk', - '8 hours', - 'infinity' - ] + 'blocktalk', + '8 hours', + 'infinity' ] ] ] @@ -1131,19 +1053,13 @@ class AbuseFilterTest extends MediaWikiTestCase { ], [ 'throttle' => [ - 'action' => 'throttle', - 'parameters' => [ - '131', - '3,60', - 'user' - ] + '131', + '3,60', + 'user' ], 'tag' => [ - 'action' => 'tag', - 'parameters' => [ - 'mytag', - 'yourtag' - ] + 'mytag', + 'yourtag' ] ] ]