Previously, the cached value would depend on the tags
parameter to be updated. The provided value may be
different for each call, so callers may receive
unexpected values.
For example, while core usually calls this with core-defined
hooks, our method AbuseFilter::isAllowedTag calls this
providing an empty array. If core's call happened shortly
after ours and hit cache, its array would be overwritten
with only AbuseFilter's tags, the rest would be lost.
Also do some clean up:
- only call array_filter on explode'd array
- call array_unique on the value, since it's usual that
multiple filters share the same tag
Noticed when thinking about moving this to a service.
Change-Id: I4f4322e80ec89e48458a3bf46a1146863bec8237
af_actions and af_hidden are treated in the same way, so avoid
duplicating that code. Some of the remaining cases are also quite
similar (although not identical), so we might want to merge them in the
future.
Change-Id: I1b48502e077e58eb9ff459326bba18bb1d127242
The only exception is mwexamples-comparisons.t which intentionally
includes examples with = and == to test the "weak" version of the
comparison operator.
Bug: T262063
Change-Id: I6f92aadc69489da481a606bfda89617b8efbb261
Copying my investigation from I8c93e2ae7e7bd4fc561c5e8490ed2feb1ef0edc2:
This code was introduced in 2009, see rEABF0f1eb8db78bfa83ddb93427f39aad619523d8f25:
$display = wfMsg( "abusefilter-action-$action" );
$display = wfEmptyMsg( "abusefilter-action-$action", $display ) ? $action : $display;
And wfEmptyMsg looked like this:
function wfEmptyMsg( $msg, $wfMsgOut ) {
return $wfMsgOut === htmlspecialchars( "<$msg>" );
}
so this made sense. But then, in 2010 (rMWae3ced88e535c7fd046f0ad6f0710cc87f0004ea) the function was changed:
function wfEmptyMsg( $key ) {
global $wgMessageCache;
return $wgMessageCache->get( $key ) === false;
}
without anyone removing the parameter from AbuseFilter.
Finally, in 2012 (rEABF176227e721c9475de2c2163d3b6e20ca4769c406) the usage of wfEmptyMsg was removed, and $display became a parameter to wfMessage().
Long story short, no need to pass that parameter.
Change-Id: Iad875f0c0ab5aaa06c795232638f52e9ca62786e
Ideally, this might live in MediaWikiIntegrationTestCase. For the
createaccount one, AuthManager should also provide a method to log the
creation, because currently we are forced to copypaste that code here.
- Add the missing tests for 'upload' in RCVariableGenerator, and adjust
the existing ones (delete file afterwards, more tablesUsed, use the
right extension).
- Exclude from the coverage report a couple of lines which should
theoretically be unreachable. Escalate logging to WARN level, where it's
more likely to be spotted.
- Remove an unused method (RCVariableGenerator::newFromID). This denies
the need to maintain and cover it. We also don't want this generator
to act as a factory.
Overall, this change brings the coverage for RCVariableGenerator to 100%
Bug: T201193
Change-Id: I425c3d9f6800f74eb6e4eda483b90cfb3bbbcb51
This was also long overdue. Also fix a bug that caused page creations to
not be shown when examining past edits (using rc_last_oldid doesn't work
for page creations).
Bug: T201193
Bug: T262903
Change-Id: I5f7a994add12332c950904146248c5de7c2beee5
- Make a separate method which determines the view
to be shown from subpage syntax and test it.
- Reduce circular dependency between SpecialAbuseFilter
and AbuseFilterView. Use params to transfer information
to views.
Change-Id: Ib9442ea5f9990a5c48f9b9e04055aa22bf7e456e
This is I4df27f3d02432c201c04d9fa118f0129b0a79778 striking again. Fool
me once, shame on thee, fool me twice...
Change-Id: Icea025a2c81e3b413b7bd9ece52866aeaf42937d
- Include an attempt to restore the dump in case the text table
contains a truncated dump (not 100% sure that this can really
happen, nor do I know the cause, but it shouldn't hurt)
- Remove a check for 'action'. The variable might be missing in case of
a corrupted dump. Having an array at that point can only mean "new
format".
- Don't assume that old_wikitext and new_wikitext are set when showing
past filter hits (again, might be unset due to data corruption).
Bug: T264513
Change-Id: I7510d28fc3f43f985a1283e23b413f07adfe7921
This is a simple change but with tons of benefits:
- Easier to track usages for IDEs
- Easier to understand in static analysis (phan)
- Can be analyzed by phpda
- Ensures no typos
- These classes can be namespaced without affecting readability here
Change-Id: Ic04d19dfbe9184baf2ef4bac53011521e2e44953
- Use null instead of empty strings
- Check the mode, and not the pattern, to decide whether the user
searched for something
- The call to parent::__construct can now be moved up
- Note in a comment how this code is problematic due to "smart"
constructors
- Avoid caching the headers, as that's not going to work anymore.
Change-Id: I420ab0215d53354a67d9d130ebd8d85dfbd2778b
These have been saved in the parent class for quite some time.
Refactor accessors in method overrides.
Change-Id: I9819caa5ab87ac3a8e47efb32b00d89c3e2a61af
It's possible that we try clicking this button before it's ready. This
theory is hard to verify because I get no problem locally, but this
shouldn't hurt.
Also specialize input selectors for a couple elements that might not be
univocally determined within the page.
Change-Id: Ida65c3c5fd4d8b3b35ecbee7e99977c71c7c4b96
These scripts were already included in the updater (and hence executed)
several MW versions ago. There's no need to write tests for them right
now, so exclude these from coverage.
Change-Id: I43e46f06b98bb3b9b9d61a45baaf232e2a99c308
The current code was more of a subpar, temporary solution. However, we
need a stable solution in case more variables will be deprecated in the
future (T213006 fixes the problem for the past deprecation round). So,
instead of setting a hacky property, directly translate all variables
when loading the var dump. This is not only stable, but has a couple
micro-performance advantages:
- Calling getDeprecatedVariables happens only once when loading the
dump, and not every time a variable is accessed
- No checks are needed when retrieving a variable,
because names can always assumed to be new
Some simple benchmarks reveals a runtime reduction of 8-15% compared to
the old code (8% when it had varsVersion = 2, 15% for varsVersion = 1),
which comes at no cost together with increased readability and
stability. It ain't much, but it's honest work.
Change-Id: Ib32a92c4ad939790633aa63eb3ef8d4629488bea
The editing view is currently full of tech debt, brittle and surprising
code and whatnot. It's basically a miracle if it works without problem,
and it'd be an even bigger miracle if you could change something there
without breaking anything.
For these reasons, and because that class must be refactored as part of
the upcoming overhaul, this patch adds a bunch of selenium tests to test
the main functionality of that page.
In particular, these tests cover all possible cases (each corresponding
to a data source) for which buildFilterEditor can be called, which FTR are:
1 - View the result of importing a filter
2 - Create a new filter
3 - Load the current version of an existing filter
4 - Load an old version of an existing filter
5 - Show the user input again if saving fails after one of the steps
above
Having automated tests to cover these cases means that we don't have to
manually test all the scenarios manually each time the class is touched.
Bug: T201193
Change-Id: I408e0a132905416effe0d6d6dc0921991edd66bd
When the user selects to see global rules and it's a remote wiki, hide the rule search field. (Note that the list of search modes needs to listen to this setting as well.)
This was discussed during reviewing I0771fa048.
Also move local/global filters setting to the top as it's more important than that for disabled and deleted filters (which will both stay together).
Change-Id: I0912aa1f5d7a5d75e6ae5a2a3362b8d38260c611
This will decouple a bit the huge and chaotic tangle of AF classes. Some
boilerplate code for AbuseFilter services is also added with this patch.
Note that this requires injecting a KeywordsManager in
AbuseFilterVariableHolder, or unit tests would fail. This is still
incomplete, and the Manager is only injected in tests, because
VariableHolder still has to be refactored.
The test for the UpdateVarDumps script had to be updated, because
serializing VHs in there was a bad choice. As pointed out in a comment,
the test is likely going to break again once we remove the BC code, but
I hope that we'll be able to remove the test at that point.
Change-Id: I12a656a310adb8c5f75cab63f6db9e121e109717
These methods had no reals reason to be static and belong to the
AbuseFilter class. Most of them were moved to Parser class as common
variations of the existing entry points. One was specific to the
EvalExpression API module and was moved there.
This change comes at no cost, and will make it possible to inject a
parser where needed.
Change-Id: Ifd169cfc99df8a5eb4ca94ac330f301ca28a2442
