Page MenuHomePhabricator

Deprecation warning: Expected RevisionRecord to belong to ...
Closed, ResolvedPublic

Description

In the wikibase release pipeline we've added some tests that check that the client repo setup works correctly (referencing items, dispatching changes etc)

These tests started failing once T272485 was merged into master.

Interestingly the tests didn't fail in any catastrophic way but any requests that was made from selenium were just left hanging without any response. I tried using the UI(wdio) & the API (wdio-mediawiki) in the tests but with the same result.

A part from the tests failing a new deprecation warning also started showing under certain circumstances like the one showed below.

borked.jpeg (231×1 px, 86 KB)

I was able to make the tests pass locally by applying a patch like this but I suspect there may be many more cases where this could occur as RevisionRecord::getId is called all over the place.

Steps to reproduce:

I've followed this guide to setup client & repo

Repo:

  1. create an item Q1
  2. create a property P1

Client:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
toan updated the task description. (Show Details)

Given the fact that this seemingly breaks things for us locally this should probably block next weeks train which is when the core patch would be rolled out until we can figure out what is happening and why.
T271343: 1.36.0-wmf.29 deployment blockers

Addshore triaged this task as High priority.Feb 2 2021, 1:57 PM
Addshore moved this task from Incoming to Active on the wdwb-tech (legacy-backlog) board.
Addshore added a subscriber: Lucas_Werkmeister_WMDE.

I’m confused by what happened in RevisionRecord: make RevisionRecord::getId() take a wiki ID. It seems to have added an optional parameter to the getId() method, but then also immediately hard deprecated calling getId() without that optional parameter: getId() calls wfDeprecatedMsg() if the $wikiId parameter (defaults to self::LOCAL) is not the same as $this->getWikiId(), so for a RevisionRecord belonging to a non-local wiki, calling getId() without arguments will now trigger a call to wfDeprecatedMsg(). But wfDeprecatedMsg() is apparently enough to make unit tests fail, and also cause warnings to be displayed in the page output, so I assume it counts as hard deprecation. Shouldn’t there have been a period of soft deprecation in between?

CCing some of the people who worked on T272485: Make RevisionRecord::getId() take a wiki ID and also marking this as a train blocker until this is cleared up a bit more.

DannyS712 raised the priority of this task from High to Unbreak Now!.Feb 2 2021, 3:36 PM

I was able to make the tests pass locally by applying a patch like this but I suspect there may be many more cases where this could occur as RevisionRecord::getId is called all over the place.

I think that class (WikiPageEntityDataLoader) might be the only affected caller. There are other callers, but all the ones I found so far are in Repo and presumably only deal with local revisions:

$ git grep -P '[rR]ev(?:ision)?(?:Record)?->getId\(\)'
lib/includes/Store/Sql/WikiPageEntityDataLoader.php:                            $revision->getId(),
lib/includes/Store/Sql/WikiPageEntityDataLoader.php:                                    'The serialized data of revision ' . $revision->getId()
repo/includes/Actions/EditEntityAction.php:             if ( $olderRevision->getId() == $newerRevision->getId() ) {
repo/includes/Actions/EditEntityAction.php:                     return Status::newFatal( 'wikibase-undo-badpage', $this->getTitle(), $newerRevision->getId() );
repo/includes/Actions/EditEntityAction.php:                     return Status::newFatal( 'wikibase-undo-badpage', $this->getTitle(), $olderRevision->getId() );
repo/includes/Actions/EditEntityAction.php:                     return Status::newFatal( 'wikibase-undo-nocontent', $this->getTitle(), $olderRevision->getId() );
repo/includes/Actions/EditEntityAction.php:                     return Status::newFatal( 'wikibase-undo-nocontent', $this->getTitle(), $newerRevision->getId() );
repo/includes/Actions/EditEntityAction.php:                     return Status::newFatal( 'wikibase-undo-nocontent', $this->getTitle(), $latestRevision->getId() );
repo/includes/Actions/EditEntityAction.php:                             $olderRevision->getId(),
repo/includes/Actions/EditEntityAction.php:                             $newerRevision->getId()
repo/includes/Actions/EditEntityAction.php:             if ( $newerRevision->getId() == $latestRevision->getId() ) {
repo/includes/Actions/EditEntityAction.php:                     $this->showConfirmationForm( $newerRevision->getId() );
repo/includes/Actions/SubmitEntityAction.php:           $summary->addAutoCommentArgs( $revision->getId(), $revUserText );
repo/includes/Diff/EntityContentDiffView.php:                   [ 'oldid' => $rev->getId() ] );
repo/includes/Diff/EntityContentDiffView.php:                                   'restore' => $rev->getId()
repo/includes/Diff/EntityContentDiffView.php:                   $parserOutput = $page->getParserOutput( $parserOptions, $rev->getId() );
repo/includes/Hooks/ArticleRevisionVisibilitySetHookHandler.php:                        $revision->getId()
repo/includes/Notifications/RepoEntityChange.php:                       'revision_id' => $revision->getId(),
repo/includes/Notifications/RepoEntityChange.php:                       'rev_id' => $revision->getId(),
repo/includes/RepoHooks.php:                            $revision->getId()
repo/includes/RepoHooks.php:                            $revision->getId(),
repo/includes/RepoHooks.php:                    && $wikiPage->getLatest() !== $revisionRecord->getId()
repo/includes/RepoHooks.php:                                    'restore' => $revisionRecord->getId()
repo/includes/Store/Sql/WikiPageEntityStore.php:                        $revision->getId(),
repo/includes/Store/Sql/WikiPageEntityStore.php:                $this->dispatcher->dispatch( 'redirectUpdated', $redirect, $revision->getId() );
repo/includes/Store/Sql/WikiPageEntityStore.php:                return $revision->getId();
repo/tests/phpunit/includes/Actions/EditEntityActionTest.php:           $params[ $key ] = $rev->getId();

I was able to make the tests pass locally by applying a patch like this but I suspect there may be many more cases where this could occur as RevisionRecord::getId is called all over the place.

I think that class (WikiPageEntityDataLoader) might be the only affected caller. There are other callers, but all the ones I found so far are in Repo and presumably only deal with local revisions:

That sound positive and that merging the change should thus unblock the train for us.

Change 661114 had a related patch set uploaded (by Addshore; owner: Tobias Andersson):
[mediawiki/extensions/Wikibase@master] Update RevisionRecord::getId usage

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

Change 661136 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Add wiki ID to WikiPageEntityDataLoader

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

Change 661137 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseMediaInfo@master] Pass $databaseName into WikiPageEntityDataLoader

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

