From 6a3b9daf293d9ecc5141d16de938cd69bcd7a116 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Mon, 30 Aug 2021 14:42:57 +0200 Subject: [PATCH] Make WikiPage a ProperPageIdentity Bug: T272424 Change-Id: I1dbcf1192390cbdfd6faaa60f9831f62ec1eff0d --- RELEASE-NOTES-1.37 | 3 + includes/Storage/PageUpdaterFactory.php | 6 -- includes/page/DeletePage.php | 9 +-- includes/page/ExistingPageRecord.php | 4 +- includes/page/PageRecord.php | 5 +- includes/page/WikiPage.php | 58 +------------------ includes/page/WikiPageFactory.php | 1 + .../phpunit/includes/page/WikiPageDbTest.php | 14 +---- 8 files changed, 13 insertions(+), 87 deletions(-) diff --git a/RELEASE-NOTES-1.37 b/RELEASE-NOTES-1.37 index 5f0b90841bc..72cc0e5753f 100644 --- a/RELEASE-NOTES-1.37 +++ b/RELEASE-NOTES-1.37 @@ -413,6 +413,9 @@ because of Phabricator reports. * Content::preloadTransform was hard-deprecated since 1.37, use ContentTransformer::preloadTransform instead. Extensions defining a content model should override ContentHandler::preloadTransform. +* Constructing WikiPage objects from Title instances that cannot exist, + hard-deprecated in 1.36, now throws an exception. Additionally, WikiPage + now implements ProperPageIdentity, rather than just PageIdentity. * Content::preSaveTransform was hard-deprecated since 1.37, use ContentTransformer::preSaveTransform instead. Extensions defining a content model should override ContentHandler::preSaveTransform. diff --git a/includes/Storage/PageUpdaterFactory.php b/includes/Storage/PageUpdaterFactory.php index 553ddf34dad..75dc8bf448e 100644 --- a/includes/Storage/PageUpdaterFactory.php +++ b/includes/Storage/PageUpdaterFactory.php @@ -42,7 +42,6 @@ use ParserCache; use Psr\Log\LoggerInterface; use TitleFormatter; use WANObjectCache; -use Wikimedia\Assert\Assert; use Wikimedia\Rdbms\ILBFactory; use WikiPage; @@ -258,11 +257,6 @@ class PageUpdaterFactory { UserIdentity $user, DerivedPageDataUpdater $derivedPageDataUpdater ): PageUpdater { - Assert::precondition( - $page->canExist(), - 'The WikiPage instance does not represent a proper page!' - ); - $pageUpdater = new PageUpdater( $user, $page, // NOTE: eventually, PageUpdater should not know about WikiPage diff --git a/includes/page/DeletePage.php b/includes/page/DeletePage.php index 8c295d629f1..ab8931ee87e 100644 --- a/includes/page/DeletePage.php +++ b/includes/page/DeletePage.php @@ -2,7 +2,6 @@ namespace MediaWiki\Page; -use BadMethodCallException; use CommentStore; use Content; use DeferrableUpdate; @@ -63,14 +62,10 @@ class DeletePage { private $deleter; /** - * @param PageIdentity $page + * @param ProperPageIdentity $page * @param UserIdentity $deleter - * @todo Should use ProperPageIdentity eventually, once WikiPage will extend that */ - public function __construct( PageIdentity $page, UserIdentity $deleter ) { - if ( !$page->canExist() ) { - throw new BadMethodCallException( __METHOD__ . ' requires a proper page identity.' ); - } + public function __construct( ProperPageIdentity $page, UserIdentity $deleter ) { $services = MediaWikiServices::getInstance(); $this->hookRunner = new HookRunner( $services->getHookContainer() ); $this->revisionStore = $services->getRevisionStore(); diff --git a/includes/page/ExistingPageRecord.php b/includes/page/ExistingPageRecord.php index 27de7b75730..0e1c1f2be41 100644 --- a/includes/page/ExistingPageRecord.php +++ b/includes/page/ExistingPageRecord.php @@ -12,12 +12,12 @@ namespace MediaWiki\Page; * * @since 1.36 */ -interface ExistingPageRecord extends PageRecord, ProperPageIdentity { +interface ExistingPageRecord extends PageRecord { /** * Always true. * - * @return bool + * @return true */ public function exists(): bool; } diff --git a/includes/page/PageRecord.php b/includes/page/PageRecord.php index 8638de49829..72e7d3f292c 100644 --- a/includes/page/PageRecord.php +++ b/includes/page/PageRecord.php @@ -18,14 +18,11 @@ namespace MediaWiki\Page; * ExistingPageRecord instead. Once WikiPage is removed or guaranteed to be immutable and * existing, ExistingPageRecord will become an alias of PageRecord. * - * @todo In the future, PageRecord should extend ProperPageIdentity. This will only - * become possible when WikiPage can no longer represent non-proper pages. - * * @stable to type * * @since 1.36 */ -interface PageRecord extends PageIdentity { +interface PageRecord extends ProperPageIdentity { /** * False if the page has had more than one edit. diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index ebfbc15b45d..08bbf868aea 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -173,13 +173,7 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { // TODO: remove the need for casting to Title. $title = Title::castFromPageIdentity( $pageIdentity ); if ( !$title->canExist() ) { - // TODO: In order to allow WikiPage to implement ProperPageIdentity, - // throw here to prevent construction of a WikiPage that doesn't - // represent a proper page. - wfDeprecatedMsg( - "WikiPage constructed on a Title that cannot exist as a page: $title", - '1.36' - ); + throw new InvalidArgumentException( "WikiPage constructed on a Title that cannot exist as a page: $title" ); } $this->mTitle = $title; @@ -471,18 +465,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { * @return void */ public function loadPageData( $from = 'fromdb' ) { - if ( !$this->mTitle->canExist() ) { - // NOTE: If and when WikiPage implements PageIdentity but not yet ProperPageIdentity, - // throw here to prevent usage of a WikiPage that doesn't - // represent a proper page. - // NOTE: The constructor will already have triggered a warning, but seeing how - // bad instances of WikiPage are used will be helpful. - wfDeprecatedMsg( - "Accessing WikiPage that cannot exist as a page: {$this->mTitle}. ", - '1.36' - ); - } - $from = self::convertSelectType( $from ); if ( is_int( $from ) && $from <= $this->mDataLoadedFrom ) { // We already have the data from the correct location, no need to load it twice. @@ -595,24 +577,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { $this->mDataLoadedFrom = self::convertSelectType( $from ); } - /** - * Code that requires this WikiPage to be a "proper page" in the sense - * defined by PageIdentity should call this method. - * - * @note In the future, this method should become redundant, as the - * constructor should not allow a WikiPage to be constructed for as title - * that does not represent a proper page. For the time being, we allow - * such instances for backwards compatibility. - * - * @throws PreconditionException - */ - private function assertProperPage() { - Assert::precondition( - $this->mTitle->canExist(), - 'This WikiPage instance does not represent a proper page!' - ); - } - /** * @param string|false $wikiId * @@ -620,7 +584,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { */ public function getId( $wikiId = self::LOCAL ): int { $this->assertWiki( $wikiId ); - $this->assertProperPage(); if ( !$this->mDataLoaded ) { $this->loadPageData(); @@ -805,18 +768,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { return; // already loaded } - if ( !$this->mTitle->canExist() ) { - // NOTE: If and when WikiPage implements PageIdentity but not yet ProperPageIdentity, - // throw here to prevent usage of a WikiPage that doesn't - // represent a proper page. - // NOTE: The constructor will already have triggered a warning, but seeing how - // bad instances of WikiPage are used will be helpful. - wfDeprecatedMsg( - "Accessing WikiPage that cannot exist as a page: {$this->mTitle}. ", - '1.36' - ); - } - $latest = $this->getLatest(); if ( !$latest ) { return; // page doesn't exist or is missing page_latest info @@ -1410,8 +1361,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { * page ID is already in use. */ public function insertOn( $dbw, $pageId = null ) { - $this->assertProperPage(); - $pageIdForInsert = $pageId ? [ 'page_id' => $pageId ] : []; $dbw->insert( 'page', @@ -2270,8 +2219,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { ) { global $wgCascadingRestrictionLevels; - $this->assertProperPage(); - if ( wfReadOnly() ) { return Status::newFatal( wfMessage( 'readonlytext', wfReadOnlyReason() ) ); } @@ -3336,8 +3283,7 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { * @since 1.36 */ public function canExist(): bool { - // NOTE: once WikiPage becomes a ProperPageIdentity, this should always return true - return $this->mTitle->canExist(); + return true; } /** diff --git a/includes/page/WikiPageFactory.php b/includes/page/WikiPageFactory.php index ca61bf1d449..ea06221b27b 100644 --- a/includes/page/WikiPageFactory.php +++ b/includes/page/WikiPageFactory.php @@ -56,6 +56,7 @@ class WikiPageFactory { } if ( !$pageIdentity->canExist() ) { + // BC with the Title class throw new InvalidArgumentException( "The given PageIdentity does not represent a proper page" ); diff --git a/tests/phpunit/includes/page/WikiPageDbTest.php b/tests/phpunit/includes/page/WikiPageDbTest.php index a3abf52a627..f59084d39d8 100644 --- a/tests/phpunit/includes/page/WikiPageDbTest.php +++ b/tests/phpunit/includes/page/WikiPageDbTest.php @@ -146,18 +146,8 @@ class WikiPageDbTest extends MediaWikiLangTestCase { */ public function testConstructionWithPageThatCannotExist( $ns, $text ) { $title = Title::makeTitle( $ns, $text ); - - // NOTE: once WikiPage becomes a ProperPageIdentity, the constructor should throw! - $this->filterDeprecated( '/WikiPage constructed on a Title that cannot exist as a page/' ); - $this->filterDeprecated( '/Accessing WikiPage that cannot exist as a page/' ); - $page = new WikiPage( $title ); - - $this->assertFalse( $page->canExist() ); - $this->assertFalse( $page->exists() ); - $this->assertNull( $page->getRevisionRecord() ); - - $this->expectException( RuntimeException::class ); - $page->getId(); + $this->expectException( InvalidArgumentException::class ); + new WikiPage( $title ); } /**