This is using core methods, so it can be unit tested. The same isn't
true for load-recent-authors, which performs a custom DB query and whose
test is probably the slowest AbuseFilter test. Simplify it for now,
until the method is moved to MW core.
Change-Id: Ifbdae1a06aabca996eeac151a6d029fd991ad64d
Additionally, avoid building Title objects in LazyVariableComputer, it
just adds a dependency on TitleFactory and creating mocks is more
complicated, but it's pointless because the caller already has a Title
object.
And also stop using Title::getEarliestRevTime(), since the replacement
is easy (we already have a RevisionLookup).
Note for reviewers about renames:
- Code VariableGeneratorDBTest was moved to LazyVariableComputerDBTest,
RCVariableGeneratorTest, and AbuseFilterVariableGeneratorTest
- AbuseFilterVariableGenerator test was moved into a dedicated
directory, methods were changed not to test the var values
Change-Id: I3dff8739a9b79f33321d836449b082c3ce63f277
The checkbox should only appear on Special:AbuseLog, not when deleting
items (checked with $this->hideEntries), AND not when viewing details of
a single entry, which is check with $isListItem.
Change-Id: Id2db07641bf98992b4838e4e7439ac3ee4b1ad8e
Mostly uncaught exceptions, that appeared in places where the previous
code was silently using DWIM-style booleans.
Also a TypeError due to ViewDiff not using filter objects.
Copy the fix from Ic8032592799756521a59ee23c0e76cb03a510b94 to another
place as well.
Bug: T271430
Bug: T271431
Bug: T271432
Bug: T271433
Change-Id: Ica4b82024c57482656cf6bca95bf37641c09cb9a
Mainly constructor and conditions limit, which can be removed from
ConsequencesTest (where it was very slow).
Additionally, inject globals into FilterRunner.
Change-Id: I56ca67de6878dbc2185038faae3eb2b04fb56be9
Additionally:
- Add typehints for stronger typing, and use strict comparison in the
callers
- Use MIN instead of sorting, as the former is optimized by the DBMS;
sorting was also happening on the wrong key, i.e. afh_timestamp, as
opposed to afh_id
Change-Id: I631772fdfeb510b0bc8b582b84bcf2533d7bc097
Code change: in buildVarDumpTable remove special-cased null value. This
was used to avoid passing null to Html::element, but is no longer
necessary, since we now pretty-print the value.
Change-Id: I6180f6c53448d2a8c8c6066f222e9fd9df577554
So everything can be loaded using PSR-4. These classes weren't renamed,
nor the alias for the AbuseFilter class was deprecated, because they
should be refactored first.
Change-Id: Ia328db58eb326968edf5591daac9bacf8c2f75da
So we can use DI in all generators. Some improvements were deliberately
omitted, e.g. injecting more services and relaxing User/Title to
UserIdentity/LinkTarget, and they'll be included in a subsequent commit.
Depends-On: I1f351071ef2b0b7c80e91407a9c3bb17be293044
Depends-On: Ie71740fac35a86f8fe03023080ae8ca08671243d
Depends-On: I589a0e1c2c5891070ab82cd5adfd9cedec19e67d
Change-Id: I92ef0abd5e45b672e6f297a71b3c2c345d56f136
This makes VariableHolder a true value object, and introduces a
stateless service, VariableManager, to operate on it.
Note, in theory, this new service is still cyclically coupled with
LazyVariableComputed. However, it's now two stateless service being
coupled, not two smart/god value objects, so we've still earned
something. For now, the dependency is hidden by using a callback. Some
alternatives for that are mentioned in a code comment.
Bug: T261069
Change-Id: I2f2c84c8e91472ba36084a8bbb4a923f6e04354b
Documentation is already in hooks.txt and in every hook interface, let's
not have to maintain it in a third place.
Change-Id: I8cc5e52b6bc164d9512d22283700966d4c51b943
I think either all or none should consider global filters.
Are there any backwards compatibility concerns?
Change-Id: I22b664e9752588edc195dc4e4f5369392f91ad23
This is an important step towards removing the AbuseFilter class. Note:
proposals for the name of the new service are welcome.
Change-Id: Ib4632173f728b1bdafadef96e01645a833bfceaa
Moves more methods away from the AbuseFilter class. Testing
buildVarDumpTable is not easy because we'd have to parse the generated HTML.
Change-Id: I073a537201de150ba9dd7bf15a99f3a009dc6ba1
Skip a test that fails with
Wikimedia\Rdbms\DBQueryError: Error 5: database is locked
Function: Wikimedia\Rdbms\Database::beginIfImplied (MediaWiki\Extension\AbuseFilter\FilterLookup::getAllActiveFiltersInGroupFromDB)
Probably due to some concurrency issue caused by the duplicate connection, and also with
Wikimedia\Rdbms\DBQueryError: Error 1: no such table: unittest_external_abuse_filter
Function: MediaWiki\Extension\AbuseFilter\FilterLookup::getAllActiveFiltersInGroupFromDB
for unknown reasons.
Move the mwGlobals override inside the test to avoid the same "database is locked" error
on every other test in that class.
Bug: T251967
Change-Id: I552a8d1fa532941f630fd734e590993e7462aeb0
Introduce ReversibleConsequence interface for Consequence classes
whose potentially destructive actions can be reverted using
Special:AbuseFilter/revert. This allows moving reverting logic from
AbuseFilterViewRevert to individual Consequence classes and testing.
Unfortunately, the code is definitely not very clean now.
Change-Id: I558da711f1645ccf64792c6102cf743827171320
See task for a description of the plan. Also note that
AFComputedVariable should be renamed and its properties made private.
This commit includes some adjustments for taint-check in
AbuseFilter::buildVarDumpTable and ::revisionToString.
There's some space for improvement in the new LazyVariableComputer, but
that's left for another commit.
Bug: T261069
Change-Id: Ia44f6e079d39f44cf0122dec5ddb5513ab54f0c6
This requires a MessageLocalizer, which currently means providing the
main RequestContext. This is the only alternative right now, until core
provides a proper MessageLocalizer service (see T247127).
Change-Id: I8c93e2ae7e7bd4fc561c5e8490ed2feb1ef0edc2
Use Echo for delivering the notification to the last
user who edited the filter.
Much boilerplate.
Change-Id: I7a46a03b4f15de20902ec70c62fb4fe750096842
Depends-On: If585b14a6dd6fb8c7d2c3bee1f20d9d08eaac706
This commit introduces some boilerplate for emitting warnings from the
AbuseFilter parser, and also code for showing these warnings in the ace
editor. Adding new warnings should be as simple as appending to
AbuseFilterParser::warnings (and adding the relevant i18n).
Bug: T264768
Bug: T269770
Change-Id: Ic11021b379f997a89f59c8c0572338d957e089a6
This is the last big step towards moving Consequences-related things away from
AbuseFilterRunner. There's still some cleanup to do (+ write proper tests), but
this should really be the last important code change.
Change-Id: I347795fe93ba496c43b1d5cfc9ba6e1326842c06
AbuseFilter emulates the storage mechanism also used for page content.
Instead of duplicating the relevant code, AbuseFilter should use the
same BlobStore service also used by RevisionStore.
Note that this change is not strictly needed to resolve T198341, but is
needed to unblock T183490
Bug: T261889
Bug: T198341
Bug: T183490
Change-Id: I3fc8475dd8d50d73d705b706ff597a130267e990
This is just a temporary location for these two methods. Since they're
used a lot, having them in the AbuseFilter class means that the
dependency graph is unnecessarily complicated. Thus, since these methods
aren't doing much, they were moved to a dedicated class. Future todo is
finding an appropriate location, that might be either as part of another
service, or keep them in a Utilities class, perhaps a single class with
all util methods, rather than a specific class.
Change-Id: I52cc47a6b9a387cd1e68c5127f6598a4c43ca428
The main change is the addition of checkboxes to hide/show multiple
entries at the same time. Also, tweaked some i18n and made the process
return more useful success/error messages.
This patch introduces some technical debt, caused by SpecialAbuseLog and
AbuseLogPager being tightly coupled (which is a pre-existing problem,
but it got worse here).
Bug: T260904
Bug: T144096
Bug: T206945
Bug: T206938
Change-Id: I13f476d8126f81b0417e7509784c83d4f21cf348
Move to the latter some methods that make more sense in there. Inject
some more services, don't require a SpecialAbuseLog to be passed in the
constructor.
There are still a couple of static calls, but fixing those would require
factoring more classes out of SpecialAbuseLog (e.g. a service to
determine visibility of AbuseLog entries).
Change-Id: I1b3012ca85bf049a07e0433fc0b357f502c355ad
This is moving code away from SpecialAbuseLog, which is already too big
and has too many purposes. As such, the behaviour is not changed,
including for now bugs that were already present in the old version.
Change-Id: Idc13f7f746ada2e425662c6948c32aa744edac61
This is achieved by creating a new ParserStatus class. Aside from the
result of parse(), it contains whether the cache was warm. This can be
used to differentiate profiling data as part of T231112.
Another use case is returning non-fatal warnings (T269770).
Change-Id: Ifcbda861ce1a44bbe9bffba5b83cd9ef338a8dba
This is the last use, and it was a bit harder to remove because it was
buried inside AFComputedVariable. Starting with
I4444cada720ab62d187f2dd0c4760697e465f2ff, we can freely change the
parameters to AFComputedVariable without breaking old log entries.
Note, we still need a fallback for other extensions calling this
method...
Bug: T246733
Depends-On: I4444cada720ab62d187f2dd0c4760697e465f2ff
Change-Id: I5d786a518ef88fad9c8d9c25ef4553a0bf30b2b2
The schema was introduced in 1.34, so there should be no issue in
starting off with writing the new columns.
Bug: T220791
Change-Id: I8f956d4a27692a33368a413fbf4a8eb5da20afe1
Add a script to migrate the columns (which can also
be executed in dry run), and a config option with the migration stage
(defaults to SCHEMA_COMPAT_OLD).
Some of the script-related code is stolen from
Ic755526d5f989c4a66b1d37527cda235f61cb437.
Bug: T220791
Change-Id: I7460a2d63f60c2933b36f8383a8abdbba8649e12
There is a try-catch block but the same call was also done
unconditionally after it, making it throw when global filters
are disabled.
Change-Id: Ic8032592799756521a59ee23c0e76cb03a510b94
$wgAbuseFilterActions shouldn't be used normally, as it excludes actions
registered by other extensions.
Note: mw:Extension:AbuseFilter#Integration_with_other_extensions should
be updated after merging.
Bug: T239348
Change-Id: I89b3f0228eacdf145e8f2dd2a5602d0c7ce75a86
This was NULL for old entries, because no default was added
in I758795f01eaf3ff56c5720d660cd989ef95764a7 (see T263324)
Bug: T269314
Change-Id: I5af8b0d3a9d7b6d2570cf79bbbe8b5b170ba1230
Also fix a bug in FilterProfiler. It would attempt to reset
stats for global filters but we do not record them (yet?).
Change-Id: I0228d8c85dab146deb877dfce506f1e8e7711a9f
* Move all SQL files into db_patches (or below)
** Remove db type from filename
* Remove a lot of duplicated code and simplify
Change-Id: If22f2a2c46a59ac24c89ce612c74d169f053ab26
Just moving code around. Without a unit test because DI
coverage of change tags in core isn't available yet.
Change-Id: Iac861e1e24dae13581b8d9173357a1d6c94be88a
It makes sense to look at this and Iedd7a5dca24 together,
as this patch itself doesn't really fix anything.
Change-Id: Ifef5266b1803d1a96489789b08d9beed044d908f
The consequence-taking logic is moved away from AbuseFilterRunner, to
dedicated classes. There's now one class per consequence, encapsulating
everything it needs to take the consequence.
Several interfaces allow customizing different types of consequences.
Every "special check" in AbuseFilter was generalized to use these
interfaces, rather than knowing how to handle each consequence.
Adding more consequences from other extensions will also be easier, and
it should happen via a hook (not a global), returning a class that
implements Consequence. The BCConsequence class was temporarily added
for legacy custom consequences.
A ConsequenceFactory class is added to instantiate consequences; this
would possibly benefit from using ObjectFactory, but it doesn't because
it would also reduce readability (although we might do that in the
future).
These classes are still not covered by unit tests, and this is left to
do for later. The new unit tests should mostly replace
AbuseFilterConsequencesTest. @covers tag were added to keep the status
quo (i.e. code that was considered covered while in AbuseFilterRunner
will still be considered covered), although we'll have to adjust them.
Change-Id: Ia1a9a8bbf55ddd875dfd5bbc55fcd612cff568ef
This will ease adding new watchers, for instance to send Echo
notifications (see T179495 and T100892).
For now, this is just boilerplate, and converting EmergencyWatcher to
the new interface.
Change-Id: I18d62aba53471202b709cdb19033b1729c5c25b4
This will avoid unneeded queries, in theory. In practice, it will
almost never happen to have more than one filter to throttle.
Change-Id: I5b8df51215463ce4464f6a2d0390f58612a5a213
-Exclude methods and classes that cannot be meaningfully covered
-Add a simple test for AbuseFilterServices
-Exclude ServiceWiring because there's no way to tell PHPUnit it's
covered
Change-Id: I4c67b0d3fea68c7a3b3cbe01b5608f87e1b492db
Needs the patch in ContentTranslation first.
Depends-On: I0b74db70ad4e9768e4dcb84b9decb9c737e942e5
Change-Id: Id186ea99fcf69aa4348e404677ce5da998d83170
The slow filter hits are logged for the target wiki, but
the fix would be on meta, so make it possible to filter
for those
Change-Id: I6e02866479e77d707f4fa951ec909c325b944158
The behaviour is:
- When assigning to an undefined offset, delete the whole array and turn
it into another DUNDEFINED
- When retrieving from an undefined offset, just return DUNDEFINED.
Bug: T237214
Change-Id: I621ee7a16c90bb86a57be04e7ce0a748ecdbfcc7
The main benefit of having a dedicated interface is that we can easily
change the output format. So we're now using a custom array without
references to the DB schema, thus making the import/export process
completely independent from the schema.
Change-Id: I4c0de41d914baf1e9a0e588bd31f95b3524a424b
Add a radio to select between "hide" and "show" instead of a single,
cryptic checkbox which doesn't really explain what it does.
Also wrap the list in a form which will later be used to mass-delete
entries.
Depends-On: I1bb45e47c3b42c01388b99778ce833e4e44419e1
Change-Id: Ie2d019fad5af7c626d722dc348f40eb0db21e527
Remove $title->exists() from the check, so we have the following
changes:
- The AbuseLog will add a diff link for page creations
- Searching the AbuseLog for impact:saved will include page creations
- We don't have to recreate the WikiPage again in RunVariableGenerator
Also remove an old reference to "bug 31656": that comment was added in
rEABFefecf8b2441ae2f31f924ff33103f5affe5d1d62, which changed
Article::getContent() to Article::getRevision()->getRawText(). Nowadays
we don't even use Article anymore, and that conditional isn't even for
retrieving the page content, so the comment is wrong.
Add logging for when the Title object cannot exist, as this should never
happen in the context of the EditFilterMergedContent hook, and always
create a WikiPage. Some signatures were changed to require a WikiPage
object now, and every caller updated to provide it.
Bug: T263104
Bug: T62179
Depends-On: Ic238eaa529ef6bfba06b4dd03924a8e0111d8259
Change-Id: Ibf3bf4f68328ba4a5616ab8f26a8b44d27a25cd7
This is a no-op, moving code around, introducing another distinction re
"filtering actions", which now happens in 2 steps:
- The first step only uses "generic" information available by looking
at enabled actions as a "group". This includes keeping only the
longest block, and removing 'disallow' if other blocking actions are
enabled.
- The second step uses information that is only available after having
"partly executed" (named "pre-checked") a consequence. For instance,
we need to pre-check 'throttle' to see if the throttle was hit, and
remove any other actions if not.
Change-Id: I7be5cfaa61e942a06f97ed52f50e9c8c70a120e8
This way we don't have special cases in executeFilterActions, and instead, we execute
all actions in the same place. In turn, this is going to ease the
transition to a new consequences system: next step is refactoring this
code into a service with proper DI etc.
Bug: T204447
Change-Id: I8134ecc41fbecdbed99faf406e9e3ca91b6123b9
