Page MenuHomePhabricator

Make RevisionRecord return a ProperPageIdentity
Open, LowPublic

Description

Anything that has a revision is a "proper" page by definition,
so RevisionRecord::getPage() should return a ProperPageIdentity.

Event Timeline

Change 666136 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Make RevisionRecord return a ProperPageIdentity

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

Change 665661 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Make RevisionRecord require a ProperPageIdentity

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

Change 666519 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Deprecate constructing revision with non-proper page

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

daniel lowered the priority of this task from High to Medium.Mar 9 2021, 5:50 PM

Change 666519 merged by jenkins-bot:
[mediawiki/core@master] Deprecate constructing revision with non-proper page

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

Change 672462 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] RevisionStore: getRevisionbyId should take a PageIdentity

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

Change 672499 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] RevisionStore: create getPage, deprecate getTitle

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

As https://gerrit.wikimedia.org/r/672462 and https://gerrit.wikimedia.org/r/672499 do not seem to be finished, and the code change in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/666519 causes issues in Wikibase (T277362), WMDE would be inclined to revert https://gerrit.wikimedia.org/r/c/mediawiki/core/+/666519. If getting the proper fix (tm) is just minutes away and you're actively getting those fixes ready and merged @daniel and team, we'd hold with reverting, but would like to have a clarity on the status.

Change 672810 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Revert "Deprecate constructing revision with non-proper page"

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

Change 672811 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@wmf/1.36.0-wmf.35] Revert "Deprecate constructing revision with non-proper page"

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

Change 672810 merged by jenkins-bot:
[mediawiki/core@master] Revert "Deprecate constructing revision with non-proper page"

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

Change 672811 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.35] Revert "Deprecate constructing revision with non-proper page"

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

