Page MenuHomePhabricator

afl_rev_id is not populated for new pages
Closed, ResolvedPublic

Description

During local test for T261630, I noticed that the afl_rev_id field (used to generate "diff" links on Special:AbuseLog) is not populated when a page is created. For subsequent edits, it works. The responsible method is AbuseFilterHooks::onPageSaveComplete.

This blocks T62179.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 17 2020, 9:08 AM

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:

AbuseFilterHooks::filterEdit
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.

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 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:

RunVariableGenerator::getEditVars
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.

RunVariableGenerator::newVariableHolderForEdit
$this->addEditVars( $this->title, $page );

so $page is passed through to addEditVars, let's see:

RunVariableGenerator::addEditVars
// 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

https://gerrit.wikimedia.org/r/628075

Daimona claimed this task.Sep 17 2020, 11:59 AM

Change 628075 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use a WikiPage object when filtering edits on a non-existing Title

https://gerrit.wikimedia.org/r/628075

Daimona closed this task as Resolved.Nov 18 2020, 8:59 PM
Daimona removed a project: Patch-For-Review.