This is also happening in production: Logstash has 11 occurrences of

[{reqId}] {exception_url} ErrorException: Expected RevisionRecord to belong to the local wiki, but it belongs to 'testwikidatawiki' [Called from MediaWiki\Revision\RevisionStoreRecord::getId]

in the last 1 hour (9 testwikidatawiki, 2 testwiki).

Change 661136 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add wiki ID to WikiPageEntityDataLoader

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

Change 661137 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Pass $databaseName into WikiPageEntityDataLoader

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

I'll make some cherry picks now so that CI can run on them before they are backported.

Change 661091 had a related patch set uploaded (by Addshore; owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@wmf/1.36.0-wmf.29] Add wiki ID to WikiPageEntityDataLoader

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

Change 661092 had a related patch set uploaded (by Addshore; owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseMediaInfo@wmf/1.36.0-wmf.29] Pass $databaseName into WikiPageEntityDataLoader

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

I guess we "just" need to backport those two merged changes to 1.36.0-wmf.29 ? If so please cherry pick to wmf/1.36.0-wmf.29 and either sync them or I can do it.

1.36.0-wmf.29 is still blocked on other changes though so I am most probably not going to push it to group 1 on Wednesday european afternoon but maybe in the evening.

Change 661114 abandoned by Addshore:
[mediawiki/extensions/Wikibase@master] Update RevisionRecord::getId usage

Reason:
Replaced by other patches in https://phabricator.wikimedia.org/T273622

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

I guess we "just" need to backport those two merged changes to 1.36.0-wmf.29 ? If so please cherry pick to wmf/1.36.0-wmf.29 and either sync them or I can do it.

1.36.0-wmf.29 is still blocked on other changes though so I am most probably not going to push it to group 1 on Wednesday european afternoon but maybe in the evening.

Seems like we were having the same thoughts at the same time!
There is a Depends-On relation between the 2 patches.
The wikibase one should be deployed first, or they should be deployed at the same time.
(less important as we are mainly on test wikis right now)

Yeah, I’ll deploy the backports now (Morning backports window, no other patches as far as I can see).

Change 661091 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@wmf/1.36.0-wmf.29] Add wiki ID to WikiPageEntityDataLoader

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

Change 661092 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@wmf/1.36.0-wmf.29] Pass $databaseName into WikiPageEntityDataLoader

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

Mentioned in SAL (#wikimedia-operations) [2021-02-02T19:57:14Z] <lucaswerkmeister-wmde@deploy1001> Synchronized php-1.36.0-wmf.29/extensions/Wikibase/: Backport: [[gerrit:661091|Add wiki ID to WikiPageEntityDataLoader (T273622)]] (duration: 01m 25s)

Mentioned in SAL (#wikimedia-operations) [2021-02-02T19:58:46Z] <lucaswerkmeister-wmde@deploy1001> Synchronized php-1.36.0-wmf.29/extensions/WikibaseMediaInfo/: Backport: [[gerrit:661092|Pass $databaseName into WikiPageEntityDataLoader (T273622)]] (duration: 01m 07s)

so for a RevisionRecord belonging to a non-local wiki, calling getId() without arguments will now trigger a call to wfDeprecatedMsg(). But wfDeprecatedMsg() is apparently enough to make unit tests fail, and also cause warnings to be displayed in the page output, so I assume it counts as hard deprecation. Shouldn’t there have been a period of soft deprecation in between?

Soft deprecation is for the period between deciding that some code or behavior should be removed, and removing all known callers. I didn't spot the callers in Wikibase code, though I suspected they exist. We decided to add the deprecation warning to find any remaining callers. I told Adam to look out for that during our lunch meeting last week.

I'm sorry for causing you work. Unfortunately, there didn't seem to be a good way to flush these issues out beforehand - afterall, Wikibase CI tests passed.

We'll take another look at extensions that use RevisionStoreFactory, to avoid this kind of thing: T273667: Survey extensions for cross-wiki RevisionStore usage

I had dinner, come back and magic the task is closed. Thank you!