@Huji Dang! I checked on logstash (from mobile phone, which is painful) and heck yes! AbuseFilter 101 is constantly taking ~150 seconds to execute on that page!
You are right, profiling is enabled on every wiki (and I already sent patches to remove profiling globals altogether). However, the avg execution time for that filter doesn't show up as very high due to the amount of edits (that's why there are patches to add the maximum recorded execution time). From a quick look I can see a couple of things to be optimized in that filter, please let me know if you need any help with it.
Aye, I was right! The prefix 'ARTICLE' wasn't updated to 'page'. The impact is that, in order to test uploads, users have to use article_* vars instead of page_*. Nothing else is affected and no need to backport as these values aren't saved in the DB.
TerraCodes is right, see for instance T218560.
@Legoktm I checked all the remaining versions up to 1.2.6 and I can now confirm that we only need 0.1.5 for php-ast. 0.1.2 should be kept for the current version only.
Wed, Mar 20
Ah, now I see. Would it still benefit from phpunit?
So I'm looking at the upgrade to PluginV2. The main change is that *Visitor classes are now instantiated by phan itself, which only wants a "get*ClassName" method instead of a constructor. This mostly means that we cannot pass in an instance of the plugin anymore. Moreover, Visitor and PreVisitor must inherit from different classes, which prevents them from having TaintednessBaseVisitor as common parent (should be fixable by turning TaintednessBaseVisitor into a trait). Plus, the MediaWiki checker runs both the generic visitor and the MW visitor, which again isn't possible in V2 because you can only provide a single class name. I'll probably do some gradual changes before the v2 upgrade.
@Legoktm Right. But anyway at the end of the migration we'll only have 0.1.4 for the current version, and ? for the final version based on 1.2.x.
@Legoktm Cool, thanks! I think for now you can use whatever hack comes more handy for seccheck, as at the end of the upgrade we should ideally have a single version installed.
A conditional checking the required phan version seems fine.
@Legoktm I think the current version should work with 0.1.4. But actually, could you please add 0.1.5 as well? We'll need it for the next upgrade. Thanks!
Tue, Mar 19
@Krinkle IMHO this should be handled in core. Failures of deletion during page move should show up to the user like the ones for normal deletions do, instead of throwing.
@Legoktm could you please take a look?
Nitpicky comment: the description "Jobs restricted to trusted users. Will vote +2." for the test pipeline should be changed.
The switch to phan's PluginV2 as part of T216974 should boost the speed per PluginV2 documentation.
This would indeed be awesome. However, see my comment in T207344#5033198. Basically, phan has many breaking changes even in patch versions, so upgrading to 0.8.13 would already break a lot of stuff. Today I tried to get there, but eventually gave up due to this.
For whoever will want to proceed with this, here are the main obstacles I found while upgrading to 0.8.13. Note that I don't really know how phan works so it could be fault of mine (but I'm still happy to help).
- Clazz::getPropertyByNameInContext now has an extra required parameter, $is_static. However, I couldn't find a way to determine if the property is static where we call it.
- This commit causes 2 troubles:
- It adds the new class UnaddressableTypedElement, which is extended by Variable. This breaks all typehints for TypedElementInterface in TaintednessBaseVisitor. Just removing them (and adding UnaddressableTypedElement to the docblock) seems to work fine, but it should be checked more deeply.
- Parameter::getContext() is replaced by Parameter::getFileRef(), which however doesn't have the getScope() method used in setTaintedness.
Mon, Mar 18
Just a random comment: data actually used by existing abuse filters like the rank can be moved from added_lines to new AF variables defined via hooks.
I'm giving up with the upgrade. Phan doesn't comply with semver at all, and thus as I was saying above you'll face plenty of breaking changes even for x.x.y => x.x.z upgrades. At this point, I think seccheck needs a major rewrite in order to work with 1.2.6. If instead, you want to do it gradually, I suggest bumping to 0.9.6 first (which is roughly the same as 0.8.13), then switch to PluginV2 and slowly move on to 1.2.6.
@sbassett Well, actually I'm facing several breaking changes even with 0.8.0 => 0.8.13. The most important is the addition of UnaddressableTypedElement (which also doesn't have a true context but just a FileRef), which breaks several things in TaintednessBaseVisitor. More specifically, I'm talking about this commit.
@sbassett Thanks for the reply. I'm trying to understand how phan, AST etc. work to see if I can start bumping the phan version. For now I'm trying to get to phan ^0.8 and ast ^0.1.5, although it won't be fast and I cannot guarantee anything.
@zeljkofilipin Yes, it does. And I can also confirm that DateTime is the only input type affected by this bug.
@Bawolff Is there a specific reason to require PHP 7.0.0, or it's just because the plugin is untested with other versions?
Different message but same cause as the other task, which already has a patch.
A quick look points at https://gerrit.wikimedia.org/r/#/c/oojs/ui/+/495296/, since setFlags is a method defined in the FlaggedElement class.
Great! Just a note - this setting is not for logging to Special:AbuseLog, but to recent changes. More specifically, to the places defined in $wgAbuseFilterNotifications, as this setting is only used to decide whether to include private filters there. Needless to say, having all filters logged to RC can greatly help patrollers (with or without abusefilter-modify right), who will know what edits are more likely to be bad.
Sun, Mar 17
This is hitting us over and over.
Train blocker because this hides form fields, high prio because there's time to fix it before the train starts.
Hoping that this won't create more problem than it solves!
Sat, Mar 16
@MarcoAurelio Well, "expected" to an extent, as I couldn't find any information about it before the actual shutdown. And BTW, is there a related task/discussion/you-name-it?
Wed, Mar 13
Well, for the specific goals of this task, we'd need a method like isValidRange but for explicit ranges. Maybe something like isValidExplicitRange.
Tue, Mar 12
@Krinkle Those filters being broken is indeed a software bug: every filter is syntax-checked upon saving, so ideally no filter should ever fail at runtime. Which is why I raised the logging level in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/482777/. And yes, it's also high priority, although this has no impact for the end user and, I think, it doesn't affect AbuseFilter result.
Mon, Mar 11
@matmarex Alright, now the Ruby test parser has to be updated. I guess the regex could be changed to allow [-@] at the beginning of the line, and the following case should have an extra when for it. However I don't know Ruby and I'm leaving it up to someone else to avoid messing up.
The patch above implements prop1 + dashes. I'm currently checking it for errors.
Done with phan.
Sun, Mar 10
Confirming. I also noticed the same thing on itwiki a few days ago, but forgot to investigate further. Of note, it's not easy to detect such edits: one may filter the AbuseLog with impact:not saved AND actions taken:none (or tag), but page creations would show up all the same (see e.g. T62179). I don't know why this happens, but I have to say that the code used to populate afl_rev_id (which signals that the edit succeeded) is pretty brittle (see onPageContentSaveComplete). I sort of tried to clean the code a bit in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/478489/, but that's far from being perfect.
Sat, Mar 9
Fri, Mar 8
For those who cannot see it, the subtask regarding AbuseFilter is now resolved, so that's not a blocker anymore.
@MarcoAurelio Maybe it's because profiling is reset when a filter is modified. So if you edit a filter it won't have any profiling data until the first action it tries to filter.
Wed, Mar 6
Tue, Mar 5
The patch for changing Database::select was still pending, but it's better to move it to a separate task.