Mentioned in SAL (#wikimedia-operations) [2021-03-16T23:56:32Z] <krinkle@deploy1002> Synchronized php-1.36.0-wmf.35/includes/Revision/: I8619ab9e92b, T277362, T275531 (duration: 00m 58s)

Change 672819 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Re-apply "Deprecate constructing revision with non-proper page"

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

Change 672499 merged by jenkins-bot:
[mediawiki/core@master] RevisionStore: create getPage, deprecate getTitle

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

Change 672462 merged by jenkins-bot:
[mediawiki/core@master] RevisionStore: getRevisionbyId should take a PageIdentity

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

Change 672819 merged by jenkins-bot:
[mediawiki/core@master] Re-apply "Deprecate constructing revision with non-proper page"

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

@daniel I was in looking at the production errors with @hashar today and it looks that the problem still happens - not very often though

See the stacktrace below:

from /srv/mediawiki/php-1.36.0-wmf.36/includes/user/UserIdentityValue.php(98)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, string, array)
#1 /srv/mediawiki/php-1.36.0-wmf.36/includes/debug/MWDebug.php(376): trigger_error(string, integer)
#2 /srv/mediawiki/php-1.36.0-wmf.36/includes/debug/MWDebug.php(352): MWDebug::sendRawDeprecated(string, boolean, string)
#3 /srv/mediawiki/php-1.36.0-wmf.36/includes/GlobalFunctions.php(1068): MWDebug::deprecatedMsg(string, string, string, integer)
#4 /srv/mediawiki/php-1.36.0-wmf.36/includes/dao/WikiAwareEntityTrait.php(78): wfDeprecatedMsg(string, string)
#5 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/UserIdentityValue.php(98): MediaWiki\User\UserIdentityValue->deprecateInvalidCrossWiki(string, string)
#6 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/ActorStore.php(489): MediaWiki\User\UserIdentityValue->getId(string)
#7 /srv/mediawiki/php-1.36.0-wmf.36/includes/ActorMigration.php(319): MediaWiki\User\ActorStore->acquireActorId(MediaWiki\User\UserIdentityValue, Wikimedia\Rdbms\DBConnRef)
#8 /srv/mediawiki/php-1.36.0-wmf.36/includes/block/DatabaseBlockStore.php(379): ActorMigration->getInsertValues(Wikimedia\Rdbms\DBConnRef, string, MediaWiki\User\UserIdentityValue)
#9 /srv/mediawiki/php-1.36.0-wmf.36/includes/block/DatabaseBlockStore.php(165): MediaWiki\Block\DatabaseBlockStore->getArrayForDatabaseBlock(MediaWiki\Block\DatabaseBlock, Wikimedia\Rdbms\DBConnRef)
#10 /srv/mediawiki/php-1.36.0-wmf.36/includes/block/DatabaseBlock.php(524): MediaWiki\Block\DatabaseBlockStore->insertBlock(MediaWiki\Block\DatabaseBlock, Wikimedia\Rdbms\DBConnRef)
#11 /srv/mediawiki/php-1.36.0-wmf.36/extensions/CentralAuth/includes/CentralAuthUser.php(1950): MediaWiki\Block\DatabaseBlock->insert(Wikimedia\Rdbms\DBConnRef)
#12 /srv/mediawiki/php-1.36.0-wmf.36/extensions/CentralAuth/includes/CentralAuthUser.php(1881): CentralAuthUser->doLocalSuppression(boolean, string, string, string)
#13 /srv/mediawiki/php-1.36.0-wmf.36/extensions/CentralAuth/includes/CentralAuthUser.php(1859): CentralAuthUser->doCrosswikiSuppression(boolean, string, string)
#14 /srv/mediawiki/php-1.36.0-wmf.36/extensions/CentralAuth/includes/CentralAuthUser.php(1810): CentralAuthUser->suppress(string, string)
#15 /srv/mediawiki/php-1.36.0-wmf.36/extensions/CentralAuth/includes/specials/SpecialCentralAuth.php(245): CentralAuthUser->adminLockHide(boolean, string, string, RequestContext)
#16 /srv/mediawiki/php-1.36.0-wmf.36/extensions/CentralAuth/includes/specials/SpecialCentralAuth.php(143): SpecialCentralAuth->doSubmit()
#17 /srv/mediawiki/php-1.36.0-wmf.36/includes/specialpage/SpecialPage.php(646): SpecialCentralAuth->execute(NULL)
#18 /srv/mediawiki/php-1.36.0-wmf.36/includes/specialpage/SpecialPageFactory.php(1382): SpecialPage->run(NULL)
#19 /srv/mediawiki/php-1.36.0-wmf.36/includes/MediaWiki.php(309): MediaWiki\SpecialPage\SpecialPageFactory->executePath(Title, RequestContext)
#20 /srv/mediawiki/php-1.36.0-wmf.36/includes/MediaWiki.php(925): MediaWiki->performRequest()
#21 /srv/mediawiki/php-1.36.0-wmf.36/includes/MediaWiki.php(547): MediaWiki->main()
#22 /srv/mediawiki/php-1.36.0-wmf.36/index.php(53): MediaWiki->run()
#23 /srv/mediawiki/php-1.36.0-wmf.36/index.php(46): wfIndexMain()
#24 /srv/mediawiki/w/index.php(3): require(string)
#25 {main}

See: https://logstash.wikimedia.org/goto/e7fff370b4f79f729b1410d1a24f1a16
reqId: 0d095dc2-9959-498b-bdff-4f16aa3cf231

@daniel I was in looking at the production errors with @hashar today and it looks that the problem still happens - not very often though

That indeed looks like an issue, but unrelated to this ticket. It has nothing to do with PageRecord or ProperPageIdentity. It has to do with a UserIdentity object being used in the context of the wrong wiki. I'll file a ticket.

Apologies for the noise, got tricked by them identities!

Apologies for the noise, got tricked by them identities!

Not at all, thanks for noticing! Looks like we have a ticket for it already: T277687: Deprecated cross-wiki access to User. Expected: 'eswiki', Actual: the local wiki. Pass expected $wikiId. [Called from User::getId]

daniel lowered the priority of this task from Medium to Low.Oct 20 2022, 8:26 AM

@daniel: Per emails from Sep18 and Oct20 and https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup , I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!). Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task. Please claim this task again when you plan to work on it (via Add Action...Assign / Claim in the dropdown menu) - it would be welcome. Thanks for your understanding!