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
Follow-up Ie9aae938cca06e38a7a834a3f74f3e8735ab01ee.
Some fields are actually necessary when the filter isn't saved. This
would cause PHP notices when showing the editor again.
Change-Id: I2b9e0f04b3e8ad4eea8e334e16ee422bb40f0eb5
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
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
composer:
* mediawiki/mediawiki-codesniffer: 28.0.0 → 29.0.0
The following sniffs are failing and were disabled:
* MediaWiki.Commenting.FunctionComment.MissingParamTag
* MediaWiki.Commenting.FunctionComment.ParamNameNoMatch
npm:
* eslint-config-wikimedia: 0.13.1 → 0.15.0
* grunt-stylelint: 0.11.1 → 0.13.0
* stylelint-config-wikimedia: 0.6.0 → 0.8.0
Additional changes:
* Remove direct "stylelint" dependency in favor of "grunt-stylelint".
* Also sorted "composer fix" command to run phpcbf last.
* Removing manual reportUnusedDisableDirectives for eslint.
Change-Id: I8f73202db1333fbc36ccf556b3bb05b1e8c279cb
Thinking about it again, all messages on ViewEdit start with
abusefilter-edit. Also add a reference to the other message to
facilitate translations.
Follow-up: I3717d06d4a757684fe6622961391ae06b5bd3c38
Bug: T235590
Change-Id: I4cbaa2e92d22296f55a4b5ef0c633fe959fe9ea3
AbuseFilterViewEdit does privilege checks based on filter ID,
and displays what is hidden under given history ID, but doesn't
make sure those two IDs actually belong to one filter.
That means user can easily change filter ID to a public
filter and view old versions of nowadays private filters.
Bug: T237887
Change-Id: Ic12790bd33982473f77551bde9599ed083a3e1f1
Currently, `abusefilter-edit-oldwarning` is shown to all users, but not all users are able to edit the filters, and thus the warning about editing isn't applicable to them.
Bug: T235590
Change-Id: I3717d06d4a757684fe6622961391ae06b5bd3c38
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
We didn't check if the provided ID was valid. While editing an existing
filter (or creating a new one), we check the ID in SpecialAbuseFilter,
so it's guaranteed to get an integer in ViewEdit, and the case of a
non-existing filter is handled later, in buildFilterEditor.
But for links like Special:AbuseFilter/history/foobarbaz/item/1 (where
"foobarbaz" should be the filter ID), no validation was performed. This
caused a useless query to be carried out on the abuse_filter_history table (which would likely return false), then accessing properties of a non-object ('$row->afh_id'), and we ended up showing filter 1. This was spotted because we actually got notices in production.
Bug: T231632
Change-Id: I6436c7d2df8c1f0fc971f4a4079dac9118aa8209
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
There are lots of calls to $user->isAllowed which could be simplified
using available accessors like canEdit(). So simplify those calls and
avoid duplication.
Note that using canEdit also fixes a bug which affected blocked users:
we used to show e.g. the import link, and not to display as disabled
several text fields, while blocked users cannot actually edit filters.
Depends-On: I22743557e162fd23b3b4e52951a649d8c21109c8
Change-Id: I62779e940949ef49018a9c6d901bb6e10aa81da8
There are lots of cases where we can inject a User object without
additional efforts. Now $wgUser is only used inside AFComputedVariable,
which is a little bit harder to handle because some instances of that
class are serialized in the DB, and thus we cannot easily change the
constructor until T213006 is resolved.
This partly copies what Ia474f02dfeee8c7d067ee7e555c08cbfef08f6a6 tried
to do, but adopting a different approach for various can*() methods:
they're now static methods in the AbuseFilter class, so future callers
don't need to instantiate an AbuseFilterView class. This also allows to
re-use those methods in an API module for editing filters (T213037).
Bug: T213037
Bug: T159299
Change-Id: I22743557e162fd23b3b4e52951a649d8c21109c8
After having removed the export link in
I72f46247f4323fb5bfe7fa74f332076dbd346187, we don't have any tool to
show for new filters. So avoid outputting an empty section.
Change-Id: Ia07bccdbadb7b874397135bc3f7468d6e0b9eb13
This was broken in I34c040dbeb3ab01158fb3db22496def6ccaf72d9. I thought
the members of that object were always arrays, but I was wrong.
Plus typehint a few array parameters and make a couple of methods
private since they're only used in this class.
Bug: T230639
Change-Id: I0c51359769c4b3054f95755a96e7e0a2d8e5bf15
Now it's always wider, and so is the "notes" field. Moreover, the
fallback textarea has the exact same size. Plus removed a parameter
which only made it hard to write a CSS rule for the textarea. Since the
textarea is generated by the same code, and we're always using it for
the same thing (filter syntax, regardless of the final goal), make it
always use the same name.
Bug: T230591
Change-Id: Ibb308e80d954c0e81aa09249c38c39572f157948
As data could be "old" and it may have no meaning.
Also remove a superfluous isset(), as $row->af_hidden is always set.
Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: I072363706c61f272c4c3691de4078e2a19148424
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 changes the buildFilterEditor function to be protected and to
behave consistently: so, instead of adding stuff to OutputPage inside it
and also returning other stuff to be added by the caller, the function
now adds everything itself.
Also, the message "you're editing an old version of the filter" is now
shown only if the user can see the filter.
Change-Id: I1f40af41c5de0f63aa6210a261928892da0b3f69
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
To make the switch to afl_filter_id and afl_global easier.
Bug: T227095
Depends-On: Ie550889495232b534c0f9aec31039cf21b2135b1
Change-Id: If557bad8f5c1a6d15e3556e4bfbd0330d7d49c59
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
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
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
Follow-up of I982d67aa62a899916a26452aceb9646df8c31232. The help text
was meant to be localized, and I probably forgot to do so in the
mentioned patch.
Change-Id: If394b02819911f9c97519b5c972977c38e6d83fa
