Page MenuHomePhabricator

Support if statements with several statements in AbuseFilterCachingParser
Closed, ResolvedPublic

Description

Svensson1 has found a regression in AbuseFilters. He is not able to log into Phabricator, so I'll just link to his report at https://en.wikipedia.org/wiki/Special:Permalink/753082436#If_statements_containing_several_statements_don.27t_work_anymore_in_edit_filters

Event Timeline

Nirmos created this task.Dec 3 2016, 10:34 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 3 2016, 10:34 AM
Reedy triaged this task as High priority.Dec 3 2016, 11:59 AM
Legoktm added subscribers: vvv, ori.Dec 4 2016, 6:31 AM
MusikAnimal updated the task description. (Show Details)Dec 5 2016, 3:01 AM
MusikAnimal added a subscriber: MusikAnimal.

Yet another regression from @ori's and @vvv's new abuse filter parser?

Someone could try reverting the config change that enabled it: https://gerrit.wikimedia.org/r/#/c/321728/

This error does not exist anymore. This has worked in a correct way since 15 December (or maybe just before that day). So the error existed for approximately one month, approximately 15 November to 15 December 2016.

Huji added a subscriber: Huji.Jan 2 2017, 11:33 PM

I just tested the master version using the logic below, and it did get triggered. Should we close this as INVALID now that it works?

v1 := 0; v2 := 0;
if ( 1 == 1 ) then
(
  v1 := 1; v2 := 1;
)
else
(
  v1 := 2; v2 := 2;
)
end;
v1 == 1

Oh, the bad config was reverted per T153217 (in https://gerrit.wikimedia.org/r/#/c/327269/). But the bug still exists in the now-unused code.

matmarex renamed this task from If statements containing several statements don't work anymore in edit filters to If statements containing several statements don't work anymore in edit filters with AbuseFilterCachingParser.Jan 3 2017, 12:24 AM
Huji added a comment.Jan 3 2017, 2:57 AM

Actually, I take it back. @matmarex is correct; if the parser is set to AbuseFilterCachingParser my snippet above will stop working. But this will always work, indicating that it really is multiple commands in the "then" or "else" that manifests the bug:

v1 := 0; v2 := 0;
if ( 1 == 1 ) then
(
  v1 := 1
)
else
(
  v1 := 2
)
end;
v1 == 1
Krinkle renamed this task from If statements containing several statements don't work anymore in edit filters with AbuseFilterCachingParser to Support if statements with several statements in AbuseFilterCachingParser.Aug 17 2017, 9:58 PM
Daimona moved this task from Backlog to Internal bugs on the AbuseFilter board.Apr 24 2018, 4:48 PM
Restricted Application added a subscriber: Daimona. · View Herald TranscriptApr 24 2018, 4:48 PM

Change 529814 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add test for multiple conditions inside conditionals

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

Daimona claimed this task.Aug 12 2019, 4:20 PM

Curiously, I had already fixed this in rEABF39fc7c12afbd1952010e9f0ca5704aedb34b1ea7.

Change 529814 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add test for multiple conditions inside conditionals

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

Daimona closed this task as Resolved.Aug 19 2019, 2:18 PM
Daimona removed a project: Patch-For-Review.