Description
Details
Project | Branch | Lines +/- | Subject | |
---|---|---|---|---|
mediawiki/extensions/AbuseFilter | master | +19 -25 | Use a WikiPage object when filtering edits on a non-existing Title |
Related Objects
- Mentioned Here
- rEABFf3788c4f0c8c: (bug 42064) AbuseFilter + EditFilterMergedContent
T217970: Some edits aren't recorded as having been saved in AbuseLog (no diff link)
T62179: Page creation caught by the abuse filter should contain a permalink in the log
T261630: Let sysops see difflinks to deleted revisions on Special:AbuseLog
Event Timeline
This is caused by AbuseFilterHooks::$lastEditPage being null in the PageSaveComplete handler. That property is set immediately before filtering the edit, but take a look at this block:
if ( $title->canExist() && $title->exists() ) { // Make sure we load the latest text saved in database (bug 31656) $page = $context->getWikiPage(); } else { $page = null; } // ... self::$lastEditPage = $page;
The Title doesn't exist, so $page ends up being null. However, the PageContentSave hook gives us a WikiPage object for the page, which is obviously not null. I need to check what that bug 31656 is, and why is the code behaving like this. perhaps we might also unconditionally use $context->getWikiPage() for self::$lastEditPage (but maybe not for $page, as that'a also used to generate variables).
I think the behaviour was changed in rEABFf3788c4f0c8cfa418752c5b311e545583c625c9f: previously, it would always set an Article object. That bug 31656 is also unrelated to this.
I tried this, and it worked for me: no errors, and a "diff" link is added, pointing to the page creation. Perhaps it should be guarded by a call to Title::canExist(), but it's something to start with. I've also noticed that we might as well use a non-null Wikipage to generate variables: take a look at this code path:
if ( $title->canExist() && $title->exists() ) { // Make sure we load the latest text saved in database (bug 31656) $page = $context->getWikiPage(); } else { $page = null; } $vars = new AbuseFilterVariableHolder(); $builder = new RunVariableGenerator( $vars, $user, $title ); $vars = $builder->getEditVars( $content, $text, $summary, $slot, $page );
So the page is only used in RunVariableGenerator:
if ( $page !== null ) { $filterText = $this->getEditTextForFiltering( $page, $content, $slot ); // ... } else { $oldAfText = ''; } return $this->newVariableHolderForEdit( $page, $summary, $content, $text, $oldAfText, $oldContent );
There the page was used just to avoid computing variables for the old text, which is fine for both cases where a Title doesn't exist or cannot exist. But this can be achieved by directly checking for that on the Title object. So let's see what the other call is doing.
$this->addEditVars( $this->title, $page );
so $page is passed through to addEditVars, let's see:
// NOTE: $page may end up remaining null, e.g. if $title points to a special page. if ( !$page && $title->canExist() ) { // TODO: The caller should do this! $page = WikiPage::factory( $title ); }
HAH! So it ends up recreating that WikiPage object anyway! I had already marked this code as bad with that "The caller should do this" comment. I think this is a great occasion to fix!
Summing up: I think we can change that conditional in filterEdit and remove the $title->exists() part. Then in getEditVars, instead of checking for $page === null, we can check $this->title->canExist() && $this->title->exists(). And finally remove that fallback from RunVariableGenerator::addEditVars.
Change 628075 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use a WikiPage object when filtering edits on a non-existing Title
Change 628075 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use a WikiPage object when filtering edits on a non-existing Title