Disabling one of the $wgAbuseFilterActions only prevents that action from being modifiable via the Special:AbuseFilter interface. Any filters that were setup while that action was enabled remain using that action, with there being no manner in which you can turn the action off, aside from creating a new filter entirely.
If an action is disabled in the config, it should not be performed by the extension.
Change 487791 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/AbuseFilter@master] If an action is disabled or nonexistent, it should not be executed
@Huji Thanks for the patch, it surely makes disabled actions not be executed. However, I see that this task also reports another problem: you can neither remove such actions from a filter, as the checkbox won't be shown.
Since IMHO this isn't an urgent problem, I guess we should take a bit more time and think of a long-term solution to also address the second part. If I'm not mistaken, if a filter has action X enabled, then you remove X from wgAbuseFilterAction and edit that filter again, the action X will be silently removed. If this is true, it could be fine to just add a warning box saying that such action(s) are actually disabled and will be removed upon saving.
@Daimona if we generalize this problem (which I did in the patch and in its commit message), the issue is not just with "disabled" actions, but also with "undefined" actions. Imagine that you expand the set of actions either through changes in AbuseFilter code, or via hooks and through other extensions; afterwards, imagine you remove those actions, or stop using said extensions. This will leave you with some filters that contain actions that do not exist anymore, which means you don't even have access to the appropriate messages, etc. to even show them in a "disabled" fashion on the filter edit form, to allow the users to uncheck the now-nonexistent actions.
In other words, when you say "then you remove X from wgAbuseFilterAction" you are actually thinking "then you disable X from wgAbuseFilterAction" (i.e. change its value from true to false) while it can also be the case that you actually remove an action (i.e. that key no longer exists in the wgAbuseFilterAction array) and we need to account for that too.
At any rate, I am open to a longer term solution, though I don't think we can find one that would work both for disabled and for removed actions. But I think the temporary fix above should not wait for this longer term solution.
Yes, this is correct.
In other words, when you say "then you remove X from wgAbuseFilterAction" you are actually thinking "then you disable X from wgAbuseFilterAction" (i.e. change its value from true to false)
And this is correct as well.
But I think the temporary fix above should not wait for this longer-term solution.
I think we should first understand the cost (time-wise) of a longer term solution, and if it's too high yes - we can merge this quick fix.
First of all, removing actions from wgAbuseFilterActions is only allowed for "external" actions (as you said): the extension.json reads "_merge_strategy": "array_plus",, which is to say that if in LocalSettings you specify $wgAbuseFilterActions = [ 'rangeblock' => true ];, then the value of $wgAbuseFilterActions will be the default one array_plused with [ 'rangeblock' => true ]. Thus, the only way to really remove an action is to manually edit the JSON (and this is a "don't care") or (again, as you said) if some other extension etc. specifies other actions.
However, I believe that we can handle this in two steps: first, check if an action is disabled (= set to false); if so, we have a localized message etc. Second step, check if the key exists in $wgAbuseFilterActions. If it doesn't, then we can use the raw action name in the warning box. Would this approach leave anything out? Having missing messages from previously installed extensions is, unfortunately, a pretty common problem. Needless to say, if the name of an action is only changed, the extension where it comes from should keep the old message as well (and, I mean, we should document it somewhere).
Okay, I like that analysis.
Next question is: how do we display the checkbox and other settings for that disabled action? Let's say the action that used to be enabled and is disabled now is the throttle action. For a filter that used to have this action, we would like to show all the checkboxes, textboxes, etc. that were pertinent (which means, we need to figure out which ones are pertinent), and show them in an "enabled" fashion (i.e. not use the disabled attribute for their HTML tags) so the user can turn them off, but perhaps in a way that makes it apparent that they are actually disabled (in red or something)? Do you have specific ideas on how to determine which HTML elements belong to a disabled action, and how to make them visually distinctive?
And all the while, we still want the action *not* to be taken; agreed?
That's the hard part. Actually, my first thought was to not display the action at all, although I definitely agree that this could confuse people even more.
Unfortunately, especially when it comes to actions defined from the outside, we won't have any information aside from the action name. However, we're lucky, because custom actions can only have a checkbox, as they're constructed here and there's not much room for customization. So we have 2 cases:
There's yet another problem: in case 1, we should show the warning if the key exists in $wgAbuseFilterActions AND the action is enabled for the current filter. However, for 2 the action won't either appear in $wgAbuseFilterActions (e.g. if the other extension has been uninstalled). I think in this case the only possible way to show a message would be to perform a DB query in buildConsequenceEditor to see if there's something which doesn't exist in $wgAbuseFilterActions.
And all the while, we still want the action *not* to be taken; agreed?
Definitely. There's nothing wrong in the patch, I just wanted to understand if we can add something.
Looking at OOUI demos I cannot find a way to make a textbox look distinguished. Using the example of disabling the throttle action (which uses a checkbox, two textboxes, and a Multiselect widget, I would have wanted a way to make them all appear in red, for instance, and that doesn't seem to be something OOUI supports.
The best idea I have is to create a new <fieldset> after the "Actions to take when matched" called "Disabled actions" in which we can show these checkboxes etc. It would have been awesome if we could give the whold fieldset a grey background or something, but I have no idea if this has been tried anywhere else.
There's yet another problem: in case 1, we should show the warning if the key exists in $wgAbuseFilterActions AND the action is enabled for the current filter. However, for 2 the action won't either appear in $wgAbuseFilterActions (e.g. if the other extension has been uninstalled). I think in this case the only possible way to show a message would be to perform a DB query in buildConsequenceEditor to see if there's something which doesn't exist in $wgAbuseFilterActions.
That is exactly what I was trying to elude to. Now that we are on the same page, let me try to create a new patch.
Yes, that's what I meant with "separate form section" :) I think the grey background etc. won't be necessary as long as there's a warning box at the top of the page and the fieldset is clearly distinguishable from the rest of the actions (e.g. if it has a border).
That is exactly what I was trying to elude to. Now that we are on the same page, let me try to create a new patch.
Thanks :) Adding a query is not ideal for performance, but I guess we may cache its result with getWithSetCallback, like we do for instance with global filters.
Removing task assignee due to inactivity, as this open task has been assigned for more than two years (see emails sent to assignee on May26 and Jun17, and T270544). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be very welcome!
(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)
Change #487791 abandoned by Thiemo Kreuz (WMDE):
[mediawiki/extensions/AbuseFilter@master] If an action is disabled or nonexistent, it should not be executed
Reason:
6 years with no further response. T215136 is still open if needed.