wikimedia/mediawiki-extensions-AbuseFilter
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/AbuseFilter.git synced 2024-11-23 21:53:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

Simplify code dealing with filter ids in FilterStore

Browse source 
Before the information if a filter was new was stored in 2 places:
In the bool $isNew and in the two variables $filter and $newID.
$newID was especially confusing because it was used for both old and
new ids.

Change-Id: I15bdf36c96c8d86a37f305aab2647f7d57bc2bf1
This commit is contained in:
Thiemo Kreuz 2022-04-26 17:39:04 +02:00
parent b1ea4f2d69
commit be3af66876
1 changed files with 33 additions and 37 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

70
includes/FilterStore.php
View file

@ -81,14 +81,14 @@ class FilterStore {
* - Fatal in case of a permission-related error
*
* @param User $user
* @param int|null $filter
* @param int|null $filterId
* @param Filter $newFilter
* @param Filter $originalFilter
* @return Status
*/
public function saveFilter(
User $user,
?int $filter,
?int $filterId,
Filter $newFilter,
Filter $originalFilter
): Status {
@ -105,7 +105,7 @@ class FilterStore {
// Everything went fine, so let's save the filter
$wasGlobal = $originalFilter->isGlobal();
list( $newID, $historyID ) = $this->doSaveFilter( $user, $newFilter, $differences, $filter, $wasGlobal );
[ $newID, $historyID ] = $this->doSaveFilter( $user, $newFilter, $differences, $filterId, $wasGlobal );
return Status::newGood( [ $newID, $historyID ] );
}
@ -115,7 +115,7 @@ class FilterStore {
* @param User $user
* @param Filter $newFilter
* @param array $differences
* @param int|null $filter
* @param int|null $filterId
* @param bool $wasGlobal
* @return int[] first element is new ID, second is history ID
*/
@ -123,7 +123,7 @@ class FilterStore {
User $user,
Filter $newFilter,
array $differences,
?int $filter,
?int $filterId,
bool $wasGlobal
): array {
$dbw = $this->loadBalancer->getConnectionRef( DB_PRIMARY );
@ -134,8 +134,7 @@ class FilterStore {
$newRow['af_user'] = $user->getId();
$newRow['af_user_text'] = $user->getName();
$isNew = $filter === null;
$newID = $filter;
$isNew = $filterId === null;
// Preserve the old throttled status (if any) only if disabling the filter.
// TODO: It might make more sense to check what was actually changed
@ -145,37 +144,34 @@ class FilterStore {
$rowForInsert = array_diff_key( $newRow, [ 'af_id' => true ] );
$dbw->startAtomic( __METHOD__ );
if ( $isNew ) {
if ( $filterId === null ) {
$dbw->insert( 'abuse_filter', $rowForInsert, __METHOD__ );
$newID = $dbw->insertId();
$filterId = $dbw->insertId();
} else {
$dbw->update( 'abuse_filter', $rowForInsert, [ 'af_id' => $newID ], __METHOD__ );
$dbw->update( 'abuse_filter', $rowForInsert, [ 'af_id' => $filterId ], __METHOD__ );
}
'@phan-var int $newID';
$newRow['af_id'] = $newID;
$newRow['af_id'] = $filterId;
$actions = $newFilter->getActions();
$actionsRows = [];
foreach ( $this->consequencesRegistry->getAllEnabledActionNames() as $action ) {
// Check if it's set
$enabled = isset( $actions[$action] );
if ( $enabled ) {
$parameters = $actions[$action];
if ( $action === 'throttle' && $parameters[0] === null ) {
// 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
// it, a maintenance script will be necessary to clean up the table.
$parameters[0] = $newID;
}
$thisRow = [
'afa_filter' => $newID,
'afa_consequence' => $action,
'afa_parameters' => implode( "\n", $parameters )
];
$actionsRows[] = $thisRow;
if ( !isset( $actions[$action] ) ) {
continue;
}
$parameters = $actions[$action];
if ( $action === 'throttle' && $parameters[0] === null ) {
// 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
// it, a maintenance script will be necessary to clean up the table.
$parameters[0] = $filterId;
}
$actionsRows[] = [
'afa_filter' => $filterId,
'afa_consequence' => $action,
'afa_parameters' => implode( "\n", $parameters ),
];
}
// Create a history row
@ -205,7 +201,7 @@ class FilterStore {
$afhRow['afh_flags'] = implode( ',', $flags );
$afhRow['afh_filter'] = $newID;
$afhRow['afh_filter'] = $filterId;
// Do the update
$dbw->insert( 'abuse_filter_history', $afhRow, __METHOD__ );
@ -213,7 +209,7 @@ class FilterStore {
if ( !$isNew ) {
$dbw->delete(
'abuse_filter_action',
[ 'afa_filter' => $filter ],
[ 'afa_filter' => $filterId ],
__METHOD__
);
}
@ -230,10 +226,10 @@ class FilterStore {
$subtype = $isNew ? 'create' : 'modify';
$logEntry = new ManualLogEntry( 'abusefilter', $subtype );
$logEntry->setPerformer( $user );
$logEntry->setTarget( SpecialAbuseFilter::getTitleForSubpage( (string)$newID ) );
$logEntry->setTarget( SpecialAbuseFilter::getTitleForSubpage( (string)$filterId ) );
$logEntry->setParameters( [
'historyId' => $historyID,
'newId' => $newID
'newId' => $filterId
] );
$logid = $logEntry->insert( $dbw );
$logEntry->publish( $logid );
@ -243,11 +239,11 @@ class FilterStore {
$this->tagsManager->purgeTagCache();
}
$this->filterProfiler->resetFilterProfile( $newID );
$this->filterProfiler->resetFilterProfile( $filterId );
if ( $newRow['af_enabled'] ) {
$this->emergencyCache->setNewForFilter( $newID, $newRow['af_group'] );
$this->emergencyCache->setNewForFilter( $filterId, $newRow['af_group'] );
}
return [ $newID, $historyID ];
return [ $filterId, $historyID ];
}
/**