In particular, this brings stronger typing for getID(), and we can get
rid of many phan suppressions.
Change-Id: Icbf3a6f7db8105082646ec227f62c09449fb165d
With explicit calls it's easier to see what method is being used,
whether it's deprecated, etc. Some methods here are in fact deprecated
or already have a proper replacement, but this is left for a follow-up.
Change-Id: Iee3154855f86c76aab98e7c14250c14e8b9ee939
- Exclude a couple of classes from coverage reports
- Add tests for all handlers
- Add tests for the runner, copied from core
- Make AbuseFilterRunner a real service
Change-Id: I7a0fe3cd8300faef5ef72d7f986b1734c324d8d1
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
Mainly constructor and conditions limit, which can be removed from
ConsequencesTest (where it was very slow).
Additionally, inject globals into FilterRunner.
Change-Id: I56ca67de6878dbc2185038faae3eb2b04fb56be9
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
This is an important step towards removing the AbuseFilter class. Note:
proposals for the name of the new service are welcome.
Change-Id: Ib4632173f728b1bdafadef96e01645a833bfceaa
Move to 'integration' all tests that are meant to stay there. Move
SaveTest outside because, while we might want to finalize it as an
integration test, some parts can still be moved to a unit test.
Change-Id: Id4b6deaac6875fdd85eebbebf0c5fb952d1fbb06
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
Add namespaces, shorten class names.
Non-unit tests and AbuseFilterTest are untouched because those should be
refactored first.
Change-Id: Ie46ef18d6ba1017e25c76b1762f678e5452264d9
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
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
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
$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
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
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
-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
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
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
The scope is still quite limited, but as noted in a todo, we might want
to make this completely independent from the database, and add the use
case of ViewDiff.
Change-Id: Ie980fff0983b3e86037265e85da04444c809a6e8
This moves a lot of things away from the AbuseFilter class. There's a
nasty static dependency on ChangeTags, but it's very limited anyway, and
it's going to be fixed once T245964 is resolved.
Change-Id: Ia7df4b4d3289c2722323f59ceecf3fdd38277785
Some pieces of code were updated to use Filter objects, while other
places are still to be updated. We also need to change the history part
to exclude actions somehow, cleanup the ViewEdit, reduce direct DB
access or anything mentioning DB fields outside of FilterLookup, etc.
Change-Id: I42b7ded685db76eddd45e4b1336f9828cba811ce
This requires adjusting some methods to work with Filter objects. Some
methods and tests are left in an inconsistent/suboptimal state, plus some todos
were added, but all of this is going to be remediated in another commit.
Change-Id: Id063ee73d97c7aef56323e1457d99704f77ab943
This is just a start; next step is adding a factory/store method to
get/store these objects. And then use these value objects whenever
applicable.
Note: the actions-related code is still not fully implemented. This is
going to happen as part of the FilterLookup.
Change-Id: I5f33227887c035e301313bbe24d1c1fefb75bc6a
This is a thin wrapper around LBFactory and the global variable, that
can be injected in classes requiring it (no real class right now, but
that's going to change soon).
Also, remove some DWIM-style returns which made the code harder to
understand.
Change-Id: I1d28ad4a67f914103f3a17cda5f61b28070c7f1c
Make FilterProfiler::getFilterProfile return stats unchanged,
in a structured way. Move computations to AbuseFilterViewEdit,
as they are only useful there. Don't return false on cache
misses, return arrays with zero values instead.
Bug: T266531
Change-Id: I8718cc31a5004340bf742315c7075e10a61fcbfd
This deals with data inconsistencies in buildFilterEditor. Every
property of $row was tested in all 5 scenarios (also using Selenium) to
check when it's set. The result is in the normalizeRow method, which
aims to remove any inconsistencies, so that buildFilterEditor always
receives a "complete" row with all defaults set.
The code in buildFilterEditor is now cleaner (because there are no
isset() checks), and it gives us a unique place where we can set
defaults (rather than partly doing that in
loadRequest/loadFilterData/loadImport, and partly relying on isset).
This will be especially useful when introducing value objects to
represent filters, because now you just have to look at normalizeRow()
to tell which properties are allowed to be missing, and thus what "kind"
of filter object you need (see
I5f33227887c035e301313bbe24d1c1fefb75bc6a).
Additionally, reduce the properties that get passed around during
export/import, and make the selenium test try a roundtrip, rather than
relying on hardcoded data that may get outdated. A future patch will
refactor the import/export code.
Change-Id: Id52c466baaf6da18e2981f27a81ffdad3a509e78
Unfortunately, this isn't using DI completely, because of the
User::newSystemUser call. I'm not even sure if we really need to call it
or we can just stick to new UserIdentityValue, but leaving like this for
now.
Also, the types were weakened to UserIdentity, so the transition is
going to be easy anyway.
Change-Id: I08f8fae0fcc622ff0ac3f86771476d06d1c18549
This commit removes several tests from AbuseFilterConsequences, thus
speeding it up a lot (especially because these tests were very slow,
with each test *case* taking up to 30s in the coverage job).
Everything is now covered by the new AbuseFilterFilterProfilerTest
which, although not being a pure unit test, is much much faster than
*Consequences.
Change-Id: Ic6b16d23ec99abee287f36093b8573505f9c613a
This service is responsible for the blockautopromote feature:
(un)block autopromotion and check status.
The patch mostly moves code from static methods to the new class
and relaxes type hints (e.g. from User to UserIdentity).
Change-Id: I79a72377881cf06717931cd09af12f3b8e5f3e3f
- Add a helper method to output an unrecoverable error, comprising a
button to go back to the filters list;
- Move the token check to attemptSave, so to make the conditionals
easier to read, and group errors together
- Make buildFilterEditor take an HTML parameter for the error, so the
caller can specify whether it's error or warning
- Move the check for non-existing filters out of buildFilterEditor
- Add a bunch of typehints
- Don't set af_throttled and af_hit_count in the empty row template, but
set af_deleted (these are only used in buildFilterEditor)
- Make AbuseFilter::translateFromHistory consistently include the af_global
property (previously it would only be set for global filters; this error
was introduced when first implementing global filters)
- The only user-facing change is that, when trying to use a custom
warning/disallow message on a global filter, this is now considered a
non-fatal error, so we now show the editing interface (and not just an
unrecoverable error).
The next step is resolving the @todo in buildFilterEditor about null
checks.
Change-Id: I9d217dcac3f4cc0b26e53eca735cc327d5efc76d
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
In particular, the interface shouldn't generate links to
"Special:AbuseFilter/history/0" (AbuseFilterHistoryPager::getTitle,
can be seen when visiting "Special:AbuseFilter/history").
Change-Id: Id3dc1bb4fc3c5e853603bf0ec04a6b1751f7d862
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
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
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
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
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
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
This adds some coverage for the *VariableGenerator classes. It's still
not perfect, but something to start with in sight of future
refactorings.
Bug: T201193
Change-Id: Iafa85fb8623ea278ce6e42118df72751806382c2
Also fix the test for _first_contributor vars to cover all variables,
use builtin methods to compute the var (rather than calling
setLazyLoadVar), and to be an integration test.
Change-Id: I2594439acc786e31bce1cd4373d3cbf434204eda
While checking a filter, if a variable is not set (e.g. added_lines for
an account creation), the VariableHolder will return a DNULL, rather
than a DUNDEFINED. This means that some filters will resume working, and
the WMF servers will stop getting AF warnings at a rate of 4 millions per
day. This also requires adjusting some tests to reflect the new
behaviour (which is actually the OLD behaviour, that filters had until
last year when we introduced the DUNDEFINED data type). It also requires
adjusting a check in the old parser, but that's not really relevant
because the plan is to remove the old parser before 1.36 is released
(see I0e75f334c7e0dfc1239f2e5f5f7d7452b0bbf29e).
Bug: T230256
Change-Id: I4d06303047397674c1edbfc32628f1bc83ac3340
This reverts commit be3fbdf347.
Reason for revert: The commit mentioned here was merged a few months ago, so this temporary workaround can now be reverted.
Change-Id: I1e8b4a23b700a2b566c087b8a3ea2229c95bcc3f
* Update ESLint config with Selenium WebdriverIO test suite
* Update modules and Selenium pageobjects and specs per ESLint
requirements
* Update grunt-eslint package to 23.0.0 as required by
eslint-config-wikimedia 0.16.0
Bug: T254495
Change-Id: Ibfcf9115adedf9f2c3e7dac1ac626b41fc97b7c4
Temporarily mark the test skipped on earlier versions of core, to avoid
a circular dependency which blocks the merge.
Change-Id: I5db9937f249edaf7c070b2436c0caea369dac8ef
The problem is explained at T250570#6068702; basically, the previous
check didn't account for DUNDEFINED nested deep inside arrays.
Bug: T250570
Change-Id: Iacee2db54ca00108de6339bb3dae70af7e2eeb56
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
Don't need to worry about supporting prior versions, since AbuseFilter
requires 1.35+
Bug: T247869
Change-Id: I112e929dcdd9edcf3ca433b75356659a3179bbcd
This script aims to fix every problem reported in T213006. Subsequent
patches will add new code and drop the back-compat one.
Bug: T213006
Bug: T187153
Bug: T204236
Bug: T187731
Bug: T204235
Bug: T214193
Bug: T214196
Bug: T34478
Depends-On: I5b29ff556eca45fe59d15e2e3df4d06f1f6b3934
Change-Id: I22cf698c5be77506727cbd227c67e037a5d89b5c
This was used to dynamically generate *_restriction_* variables.
However, it had two big problems:
- We only have i18n for 'create', 'move', 'edit', and 'upload' (the
default value of the global); other restrictions would show missing
messages in various pages.
- We had to access the global state in various points.
This change also makes some code in AbuseFilterVariableHolder simpler,
and also allows us to make AbuseFilterTest a unit test.
Change-Id: I321ad6e07f8243200af67a581b6e485970efd3ce
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
Follow-up Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba.
This patch:
- Moves some save-related code to a separate method
- Reduces conditionals nesting
- Fixes an edge case where the content of the form would be
wiped in case the token didn't match.
- Adds another (basic) selenium test
- Standardizes return types
- Moves data load outside of buildFilterEditor
Change-Id: I89444b59f04c495c9ab59244151c8ed5d38cf0fe
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
Also fix a couple of broken tests in Consequences:
- For createaccount, $user->addToDatabase must be called before
testForAccountCreation, or it will throw a CannotCreateActorException.
- In testThrottleLimit, also set wgAbuseFilterEmergencyDisableThreshold
to avoid relying on the local config.
Bug: T201193
Change-Id: If1a50b0a729e4d554485f2e2225d5877510966b6
Most of them are overwritten either in ViewEdit::loadRequest or
AbuseFilter::saveFilter. af_hit_count and af_throttled are actually
relevant for the old version, so list them explicitly. And also add
default af_group and af_global, which are later read, for import action.
Depends-On: Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba
Change-Id: Ie9aae938cca06e38a7a834a3f74f3e8735ab01ee
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
-new_html: also strip the "Transclusion limit" comment if present, and
anyway take it into account (as well as a "</div>"), which right now
prevent the PP limit report from being stripped as well.
-new_text: trim extra whitespace on the right, which is created when
stripping the aforementioned comments.
Also simplify the test for getEditVars, make it not blindly copy what
AFComputedVariable does.
Extra: kill a temporary variable.
These changes are partly taken from
I96785c6c5fdf381c21d5f8930ee12e706abb7f3f.
Change-Id: I2b4c84a3d9d0d17ce229088197b75781d5181b4f
Even if the array is DUNDEFINED, we need to check the offset to ensure
that it's valid.
Bug: T237351
Change-Id: Ibfa360c4ae1d80abe14d9fdf66991b76cb5954df
For the new parser, xhgui shows that AbuseFilterParser::getVarValue is
taking up a lot of time; in turn, most of the time spent inside
getVarValue is used to log the use of deprecated variables. Hence, given
that:
- We should keep the new parser performant
- There are tons of deprecated variables out there and they likely
won't be replaced
- Having gazillions of debugLog entries doesn't help
log them only in the cached phase.
Bug: T234427
Change-Id: I2bfc692c829c3cbe889e5076f5205e2c99097087
This is identical to I8a3c31e7385283d95b4712d457784016239a0b3b, except
for the array append case.
Bug: T236870
Change-Id: Iac033ba467232f6ff110d575920e968759ce0e15
This is especially useful for old patches, created before the
introduction of FUNC_ARG_COUNT, where a rebase may break the parser.
Change-Id: Ib142438626a7305f102dc3e4cc9cb07ad33902b8
This will allow people to switch their filters to the new syntax. The
deprecation warning is now more exhaustive, and the info() warning is
kept to ensure that everything proceeds smoothly.
The regex v2 has also been fixed to:
- Consume all the digits/letters on the right (*)
- Have named groups
- Be created dynamically with other constants
(*) The previous version of v2 could complete the match and leave
digits/letters on the right when encountering numbers with the old
syntax, hence dropping support too early. We also cannot use a word
boundary (\b) because that would prevent matching numbers with trailing
dots (e.g. "5.").
Bug: T212730
Change-Id: Ibf6ac571f6b5c09149d69a19c38240ce6b024dff
This bumps the level to WARN, and makes it very clear that people should
fix the affected filters. It also removes the calling method, which was
mostly meant for debugging purposes, and changes the type to 'op_type'
to avoid conflicting with type:mediawiki in logstash.
Bug: T156096
Change-Id: Ie73f1604e8ed82bc2e1be9fc90fa065be37889a3
Always run the keyword/function handler, even if there are DUNDEFINED
arguments, so that the handler can perform further validation on the
input and report any error to the user. However, replace DUNDEFINED with
DNULL before running the handler, to avoid special-casing DUNDEFINED in
every handler. If any argument was a DUNDEFINED, we will return
DUNDEFINED anyway.
Also centralize the keyword handling logic to a new method, like it
happens for functions.
Bug: T234339
Change-Id: I875cb77418a39790e91fe5867c49917bfe406ed4
This emits its own error because:
1- It's clearer to understand
2- It's easier to find where we're dealing with negative offsets, if
we'll ever want to allow that.
Note that trying to use a negative index already results in a hard PHP
error being thrown.
Bug: T237219
Change-Id: Ib11eaaca5e21f740269141c75e62bac48093e8d0
In Ib7427e15f673a575738489476e604c387f449ddd, I thought that $parameters could've only been null if $action wasn't
enabled, but actually, they're null even if the action is just not set.
Which is true for all actions when creating a new filter, and all
non-set actions when editing an existing one.
Hence, revert the part that touched ViewEdit.
Also add a selenium test to ensure that warn parameters are visible.
Bug: T236286
Change-Id: I8150baa077208eb1fc54ebc1d8415a243d0f3bd3
This is some sort of Hello World for selenium. This patch adds the
config files and a couple of very basic tests.
Bug: T214478
Change-Id: I8193b4edb40332bea1d08e24ec020bf36004320d
This is similar to the old parser: when discarding a node, actually
evaluate it if short-circuit is not allowed.
Add a whole lot of tests for all possible exceptions.
Move the logic to extract a message from an AFPUserVisibleException away
from the parser, to keep unit tests working.
Bug: T232498
Change-Id: I31ee4e255c6a87dd693b9bcd582539fdf57acd45
This implements T230982#5475400, and it should speed up the CachingParser by roughly 40%.
Bug: T230982
Change-Id: I803cc58637d50eb90e57decf243f5ca78075d63d
Setting 'apiHookResult' results in a "successful" response; if we want
to report an error, we need to use ApiMessage. We already were doing
this for action=upload. Now our action=edit API responses will be
consistent with MediaWiki and other extensions, and will be able to
take advantage of errorformat=html.
Since this breaks compatibility anyway, also remove some redundant
backwards-compatibility values from the output.
To avoid user interface regressions in VisualEditor, the changes
I3b9c4fef (in VE) and I106dbd3c (in MediaWiki) should be merged first.
Before:
{
"edit": {
"code": "abusefilter-disallowed",
"message": {
"key": "abusefilter-disallowed",
"params": [ ... ]
},
"abusefilter": { ... },
"info": "Hit AbuseFilter: Test filter disallow",
"warning": "This action has been automatically identified ...",
"result": "Failure"
}
}
After:
{
"errors": [
{
"code": "abusefilter-disallowed",
"data": {
"abusefilter": { ... },
},
"module": "edit",
"*": "This action has been automatically identified ..."
}
],
"*": "See http://localhost:3080/w/api.php for API usage. ..."
}
For comparison, a 'readonly' error:
{
"errors": [
{
"code": "readonly",
"data": {
"readonlyreason": "foo bar"
},
"module": "main",
"*": "The wiki is currently in read-only mode."
}
],
"*": "See http://localhost:3080/w/api.php for API usage. ..."
}
Bug: T229539
Depends-On: I106dbd3cbdbf7082b1d1f1c1106ece6b19c22a86
Depends-On: I3b9c4fefc0869ef7999c21cef754434febd852ec
Change-Id: I5424de387cbbcc9c85026b8cfeaf01635eee34a0
This is because there are many filters using this feature. Moreover, it
could make it a little easier to add new arguments, just like dangling
commas in PHP arrays do.
Also re-align the CachingParser code of doLevelFunctions to the one in
the old Parser.
Bug: T153251
Change-Id: Ie4325159f47310788da57415a5e36e62aa4efad0
This will help mitigating problems like T230256 by enforcing that the
requested variables must exist. For now, it will only log bad usages,
thus providing a way to identify affected filters and fix them.
Bug: T230256
Change-Id: I7a61916576e444a56f0e07da7b6e5033346226bd
Using `new LanguageEn()` involved a global, so use a MockObject instead.
Also fix LoggerFactory usage in Tokenizer to use DI instead.
Change-Id: I94d03f9459ab6444e239386eb96a0c2434bfe3dc
We don't need to call it in the constructor, as long as the call in
setUp() is moved to before we start adding groups and checking blocks.
Follows-up Id8412e2b8a4e873fd4821ecc1a3c95710be9a.
Change-Id: I339363499f99295a83004074d6a44574cd622a58
IMHO these can be considered unit tests; they were already fast, but now
they're executed in an instant.
This requires several changes: 1 - delay retrieving messages in
AFPUserVisibleException, to avoid having to deal with i18n whenever we
want to test exceptions; 2 - Use some DI for Parser and Tokenizer.
Equivset-dependend tests are also moved to a new class, thus helping to
fix the AF part of T189560.
Change-Id: If4585bf9bb696857005cf40a0d6985c36ac7e7a8
Some of these are transformed into real unit tests, while the
AbuseFilterSaveTest class is refactored to avoid using the DB and to use
a lot more of mocks and DI.
Depends-On: I22743557e162fd23b3b4e52951a649d8c21109c8
Change-Id: Id8412e2b8a4e873fd4821ecc1a3c95710be9a870
While this is not as important as throwing for too few parameters, IMHO
it's still important to fail in this case. Mostly because if a function
receives too many parameters, chances are that who wrote the filter
didn't do that intendedly, and thus there may be a hidden bug.
Bonus: fix a few docblocks.
Bug: T230803
Change-Id: Iac2931f17b50ace8c8f4c2faa44b3f54ca134c54
In general, it's not safe to change configuration in the middle of a
test, because services could wind up in an inconsistent state. In
particular, I'm trying to have setMwGlobals() reset services, which will
cause stuff to break if it happens in the middle of a test. So just
specify the settings you want up front, like in setUp().
Change-Id: I00e35ecea6a27468674b2a6e7d9d9eb6518e3bd5
This allows a little bit more of abstraction: we can store other data in the
tree, without having to store it in a specific node (e.g. the variables map,
which is still unused). It also adds a few typehints, and specializes
the return value of eval'ing the AST: previously, it was the one of
evalNode, which wasn't guaranteed to be an AFPData. Now we have this
guarantee. Last but not least, we can now measure runtime metrics for
evalTree, which doesn't recurse.
Bonus: fix a check in the old parser, which used the wrong variable when
reporting outofbounds errors.
Change-Id: Iff806793b1d968e9bb6220f1459f3d0ac587c7da
And fix a couple of minor bugs.
Bug: T156096
Depends-On: I3b85087677607573f4fa68681735dc35348dcd87
Change-Id: Ia4c713a1d45827f6a8bc5566a8d8835c49f8108a
Ensure that the variable isn't set before marking it as DUNDEFINED:
that's only for when we cannot use a default, but if the variable is set
we already have one. Most notably, this fixes conditionals handling: right
now, if you have a conditional with an assignment in both
branches, the variable will be undefined. That's obviously wrong, so
it's fixed in this patch.
Plus: catch only AFPExceptions in a test to avoid unintentionally
catching the assert exception; simplify some assignments using wfSetVar.
Depends-On: I446a307e5395ea8cc8ec5ca5d5390b074bea2f24
Change-Id: I8e7f7710b8cb37ada8531b631456a3ce7b27ee45
This patch includes various fixes to how func arguments are handled in
CachingParser:
- Add a comment about a future improvement of checkSyntax, which we
could limit to try building the AST.
- Having enough args for each function is now also checked when
building the AST. This allows implementing the previous point without
stopping to report notenoughargs at syntaxcheck-time (otherwise it'd be
a runtime error). And it also ensure that we check for the params count
inside skipped branches, e.g. inside if/else: these were already only
discovered at runtime in CachingParser. The old parser is not affected
by this change, because when checking syntax it will always execute
all branches, and at runtime it will skip braces altogether.
- Fix arg count for CachingParser, which previously added a bogus param
in case of a function called without parameters. This was fixed for
the other parser in I484fe2994292970276150d2e417801453339e540, and I
just ported the updated fix. Also note that the CachingParser was
already failing for e.g. `count()`, but instead of complaining about
missing arguments, it failed hard when trying to pass NULL to
evalNode.
- Fixed some tests not to use setExpectedException, which caused the
previous point to remain unnoticed: calling that method prevents the
loop from continuing, and thus only the AbuseFilterParser part was
being executed. The new implementation checks the exception ID and is
thus more future-proof if the i18n message changes.
- Fixed some function names in error reporting for the old parser.
- The arg count is now checked outside of the function handlers, thus
it's no more necessary to call checkEnoughArguments at the beginning
of each handler. This also produces clearer error messages in case of
aliases (e.g. set/set_var).
- Check the args count even if some of the args are DUNDEFINED. This is
much easier now that the check is outside of the handler. This will
make syntax check fail for e.g. `contains_any(added_lines)`.
Bug: T156095
Change-Id: I446a307e5395ea8cc8ec5ca5d5390b074bea2f24
The regression itself was fixed in
I980aec3481a52ecc35f1811a366014a5581a7cdb, so this patch only adds a
test for it.
Also remove a comment about CachingParser failures: we don't want to
encourage people to remove it from tests anymore.
Bug: T152281
Change-Id: I3ad49050ea49bf45d3226878e091da3c8dbefdb1
Just like we do for functions, it doesn't really make sense to have
keywords separately, in AFPData.
Change-Id: I208a9b1ce2bd12038e9fbcc515c48d604ec80eb8
This is more complicated than the := operator, because the var name
could be a complicated expression, and we have to handle a function
call. This patch only covers the case where the variable name is a
literal, which is enough for WMF production.
Bug: T214674
Change-Id: I6c0f8e95663919a0235b5ccf0c88ad0a539315a7
As for all mostly unused consequences, blockautopromote has a couple of
major problems: first, it blocked the status for a random time between 3
and 7 days, which to me makes no sense at all (is it some sort of
casino?), and this patch fixes it to 5 days. Second, nothing was logged,
not the blocking nor the unblocking. Here I'm adding a LogHandler for
two new sub-actions of 'rights' to keep track of both action.
Bug: T49412
Change-Id: If48a48f5b8baaf9e77c0826466f5d03bb7f691d0
The last step of the profiling overhaul. See T53294 for the original description by Dragons flight.
Note: Here I'm adding a FixMe for a problem which already exists in the code
and the child patch will fix it.
Bug: T53294
Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: Ib12e072a245fcad93c6c6bd452041f3441f68bb7
I5ec4ab44c4e88aaf18c0d7b73355d27050beeda7 almost fixed this bug, but we
also have to make it possible to access builtin variables as arrays.
This will only make sense for a few variables (e.g. added_lines and
removed_lines), but I don't think we should validate it when checking
syntax.
Bug: T198531
Change-Id: I417e1b8d4802bbfccd091ce5c7617659cfd1e4ea
Instead of seconds, and round the average condition at 1dp instead of 0.
Split from child patch by Dragons flight.
Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: I339aed5f8c1d49714e7927ce49286f9ce6c839f5
They're currently stored separately, so move matches count together with
other per-filter data to keep it consistent. This also removes a
parameter from filterMatchesKey, as it's not needed anymore.
Split from child patch by Dragons flight.
Bug: T53294
Depends-On: I8f47beb73cfc1b63c4b3c809fc6d65a1e66ee334
Change-Id: I2d8c8f8278073a9420e3eb373fb89a655925618a
This patch includes:
* Making it possible to access offsets of a DNONE (returning a DNONE)
* Initializing user-defined variables as DNONE inside short-circuited branches
* Make DNONE propagate with other operators
* Make DNONE count as false for logic operators
* Remove a now-outaded bit in doLevelAtom. In case of shortcircuit,
$result is now DNONE instead of DNULL, and thus it's possible to
access offsets of it. Performance++!
* Don't allow modifying or adding an element of a DNONE as if it were an
array (to avoid inconsistencies)
This re-applies Id85c673337fa90a3782fd22eb9690cd996967111 with several fixes.
NOTE: Haven't tested locally, although I'm pretty confident thanks to
the amount of tests added.
Bug: T214674
Bug: T228677
Change-Id: I5ec4ab44c4e88aaf18c0d7b73355d27050beeda7
Currently we strongly abuse (pardon the pun) the AbuseFilter class: its
purpose should be to hold static functions intended as generic utility
functions (e.g. to format messages, determine whether a filter is global
etc.), but we actually use it for all methods related to running filters.
This patch creates a new class, AbuseFilterRunner, containing all such
methods, which have been made non-static. This leads to several
improvements (also for related methods and the parser), and opens the
way to further improve the code.
Aside from making the code prettier, less global and easier to test,
this patch could also produce a performance improvement, although I
don't have tools to measure that.
Also note that many public methods have been removed, and almost any of
them has been made protected; a couple of them (the ones used from outside)
are left for back-compat, and will be removed in the future.
Change-Id: I2eab2e50356eeb5224446ee2d0df9c787ae95b80
Added in I5a14d4b2bc3ffd9caaaa095f16f36b9b6009db05, but .r files aren't
used anymore since I6c06e596587750c4ebaabafbd277bc75eeb436a5, and I
forgot to remove the file upon rebasing.
Change-Id: Id688d215b1136bd0a04b8c0d8d8d16de5da1295e
This should allow more flexibility when checking syntax, and a saner
behaviour overall.
Aside from not throwing exception in certain cases, the results should
be almost equal to the ones you would get without this patch. However,
there are still a few things to improve (which for convenience I wrote
inside the parser test) and many to test.
Bug: T204654
Depends-On: I69bfec45c76509fb1112641393f78e8d8834adcd
Change-Id: I5a14d4b2bc3ffd9caaaa095f16f36b9b6009db05
Aside from the 14 thingy reported in the task, this syntax is awful! The
fix to the regex should only be intended as a temporary stopgap. A
proper fix would be to introduce a new syntax, like for instance the one
used in PHP.
Bug: T212726
Change-Id: Idc37a17ce539e6c63d67fc07d47d812569debe0e
This property is meant to be private, since it has all kinds of
getters/setters, aside from one which is introduced in this patch.
Change-Id: I217b1e22cabd3c0468c84b1d6a69a6ed3c6fa8e6
The current form is awkward. They're all like
[ actionname => [ 'action' => actionname, 'parameters' => params ] ]
This is greatly confusing since adds a nesting level, and just
duplicates the actionname information (also, we actually never retrieve
it from the internal array). Instead, change all of them to be
[ actionname => params ]
which is a lot shorter and clearer (and easier to handle).
A similar case is handled in I8134ecc41fbecdbed99faf406e9e3ca91b6123b9
(see PS 8..10).
Change-Id: I34c040dbeb3ab01158fb3db22496def6ccaf72d9
As explained on phabricator, they don't work with shortcircuit, so they
already fail for all filters using them. Plus IMHO it's an unnecessary
deviation from PHP's behaviour, given that this syntax doesn't do what
users may expect.
Bug: T218906
Change-Id: If9e7545e14044c8dc3b4163bb6fca8ab0683b9fa
Using the new PageEditStash class allows to simplify a bit the
integration tests for edit stashing. As I wrote in a ToDo, it may be
enough to manually run the hook, but that's left to do as a follow-up.
Change-Id: I3389a6961b4f39ecd980be2f429c23f8b7706a15
Instead of relying on static methods and members in the AbuseFilter
class, move everything related to conditions inside the Parser, as the
amount of used conditions is something pertaining a single
AbuseFilter(Caching)Parser instance.
This change requires changing some signatures and adding parameters,
but will make introducing the new AbuseFilterRunner class easier (and
that will clean signatures, too).
Depends-On: I5b29ff556eca45fe59d15e2e3df4d06f1f6b3934
Change-Id: I7c1ea17adf7f42cf9260d416906bfbf3b8a20688
Now it returns an array with a bit more info, and has a different name
to reflect the fact that its input is now split in two parts. Plus, make
it throw whenever it gets an unexpected input, and add a bunch of test
cases for it.
Depends-On: Ib5fdeb75c1324f672b4ded39681f006fde34b4d1
Change-Id: Ie550889495232b534c0f9aec31039cf21b2135b1
Partial revert of I4dd81a723e2bdb828b90594ad66a3918d8ec5b6c.
Thinking again of it, I think it's not worth it to have this data over
the network. Plus, given that it's not-that-slow to be computed, I think
there can only be a performance gain in using APC (as opposed to e.g.
memcached/redis) for 99.9% of the filters.
Change-Id: I8c6a4a95ec12c18ede8e6419540f7a2ac943457c
Added cachingParser back to *all* the parser tests, fixed a couple of
differences with the normal parser, and added a couple of tests so that
any cachingParser-related file has 100% coverage. Also move the remaining
get_matches tests inside parserTests, and specify the parser used in case of failure.
This also adds a new base class for parser-related tests with a couple
of util methods.
Bug: T201193
Change-Id: I980aec3481a52ecc35f1811a366014a5581a7cdb
Another crucial part to have covered. Also clarify that
AbuseFilterCentralDB can be of the form "dbname-prefix".
Remove a filter used for profiling and replace it with a global one:
we're still fine, and the list is kept shorter.
Bug: T201193
Depends-On: I5ee7ba44a6cd82a5ddb24fb4127af04d96e647f4
Change-Id: If6b91711534c0d60e1aa27bd5748c3023e29f376
Yet another important part to have covered. While for normal edits it
already works, for stashed ones it doesn't. That's why we need the patch
for checkAllFilters. Since for stashed edits profiling stats are all
zeros, this may explain T201334.
Changed the timestamp variable to use wfTimestamp instead of time() so
that we can fake it inside unit tests.
In a subsequent patch we should add average runtime conditions to tests
(really tricky).
Bug: T201193
Depends-On: Ib17821240b25c972a187e6b5eae42c5ada6c65e7
Change-Id: I5ee7ba44a6cd82a5ddb24fb4127af04d96e647f4
This is an important part to cover, and should be further expanded.
Also, fix a couple of minor things around, including making some methods
non-static.
Bug: T201193
Depends-On: I5e35d773904a62105767ce6d7d962ab5525c2d12
Change-Id: Ib17821240b25c972a187e6b5eae42c5ada6c65e7
This is fixing potential bugs where invalid strings with more than one
comma have silently been accepted.
Change-Id: Ib1e7d0c99973f243ef6faad6389bab688187c1cf
I find it obvious that a file called "AbuseFilterTokenizerTest" is a
"test for the AbuseFilterTokenizer class". A comment that is just
repeating this information is typicalls not helpful, but distracting
and a potential source of mistakes, e.g. when stuff is copy-pasted,
but the comment not adjusted.
Change-Id: I1d4cc06e9e5631955ff73bf675090cf9c33c9390
Split a method, use WAN cache so that we're enabled to use
getWithSetCallback, pass the "version" option there and adapt the test
to it.
Follow-up of I9b3bc36b552901bc6ca7609ee51e80be2979a9c4
Change-Id: I4dd81a723e2bdb828b90594ad66a3918d8ec5b6c
I didn't fix every case where this happens, just what blocks
I6ddcc9f34a48f997ae39b79cd2df40dd2cc10197 from landing.
Change-Id: I971e619eb76c4474fe037fad258f9c496717bf41
Caching the result of the tokenization is pretty important
performance-wise, so this test ensures that caching works as expected.
I have also extracted the method used to generate the cache key for
easier testing, and moved the cache instance to a class member because
otherwise that piece of code can't be tested...
Bug: T201193
Change-Id: I9b3bc36b552901bc6ca7609ee51e80be2979a9c4
These are the ones which other tests don't cover, mostly because no
filter syntax can trigger those cases. This patch should bring coverage
for AFPData to 100%.
Bug: T201193
Change-Id: I997576141943959d4602a9f839311108928ec766
Follow-up of Ic30883f7d261d974a2be46308d023e2714119e95, with two files
that I forgot to git-add and a repositioning of comments to avoid the
last bracket to be reported as uncovered.
Bug: T201193
Change-Id: I6bf7e5892a0f49f6a138792f0aedf230a70c18a8
This patch mostly adds coverageIgnore comments for intendedly
unreachable code etc. Some of them could be made testable by adding a new
filter function (e.g. array cast), but this patch is meant to be
comment-only (aside from the parser test).
Ignoring coverage for these lines makes some methods reach 100%
coverage, which in turn makes it easier to look at the coverage chart
and identify at a glance which parts of the code *really* need to be
covered.
Bug: T201193
Change-Id: Ic30883f7d261d974a2be46308d023e2714119e95
These are specific tests for storeVarDump and loadVarDump, both alone
and in the context of running filters.
Also, include disabled variables in the VariableHolder object if they're
saved in the DB.
Bug: T201193
Depends-On: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
Change-Id: I5e35d773904a62105767ce6d7d962ab5525c2d12
If the User passed to $logEntry->setPerformer() represents a creatable
username, then it has to actually exist so the actor row can be created.
Bug: T188327
Change-Id: Iab2fc9593a020ffacd219d644103d685028e3336
Mostly delete result files and assume the result is always true. The few
exceptions were either moved to standalone test, or inverted.
Change-Id: I6c06e596587750c4ebaabafbd277bc75eeb436a5
The reasoning is similar to the one of the parent patch (Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb). Plus, it records runtime metrics on action different than edits, as there's no reason not to do it.
No performance issues in production.
Bug: T191039
Depends-On: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
Change-Id: Ib1112e2fefd0631550d386ba87e5f87db84c3036
This variable was introduced to selectively enable profiling because
stats recording was bad for performance. Nowadays, stats are recorded in
a deferredupdate and don't harm performance anymore. Thus, this variable
can be removed and profiling be enabled by default.
Bug: T191039
Depends-On: Ib5fdeb75c1324f672b4ded39681f006fde34b4d1
Change-Id: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
Follow-up of I1721a3ba532d481e3ecf35f51099c1438b6b73b2. This is the only
wrong replacement: strict checking will let 5 / 0.0 pass, with
unexpected results. Adding a regression test for it, too.
Change-Id: I25dbe9fafa92fd9a11bd8bc6ab8e66f305b8d48e
Since double-equals are evil. I left some of them in place where I
wasn't sure, but I may be changed some which were intended to be
doubles. It could be a good idea to delay merging this patch until we'll
have more code coverage.
Change-Id: I1721a3ba532d481e3ecf35f51099c1438b6b73b2
If "tag" option is selected and the form is submitted without adding any
tag, just show it blank instead of adding an empty tag to the topbar.
Separately validate the empty tag case (and added a test for it).
Bug: T203353
Depends-On: I3b2e763bd8835207dc5df1db43d3e1881e6961c3
Change-Id: I8884b739fd17fa2eace5aac8775d3524aa606f1f
Adding PHPdocs to every class members, in every file. This patch only
touches comments, and moved properties on their own lines. Note that
some of these properties would need to be moved, somehow changed, or
just removed (either because they're old, unused leftovers, or just
because we can move them to local scope), but I wanted to keep this
patch doc-only.
Change-Id: I9fe701445bea8f09d82783789ff1ec537ac6704b
Remove all globals, make methods non-static, improve assertions and
computing some variables, add names to the tests and other minor
improvements.
Change-Id: Ifbcd9adf34d173d0da0aa568fc6f91fdc2d61609
Where prevents is used as a setter, use the new setter methods;
where it is used to determine whether a block blocks the target
from editing their talk page, use appliesToUsertalk.
Block::prevents was deprecated and replaced by several other
methods in I0e131696419211.
Bug: T211578
Change-Id: I166cc6f64c0f895ff8c631d2655c1c3208131371
The @expectedException annotation got deprecated in PHPUnit 7.5, and
removed in PHPUnit 8.0. This was done because the annotation does have
two disadvantages:
* The class name is encoded in string, where it is not easy to find for
all IDEs and tools.
* it did not allow to say exactly *when* the exception is expected.
Change-Id: I85f0b5f44b2f400a121115d402b64827ea534c32
Using break could halt parsing between operations, instead use continue
to parse all operations.
Bug: T214642
Change-Id: If67ddaffef280c2448c55ae536013758617bba68
For wgLang, there's a Language object available in the proximity, so just pass it.
For wgContLang, use MediaWikiServices.
Change-Id: Ic492007f2d5eeb8048d0919a4b9b7dd98c15c350
This makes the code easier to maintain and more flexible, plus adds
several tests. Some flaky tests are also improved.
Depends-On: I57ce67c5202c8574fcf1957999a6999fec264cb7
Change-Id: Ibb5322bca93b464e9014b53644c04f2bc1141e72
We just passed the description as a parameter, but it's much quicker to
use it as the key in the data provider: PHPUnit will automatically
display it in case of failure, so that we don't have to do that
manually (and still get messages like "failed with data set #7").
Depends-On: I8edcca17ecdcf71397cc9b0d101e8b13ac112047
Change-Id: I57ce67c5202c8574fcf1957999a6999fec264cb7
Right now, we allow empty messages, and when the "warn" action is
executed we use "abusefilter-warning" if no message is specified.
However, this also produces a PHP notice while editing a filter with
empty message (see Phab). With this patch, empty messages will be
rejected, and a follow-up will be discussed on Phab.
Update: added disallow message as follow-up of
Ic1de03a6944c43a346fa317ee0a217551f0d284a.
Bug: T203353
Depends-On: I8df247f61d9f3769e9580544f324dd174811e939
Change-Id: I71b1f81d10c02de4de141b1ab9b630d05cf4619c
As follow-up of I10b1fd2d9bdfe518089c053d77fef568170ecb65, use
'AbuseFilter' instead of 'AbuseFilterDeprecatedVars' as channel name.
Raise level for null-title filtering. Since with a null title
several things are likely to break, a warning is more appropriate here.
Tweaked the message as well, to include the bug number and to avoid
pointlessly including the title (which is null).
Lower the level for stashedit hit/miss (as it's really spammy and not
that useful right now).
Use 'abusefilter' instead of 'AbuseFilter' for statsd so that everything
has the same prefix.
Also raise the level for parser exceptions and unrecognized
consequences.
Change-Id: I1f9988155e924232b201281795cd322636da8082
Follow-up of Ic1de03a6944c43a346fa317ee0a217551f0d284a, adding some unit
tests for this newly introduced feature, plus a couple of tweaks for
both tests themselves and i18n.
Change-Id: I8df247f61d9f3769e9580544f324dd174811e939
This is a first step towards MCR support in AbuseFilter. The textual
representation of all slots is concatenated. Since AbuseFilter uses
getTextForSearchIndex to determine the textual representation of
content, blind concatenation should not break any assumptions
made by AbsueFilter rules: this naive approach is no worse than
AbuseFilters handling of non-textual content in general, and should
work fine for textual content.
Bug: T209291
Change-Id: Ic141085cad2e11bfe106fe83dafcb35ac31206ba
For several reasons:
*We're not really checking permissions (and the hook previously used is
meant to be used in such case)
*We'll show a cleaner error message (i.e. without the "You do not have
permission..." part)
*Filtering will happen closer to the actual move
Bug: T208907
Depends-On: I4733724075b7514e9db59e7be772d9409aa9da87
Change-Id: If88f736a446247f8b4b13c055c641d56f544d1ea
Simplify some logic constructs, reduce the amount of return statements
inside methods, explicitly declare variables before using them, reduce
code duplication, add names to JS anonymous function to produce clearer
stack traces.
Change-Id: Ife4546a91c30d4c519d09a712ba56a2f33abe579
Long (sigh) explanation in T203587#4569698. Also, simplified the way
TagMultiselect are generated, this one and the one for change tags.
This new selector is back-compat both with the old textarea and the OOUI
checkboxMultiselect; actually, this one is //fully// compatible with the
old textarea.
Add validation for throttle parameters and unit tests for validation
(split from I976c95658cddb2585910b6f8a5f047aadc4e4d47).
Added a trim when retrieving throttle identifier to allow syntax like
'ip, user'.
Improved the message shown on history.
Re-added the maintenance script to clean DB.
As I wrote in the task, a review by two other people would be great, at
least for the maintenance script (it could potentially break the DB).
Bug: T203587
Bug: T203336
Bug: T203584
Bug: T203585
Depends-On: I3b2e763bd8835207dc5df1db43d3e1881e6961c3
Change-Id: I7831dbb0bab55807392ac1f7915d6cb0cb713593
* AbuseFilterConsequencesTest is somehow leaving blocks behind. Mark
ipblocks as being used to avoid that.
* AFComputedVariable::getLastPageAuthors() uses indeterminate order for
multiple revisions with the same timestamp. Fall back to rev_id
ordering like MySQL accidentally did before.
* AbuseFilterTest tries to create revisions attributed to users that
don't exist. Switch to interwiki usernames.
Change-Id: I30f7cdcc3875f3f7af116c1e41e88f62ab9e91d0
These are updated in deferred updates and should not rely on the same
User instance being used in those updates. This also avoids convoluted
logic in User to set the new edit count for various cases.
Change-Id: I6d239a5ea286afb10d9e317b2ee1436de60f7e4f
This simplifies the test for user_age, although I'm not totally sure it
will be fixed. AFAICS, there's nothing wrong in there, but we'll see on
future phpunit executions.
Bug: T206501
Change-Id: Iee1a2a65d08c2cffc7a0d655be1eadb018d8bf37
Use a single function to check parameters amount, avoid duplication
between keywordIn and keywordContains, use if...elseif instead of
if-else when statements have a return inside, simplify some other logic,
add typehinting, and change method visibility according to use of such
methods.
Change-Id: I22225a5cbbb93679a0e78bf6e15866829167fbf4
Fixed some comments adding explanations, fixing syntax, and parameter types
for docblocks. Also fixed some whitespace mess, and added a missing use
statement.
Change-Id: I3547c90bdaa2cab5443e8bf0c63b217fe6ba663f
This problem have been making filters potentially fail silently since
2009. Also add tests for arrays to make sure that no problems arise
when short circuit is used.
Bug: T204841
Change-Id: Ie4e2e06498c1202ba73afcc5d164a72427abbca5
This test checks every deprecated variable to be identical to the
newly-named one, and to emit a debug notice. It also changes such debug
to be emitted via logger instead of wfDebug.
Bug: T201193
Bug: T173889
Change-Id: Ie55746bb7731062ae2d46d84857af2a05d78cf4c
This will make tokenizer almost fully covered. The only uncovered parts
are the one with cache and an else condition which I think won't ever be
executed, and thus added a comment for that. Also, remove an obsolete
xxx comment from ComputedVariable (fixed in
I8e420f0259ef6c9e579f7a00beb58f28af9da37d)
Bug: T201193
Change-Id: I6e9a73aa9e437f096f6a1e20d53a7cb50e5ed85d
This should help with tracking code coverage and also explains some
coverage discrepancies encountered while writing other tests.
Bug: T201193
Change-Id: I8b20abc46c2d6c6f582953139b9a9f3710b2e4ea
Check a bunch of them, they should be computed and be identical to the
ones with new syntax.
Bug: T173889
Depends-On: I5c370b54e6516889624088e27928ad3a1f48a821
Change-Id: I276913a98e06b5f2ff1c5f5f3ba5bcc7b1e8c997
Variables regarding title (full list in task description) are quite
deceiving, since they use "text" instead of "title". As proposed in the
task, this is the first patch to add aliases for those variables and
slightly deprecate the old ones. In the future we may be able to replace
every occurrence (either with a search function or directly on the
database), but even a coexistence would be enough to avoid
confusion. A wfDebug log is generated whenever a deprecated variable is
parsed. The "article_" prefix is also changed to "title_", in the same
way as above.
Also, added a hook which other extension may use to specify their
deprecated variables, which will be handled the same as core ones.
Bug: T173889
Change-Id: I5c370b54e6516889624088e27928ad3a1f48a821
Add some tests and improve others to raise coverage percentage. This
should lead to almost 100% for the AbuseFilterParser class. Aside from
this, a couple of changes:
* Remove an unused function
* Let equals_to_any return a genuine result with empty strings
* Remove an if which will never be true in skipOverBraces, since the
function is called after checking the same conditions.
Bug: T201193
Change-Id: I7020b2ed996236c38c5784d161ad98ec44163406
We're currently emitting the same error twice, but in one of those cases
it's completely wrong. Damned copy&pasting!
Bug: T202073
Change-Id: I7687826a85f3ef0abaf15d7cd973afc4e55758b2
Adding tests for generic functions in AbuseFilter class, ranging from
simple utility function to variable computation.
Bug: T42478
Change-Id: I903fb7ffbc436b27462e3e4611ab65ecb8a543ba
Adding the template for unit tests and some tests. These should cover
all the validation failure cases.
Bug: T42478
Depends-On: Ib7a0335fa7fb3b8a21765438a720205656c1ea09
Change-Id: I3fd0d627295d680ed33b1cbc730435df0446277f
The last one of what I think are the must-have tests. This patch
provides the basic tests and the framework, which may be further
expanded later on. Please note that the failures are due to an actual
problem in core, for which there is I7bb0e92b2906a2511fc4290bdc76fc39ec4617fe.
Bug: T42478
Change-Id: I28eb464c63fda7faa3ec7d1f6082f36154d66962
We're really missing exception tests: in fact, 'noparams' not being
thrown was discovered only a few days ago and worked like that for
years. This patch adds phpunit tests for both noparams and notenoughargs
exception, also checking the returned message.
Depends-On: I484fe2994292970276150d2e417801453339e540
Change-Id: Ia0b9b8fd5c979be06879723b746f9356c628f5cd
Follow-up of Iacb8f7a361079e3e117dc6845597c7bd8473e54a for exceptions
thrown outside the parser. With this patch all uses of AFPUserVisibleException
will be covered.
Depends-On: Iacb8f7a361079e3e117dc6845597c7bd8473e54a
Change-Id: Ia7ef6eb832d5725a804a60cb58bc110b06c8abe2
All uses of "throw" inside AbuseFilterParser are now covered.
Bonus: added a standard suppresswarning when checking regex validity.
Change-Id: Iacb8f7a361079e3e117dc6845597c7bd8473e54a
Arrays were introduced with the name "lists". While it **may** look
user-friendlier and so on, it actually uses a wrong name: lists are
different from arrays. I ran a grep and I should've replaced
every occurrence, plus everything seems to work, however a double check
wouldn't be bad.
Change-Id: I6a858f02f5dd9250ba7e1abf9c6422fd98758c9e
This is taken from I6a57a28f22600aafb2e529587ecce6083e9f7da4 and makes
all the needed changes to make phan pass. Seccheck will instead fail,
but since it's not clear how to fix it (and it is non-voting), for the
moment we may merge this and enable phan on IC.
Bug: T192325
Change-Id: I77648b6f8e146114fd43bb0f4dfccdb36b7ac1ac
I added on MW an example of comparison with empty array, which we should
keep inside the dedicated test as well.
Change-Id: Ifa4bca85c8978ef24ed5bb26787730bb4521261f
