Page MenuHomePhabricator

Remove a part of tags in abusefilter will not be saved in Chinese Wikipedia
Closed, ResolvedPublic

Description

  1. In this version, abusefilter has 2 tags: "test" and "test2".
  2. Remove "test2" from "Tags to apply" box. There is only one tags "test".
  3. Click "Save filter".
  4. The change is not be saved.

If I make another change at the same time, ex. leave some message in "Notes" box. It can be saved.
There are the same bugs in zh.wikipedia.org and zh.wikipedia.beta.wmflabs.org.

Event Timeline

Seems that the issue is related to the compareVersions function. The function calls array_diff whose result is not symmetrical. Since new tags are in the first argument when calling compareVersions, if new tags are a subset of old ones, it will be treated as there are no changes.

What does this bug have to do with Chinese sites?

matej_suchanek moved this task from Backlog to Internal bugs on the AbuseFilter board.

I came across this bug by chance, but I want to spend a couple of words. Because of this use of array_diff, I also faced some troubles for T32024: the 2 arrays were different but the edit wasn't saved because array_diff didn't return true. I solved this by adding an equality check for the arrays, however limited to block action. It wouldn't be a problem to extend it to every action, and I'll probably do that if it's needed. Actually, I can't see why such simple check wasn't added in the beginning.

EDIT RIGHT BEFORE POSTING: Maybe we could be even safer by leaving array_diff as it is and adding an opposite array_diff (swapped parameters) instead of an equality check. @matej_suchanek do you think it would be fine?

I was thinking about this for a few minutes. I think adding another array_diff should fix the problem (when used with OR). Thanks for looking into this!

Daimona added a project: Patch-For-Review.

Indeed. Much better and safer than equality check. For reference: https://gerrit.wikimedia.org/r/#/c/412892/

I also agree that adding a second array_diff with the parameters swapped is a good idea. @Daimona, I assume you will work on a patch, will you?

@Huji I will implement that in the patch linked above, if it's ok, since it already touched that point. If it's better to file a separate commit, I'll still do it tonight or tomorrow morning.

Let's keep it in a separate patch. I can quickly review and merge that, and then you can rebase 412892

Alright. No need to rebase, BTW, it still has pending changes in local, so I'd just copy that. Filing commit ASAP.

Change 416360 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Duplicate check for array_diff

https://gerrit.wikimedia.org/r/416360

Huji removed a project: Patch-For-Review.

Change 416360 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Duplicate check for array_diff

https://gerrit.wikimedia.org/r/416360