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
Description
Details
Project | Branch | Lines +/- | Subject | |
---|---|---|---|---|
mediawiki/extensions/AbuseFilter | master | +31 -5 | Add test for multiple conditions inside conditionals |
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Open | None | T231112 Rethink slow filters and profiling after enabling CachingParser | |||
Resolved | Nullzero | T231536 CachingParser::checkSyntax should only try to build the AST without evaluating it | |||
Resolved | Daimona | T234427 Improve CachingParser performance (2019) | |||
Resolved | Krinkle | T156095 Re-enable AbuseFilterCachingParser once we are sure it's safe | |||
Resolved | Daimona | T152281 Support if statements with several statements in AbuseFilterCachingParser |
Event Timeline
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.
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
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
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