From 6710e8c212ffba168d2ba32367863b8bf4b46e30 Mon Sep 17 00:00:00 2001 From: David Barratt Date: Wed, 30 Jan 2019 15:45:36 -0800 Subject: [PATCH] Prevent fatal PHP errors when PageRestriction::getTitle() returns null. Update usages of PageRestriction::getTitle() to handle a null response. Bug: T214763 Change-Id: Ied33e2c3c9442c47ae8084a97bb0921869fb9d49 --- includes/api/ApiQueryBlocks.php | 4 +++- includes/block/Restriction/PageRestriction.php | 16 +++++++++++++--- includes/specials/SpecialBlock.php | 4 +++- includes/specials/pagers/BlockListPager.php | 12 +++++++----- .../phpunit/includes/api/ApiQueryBlocksTest.php | 6 ++++++ .../block/Restriction/PageRestrictionTest.php | 5 +++++ .../includes/specials/SpecialBlockTest.php | 2 ++ .../specials/pagers/BlockListPagerTest.php | 4 +++- 8 files changed, 42 insertions(+), 11 deletions(-) diff --git a/includes/api/ApiQueryBlocks.php b/includes/api/ApiQueryBlocks.php index 95f8cda818..8aff2aa6b4 100644 --- a/includes/api/ApiQueryBlocks.php +++ b/includes/api/ApiQueryBlocks.php @@ -305,7 +305,9 @@ class ApiQueryBlocks extends ApiQueryBase { switch ( $restriction->getType() ) { case 'page': $value = [ 'id' => $restriction->getValue() ]; - self::addTitleInfo( $value, $restriction->getTitle() ); + if ( $restriction->getTitle() ) { + self::addTitleInfo( $value, $restriction->getTitle() ); + } break; default: $value = $restriction->getValue(); diff --git a/includes/block/Restriction/PageRestriction.php b/includes/block/Restriction/PageRestriction.php index bf7ef04ac0..5d3fabb386 100644 --- a/includes/block/Restriction/PageRestriction.php +++ b/includes/block/Restriction/PageRestriction.php @@ -35,7 +35,7 @@ class PageRestriction extends AbstractRestriction { const TYPE_ID = 1; /** - * @var \Title + * @var \Title|bool */ protected $title; @@ -43,6 +43,10 @@ class PageRestriction extends AbstractRestriction { * {@inheritdoc} */ public function matches( \Title $title ) { + if ( !$this->getTitle() ) { + return false; + } + return $title->equals( $this->getTitle() ); } @@ -66,11 +70,17 @@ class PageRestriction extends AbstractRestriction { * @return \Title|null */ public function getTitle() { - if ( !$this->title ) { + if ( $this->title === null ) { $this->title = \Title::newFromID( $this->value ); + + // If the title does not exist, set to false to prevent multiple database + // queries. + if ( $this->title === null ) { + $this->title = false; + } } - return $this->title; + return $this->title ?? null; } /** diff --git a/includes/specials/SpecialBlock.php b/includes/specials/SpecialBlock.php index bab3c8c5a3..287dbb31b0 100644 --- a/includes/specials/SpecialBlock.php +++ b/includes/specials/SpecialBlock.php @@ -406,7 +406,9 @@ class SpecialBlock extends FormSpecialPage { foreach ( $block->getRestrictions() as $restriction ) { switch ( $restriction->getType() ) { case PageRestriction::TYPE: - $pageRestrictions[] = $restriction->getTitle()->getPrefixedText(); + if ( $restriction->getTitle() ) { + $pageRestrictions[] = $restriction->getTitle()->getPrefixedText(); + } break; case NamespaceRestriction::TYPE: $namespaceRestrictions[] = $restriction->getValue(); diff --git a/includes/specials/pagers/BlockListPager.php b/includes/specials/pagers/BlockListPager.php index 8fc586bedd..69dce53cb6 100644 --- a/includes/specials/pagers/BlockListPager.php +++ b/includes/specials/pagers/BlockListPager.php @@ -262,11 +262,13 @@ class BlockListPager extends TablePager { switch ( $restriction->getType() ) { case PageRestriction::TYPE: - $items[$restriction->getType()][] = HTML::rawElement( - 'li', - [], - Linker::link( $restriction->getTitle() ) - ); + if ( $restriction->getTitle() ) { + $items[$restriction->getType()][] = HTML::rawElement( + 'li', + [], + Linker::link( $restriction->getTitle() ) + ); + } break; case NamespaceRestriction::TYPE: $text = $restriction->getValue() === NS_MAIN diff --git a/tests/phpunit/includes/api/ApiQueryBlocksTest.php b/tests/phpunit/includes/api/ApiQueryBlocksTest.php index 03198a8781..6e0084276f 100644 --- a/tests/phpunit/includes/api/ApiQueryBlocksTest.php +++ b/tests/phpunit/includes/api/ApiQueryBlocksTest.php @@ -112,6 +112,12 @@ class ApiQueryBlocksTest extends ApiTestCase { 'ir_type' => PageRestriction::TYPE_ID, 'ir_value' => $pageId, ] ); + // Page that has been deleted. + $this->db->insert( 'ipblocks_restrictions', [ + 'ir_ipb_id' => $block->getId(), + 'ir_type' => PageRestriction::TYPE_ID, + 'ir_value' => 999999, + ] ); $this->db->insert( 'ipblocks_restrictions', [ 'ir_ipb_id' => $block->getId(), 'ir_type' => NamespaceRestriction::TYPE_ID, diff --git a/tests/phpunit/includes/block/Restriction/PageRestrictionTest.php b/tests/phpunit/includes/block/Restriction/PageRestrictionTest.php index 95cb3b7b72..58822796ce 100644 --- a/tests/phpunit/includes/block/Restriction/PageRestrictionTest.php +++ b/tests/phpunit/includes/block/Restriction/PageRestrictionTest.php @@ -20,6 +20,11 @@ class PageRestrictionTest extends RestrictionTestCase { $page = $this->getExistingTestPage( 'Mars' ); $this->assertFalse( $restriction->matches( $page->getTitle() ) ); + + // Deleted page. + $restriction = new $class( 2, 99999 ); + $page = $this->getExistingTestPage( 'Saturn' ); + $this->assertFalse( $restriction->matches( $page->getTitle() ) ); } public function testGetType() { diff --git a/tests/phpunit/includes/specials/SpecialBlockTest.php b/tests/phpunit/includes/specials/SpecialBlockTest.php index 55a8b66e57..3d88bc2ad0 100644 --- a/tests/phpunit/includes/specials/SpecialBlockTest.php +++ b/tests/phpunit/includes/specials/SpecialBlockTest.php @@ -119,6 +119,8 @@ class SpecialBlockTest extends SpecialPageTestBase { new PageRestriction( 0, $pageSaturn->getId() ), new PageRestriction( 0, $pageMars->getId() ), new NamespaceRestriction( 0, NS_TALK ), + // Deleted page. + new PageRestriction( 0, 999999 ), ] ); $block->insert(); diff --git a/tests/phpunit/includes/specials/pagers/BlockListPagerTest.php b/tests/phpunit/includes/specials/pagers/BlockListPagerTest.php index 570291cd6e..6ffd03aace 100644 --- a/tests/phpunit/includes/specials/pagers/BlockListPagerTest.php +++ b/tests/phpunit/includes/specials/pagers/BlockListPagerTest.php @@ -140,7 +140,9 @@ class BlockListPagerTest extends MediaWikiTestCase { $restrictions = [ ( new PageRestriction( 0, $pageId ) )->setTitle( $title ), - new NamespaceRestriction( 0, NS_MAIN ) + new NamespaceRestriction( 0, NS_MAIN ), + // Deleted page. + new PageRestriction( 0, 999999 ), ]; $wrappedPager = TestingAccessWrapper::newFromObject( $pager ); -- 2.20.1