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

Reedy triaged this task as High priority.Dec 3 2016, 11:59 AM

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.

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

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

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

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

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