Page MenuHomePhabricator

CognateIntegrationTest::testCreateDeleteAndRestorePageResultsInEntry is failing
Closed, ResolvedPublic5 Estimated Story Points

Description

See eg empty patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cognate/+/695029

There was 1 failure:

1) Cognate\Tests\CognateIntegrationTest::testCreateDeleteAndRestorePageResultsInEntry
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 (
-    0 => 'wikidb-prefix:CognateIntegrationTest-pageName'
-)
+Array &0 ()

/workspace/src/extensions/Cognate/tests/phpunit/CognateIntegrationTest.php:126
/workspace/src/extensions/Cognate/tests/phpunit/CognateIntegrationTest.php:110
/workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:447

Acceptance criteria🏕️🌟:

  • Cognate CI is green for master
  • The test does not fail

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Lydia_Pintscher set the point value for this task to 5.
Lydia_Pintscher updated Other Assignee, added: toan.
toan added a subscriber: dang.

I unassigned @dang from this task as we have now both moved to the WBaaS "mini-hike" https://phabricator.wikimedia.org/project/board/5390/

We didn't make much progress except for the short hour today where we had a look at this together.

From what we could see CognatePageHookHandler::newRevisionRecordFromId does not seem to return the correct revision ID for that test, but didn't have time to investigate that any further.

I found a fix –

diff --git a/src/hooks/CognatePageHookHandler.php b/src/hooks/CognatePageHookHandler.php
index 01d2002..45b6392 100644
--- a/src/hooks/CognatePageHookHandler.php
+++ b/src/hooks/CognatePageHookHandler.php
@@ -153,7 +153,8 @@ function () use ( $dbName, $linkTarget ) {
 	public function onArticleUndelete(
 		Title $title
 	) {
-		$revision = $this->newRevisionRecordFromId( $title->getLatestRevID() );
+		$revisionId = $title->getLatestRevID( Title::READ_LATEST );
+		$revision = $this->newRevisionRecordFromId( $revisionId );
 		if ( !$this->isActionableTarget( $title ) || $revision == null ) {
 			return;
 		}

– but I’m still looking into why this is necessary. I feel like this shouldn’t be necessary on our end, and that MediaWiki core should ensure the title has an up-to-date latest revision ID before calling our hook.

Reverting the MediaWiki core change Rearrange PageArchive::undeleteRevisions to avoid race condition (linked to T271644) is also enough to make the test pass again.

This change in MediaWiki core also fixes the test:

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index b5b538a85b..f1a37d3fe7 100644
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -1508,10 +1508,11 @@ public function updateRevisionOn(
 
 		$result = $dbw->affectedRows() > 0;
 		if ( $result ) {
 			$this->updateRedirectOn( $dbw, $rt, $lastRevIsRedirect );
 			$this->setLastEdit( $revision );
+			$this->mTitle->resetArticleID( $this->getId() );
 			$this->mRedirectTarget = null;
 			$this->mHasRedirectTarget = null;
 			$this->mPageIsRedirectField = (bool)$rt;
 			$this->mIsNew = (bool)$isNew;
 			$this->mIsRedirect = (bool)$isRedirect;

The sequence of events seems to be:

  • The Title was just created. Its mLatestID is false, meaning “not loaded yet”.
  • PageArchive::undeleteRevisions() calls WikiPage::updateRevisionOn() to attach the latest revision to the page.
  • WikiPage::updateRevisionOn() loads its own page ID (article ID) so it can update that page row. (I think it even loads this from a replica DB?) This eventually causes the mLatestID of the Title to become 0, meaning “no latest revision”. At this point, that’s correct, because the latest revision hasn’t been attached to the page yet.
  • WikiPage::updateRevisionOn() proceeds to update the page table, connecting the latest revision and updating the page_latest column.
  • WikiPage::updateRevisionOn() updates its own metadata, and the link cache, with the new latest revision that it just wrote to the database. However, it does not update the metadata of the Title object.
  • PageArchive::undeleteRevisions() continues, and calls the hook with the same Title, which still contains the mLatestID = 0 even though this is now incorrect (the page_latest was updated).

I’m not quite sure where this should be fixed (the above patch works but probably isn’t quite right), but I think it’s indeed a MediaWiki bug, and nothing that we should work around in Cognate.

Change 700937 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] WikiPage: update mTitle in updateRevisionOn()

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

Change 700938 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Cognate@master] Remove backwards compatibility code

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

Change 700937 merged by jenkins-bot:

[mediawiki/core@master] WikiPage: update mTitle in updateRevisionOn()

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

Change 700938 merged by jenkins-bot:

[mediawiki/extensions/Cognate@master] Remove backwards compatibility code

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