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

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.

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();

In T273622#6796443, @Lucas_Werkmeister_WMDE wrote:

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

In T273622#6797282, @hashar wrote:

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)

@Lucas_Werkmeister_WMDE wrote:

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!