There's no point in repeating the action name, because it's already used
as key. We can then flatten the array and just keep the parameters in
the third nesting level.
Change-Id: I54abcc49322f432cedd361abeedb72e067d3de41
So that sysadmins can further customize the extension. It was also wrong
to use the same variable for many different things.
Note that there's no associated patch in wmf-config because we use the
defaults. However, before merging this patch, please recheck that
AbuseFilterRestrictions and AbuseFilterDisallowGlobalLocalBlocks aren't
used there (https://codesearch.wmflabs.org/operations/?q=AbuseFilterDisallowGlobalLocalBlocks%7CAbuseFilterRestrictions&i=nope&files=&repos=)
Bug: T175221
Change-Id: I7581b3ee6d9d11a6cf1599b8ff874e8c3d54adf4
The logic about action IDs and the persistent buffer is now encapsulated
inside a single service, which is a step towards getting rid of global
state in the AbuseFilter class, and reducing the responsibilities of the
Runner.
An important change made here is that we now require a LinkTarget rather
than a Title. This removes a dependency on the Title class (a monster
object), makes tests simpler, and denies the need to inject a
TitleFactory. This means living without some bits of context (e.g. we're
no longer using makeTitleSafe to ensure a valid title, and we have to
build a "prefixedtext" manually), but this shouldn't be a problem, given
that the titles are only used to create a cache key: invalid titles are
not a problem, and concatenating namespace + title should always be
sufficient.
Bug: T265370
Change-Id: Iff59cd3d889454a482a89c16691bfefcc5ec0a12
Don't create <a> tags without a href. Show a placeholder
message instead of nothing (alternatively, we could create
a new message for each existing one).
Bug: T174000
Change-Id: Id55b90881aacc620ff3c519ad6eedf212f36c4ed
- Depend on a generic IContextSource rather than SpecialAbuseFilter
(lower coupling);
- Inject a LinkRenderer (IContextSource doesn't have a ::getLinkRenderer
method)
- Add a helper method in SpecialAbuseFilter to get the page title, that
can also be used elsewhere (and the name constant can be made private
now)
- Pull down the mFilter property (and rename it to just 'filter') to
classes that actually need it. Some classes didn't need this at all
and the types were different among subclasses
Now the only cause of coupling between the View classes and
SpecialAbuseFilter is the static call in getTitle.
Change-Id: I3df0c3a7621f0cc9a64a16b0a402a15aae2d5d73
This service should act as a mediator between the AF code and the
permission manager, and it should know what are the permissions required
by each action.
Change-Id: Ieb177d9992147b11fa7b8f05929da6c182cc2286
PHP is not strongly typed, so it's not a good idea to use scalars of
different types (here it's an integer vs the string 'new') to represent
different possibilities. This can have bad effects when type juggling
occurs, and it's also harder to figure out what the type of the
parameter can be (because a numeric ID might have been passed as a
string). Using integer vs null avoids all of this, and also allows us to
use nullable typehints.
These changes were partly copied from
If981cb35bf19a8469aa6c43c907e107cf8c65bc2 and should help with the
migration to the Filter value objects.
Change-Id: I8837d46c3c33761fea53f67b530b721dc7bd49b0
This patch removes the dependency of saveFilter on the ContextSource
kitchen sink. It also removes some unneded dependency, and adds
$originalRow/$originalActions as parameter, rather than hacky properties
in $newRow that are easy to forget. The related test can also be greatly
simplified.
This also introduces a behaviour change: checking $newRow instead of the Request allows us
to account for values normalization done in
AbuseFilterViewEdit::loadRequest, and to also work correctly for imports
(and generally speaking, it makes the method suitable for an
AbuseFilterEdit API module, too).
Next step is moving this method to a service. Some signatures,
indenting, name choices etc. are subpar, but this is just because these
methods are temporary anyway.
Bug: T213037
Change-Id: I235b928d7b9c2ef1c46ea0bf3e3ed212500b4161
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
- 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
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
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
Rather than always using 5 days, the length (in days) can be configured by setting
`AbuseFilterBlockAutopromoteDuration` to the desired length.
Bug: T231756
Change-Id: I996e08a9099ab59657fe511ec2934d26edfa5c7b
When using RecentChange::getQueryInfo() it should be used to instance a
RecentChange class
RCDatabaseLogEntry is only useful in context of LogFormatter
This is a breaking change for the hook variable,
but "RC entry" refers more to the RecentChange class than to the
RCDatabaseLogEntry class
Change-Id: I3af1e42594f8235be815ce38e3411c762ae01092
Additional changes:
* Removed phan-taint-check-plugin from extra, now inherited from mediawiki-phan-config.
Change-Id: Ib63be75df4bfdbd2c5b97de5f80dbec715108c01
Using var_export for better visual effect, especially for arrays.
The result from /tools is much clearer and the 'wrong syntax' message is
a bit more explicative than before.
Bug: T190653
Bug: T239972
Change-Id: I79a17305c7f19f7900f896f895e9365bb5f2fd58
- Increase batch size to 500
- Add an option to print progress markers
- Fix some bad logic which caused some JSONified data to be stored in
the text table without checking (and respecting) old_flags. This caused
some errors on the beta cluster.
Additionally, add a return typehint to AbuseFilter::loadVarDump to make
sure that errors are caught asap. Not only there's no apparent way that
loadVarDump can return an array, but most code is already using the
result as a VariableHolder, unconditionally. This is probably another
leftover from the past.
Bug: T213006
Bug: T246539
Change-Id: Iaebd28badb70d27693fa809cad4db956881e3e5e
Complete WikiPage/Article split and deprecate Page interface
Using actual WikiPage/Article contract
Bug: T239975
Change-Id: I343c3ca2e30715656950cab49c6470061c72b9a0
At the moment there's no validation for import data, so it's totally
possible to insert rubbish in the field, and the code will produce other
rubbish. For instance, it's not so uncommon to see lots of PHP notices
on logstash for ViewEdit code trying to access members of the imported
data as if it were an object.
Change-Id: If9d783f0f9242d3d1bc297572471e62f51ee0e40
In T43172 it was told that adding the site name could increase the risk of
attracting more spam, but I don't see how this variable could cause that.
Bug: T240948
Bug: T97933
Change-Id: I1d2aeabaf008ac06798b8d7e4af7d61ae1702776
This code was introduced with Iba59fe8d190dd338ecc8cfd682205bce33c9738b
and is unused since then. The name should highlight that those variables are not
supposed to be "static", i.e. immutable. Examples are: timestamp, spam
blacklist, site name, site language. These are not immutable, but rather
"generic", and they're known even without an ongoing action.
Also add an RC row param and update docs.
Change-Id: I402f04585e9154059fc413e527e39dcb8e6b3d7c
This is another step needed to reduce the size of the gigantic
AbuseFilter and AbuseFilterHooks classes. It also makes many methods
non-static, for more testability.
Note, this layout is still not final. We should somehow merge the
functionality of VariableGenerator and AFComputedVariable, for which
I already have plans.
Change-Id: I366d598b69ad866496b7cb0059e0835c02e54041
RunVariableGenerator is for generating variables based on the current
action;
RowVariableGenerator is for RC entries;
VariableGenerator is the generic one.
This patch only moves the methods to the new classes, to keep the diff
easier to read, and facilitate conflict resolution. These classes will
then be revamped in I366d598b69ad866496b7cb0059e0835c02e54041.
Note that these classes are now namespaced.
One method, AbuseFilter::getEditVars, was renamed to
AbuseFilterVariableGenerator::generateEditVars, because it would
otherwise conflict with an incompatible method in RunVariableGenerator.
Change-Id: Iff412e5492873d4fae55402939a51609e64d55a8
This provides various shortcuts for user, target, comment, etc.,
avoiding direct access to the row, and thus a dependency on the
schema.
Change-Id: I250f94e0ac6cade33441a31ae8a27093a4d937a0
Instead of having a single loadRequest method (which could end up
loading from the DB...), split it in a DB-only method and a request-only
one. Simplify the logic used to show the filter editor. Show the page
without changes or warnings if the user lost editing rights in the
meanwhile. Avoid two static properties, and pass them in when relevant
instead. Bonus: optimize a query to sort by afh_id instead of afh_timestamp to avoid filesort.
This will allow a subsequent patch to clean the $row object in
loadRequest.
Change-Id: Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba
