jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/404757 )
Change subject: [MCR] RevisionStore::getTitle final logged fallback to master
......................................................................
[MCR] RevisionStore::getTitle final logged fallback to master
There have been many issues with RevisionStore and titles due
to code paths that already know the title for a Revision not
passing the title into Revision in various ways or not passing
in the correct queryFlags.
The getTitle method now has a further fallback using Title::newFromID
and Title::GAID_FOR_UPDATE if not already attempted.
Bug: T183548
Bug: T183716
Bug: T183717
Bug: T183550
Bug: T183505
Bug: T184559
Bug: T184595
Change-Id: I6cf13e6baba354b08533a6151bbbc88a317be9d6
(cherry picked from commit 9657e4db906f029ab2f89e127a83d75e6e2feff8)
---
M includes/ServiceWiring.php
M includes/Storage/RevisionStore.php
M tests/phpunit/includes/Storage/RevisionStoreTest.php
3 files changed, 51 insertions(+), 10 deletions(-)
Approvals:
Thcipriani: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 79e5b84..d6c1a64 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -476,6 +476,8 @@
$services->getMainWANObjectCache()
);
+ $store->setLogger( LoggerFactory::getInstance( 'RevisionStore'
) );
+
$config = $services->getMainConfig();
$store->setContentHandlerUseDB( $config->get(
'ContentHandlerUseDB' ) );
diff --git a/includes/Storage/RevisionStore.php
b/includes/Storage/RevisionStore.php
index f8481fe..e0de257 100644
--- a/includes/Storage/RevisionStore.php
+++ b/includes/Storage/RevisionStore.php
@@ -42,6 +42,9 @@
use Message;
use MWException;
use MWUnknownContentModelException;
+use Psr\Log\LoggerAwareInterface;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
use RecentChange;
use stdClass;
use Title;
@@ -61,7 +64,8 @@
* @note This was written to act as a drop-in replacement for the corresponding
* static methods in Revision.
*/
-class RevisionStore implements IDBAccessObject, RevisionFactory,
RevisionLookup {
+class RevisionStore
+ implements IDBAccessObject, RevisionFactory, RevisionLookup,
LoggerAwareInterface {
/**
* @var SqlBlobStore
@@ -89,6 +93,11 @@
private $cache;
/**
+ * @var LoggerInterface
+ */
+ private $logger;
+
+ /**
* @todo $blobStore should be allowed to be any BlobStore!
*
* @param LoadBalancer $loadBalancer
@@ -108,6 +117,11 @@
$this->blobStore = $blobStore;
$this->cache = $cache;
$this->wikiId = $wikiId;
+ $this->logger = new NullLogger();
+ }
+
+ public function setLogger( LoggerInterface $logger ) {
+ $this->logger = $logger;
}
/**
@@ -178,18 +192,23 @@
throw new InvalidArgumentException( '$pageId and $revId
cannot both be 0 or null' );
}
+ $canUseTitleNewFromId = ( $pageId !== null && $pageId > 0 &&
$this->wikiId === false );
list( $dbMode, $dbOptions, , ) =
DBAccessObjectUtils::getDBOptions( $queryFlags );
$titleFlags = $dbMode == DB_MASTER ? Title::GAID_FOR_UPDATE : 0;
- $title = null;
// Loading by ID is best, but Title::newFromID does not support
that for foreign IDs.
- if ( $pageId !== null && $pageId > 0 && $this->wikiId === false
) {
+ if ( $canUseTitleNewFromId ) {
// TODO: better foreign title handling (introduce
TitleFactory)
$title = Title::newFromID( $pageId, $titleFlags );
+ if ( $title ) {
+ return $title;
+ }
}
// rev_id is defined as NOT NULL, but this revision may not yet
have been inserted.
- if ( !$title && $revId !== null && $revId > 0 ) {
+ $canUseRevId = ( $revId !== null && $revId > 0 );
+
+ if ( $canUseRevId ) {
$dbr = $this->getDbConnectionRef( $dbMode );
// @todo: Title::getSelectFields(), or
Title::getQueryInfo(), or something like that
$row = $dbr->selectRow(
@@ -209,17 +228,26 @@
);
if ( $row ) {
// TODO: better foreign title handling
(introduce TitleFactory)
- $title = Title::newFromRow( $row );
+ return Title::newFromRow( $row );
}
}
- if ( !$title ) {
- throw new RevisionAccessException(
- "Could not determine title for page ID $pageId
and revision ID $revId"
- );
+ // If we still don't have a title use Title::GAID_FOR_UPDATE
and try one last time
+ if ( $canUseTitleNewFromId && $titleFlags !==
Title::GAID_FOR_UPDATE ) {
+ // TODO: better foreign title handling (introduce
TitleFactory)
+ $title = Title::newFromID( $pageId,
Title::GAID_FOR_UPDATE );
+ if ( $title ) {
+ $this->logger->info(
+ __METHOD__ . ' fell back to
Title::GAID_FOR_UPDATE and got a Title.',
+ [ 'trace' => wfDebugBacktrace() ]
+ );
+ return $title;
+ }
}
- return $title;
+ throw new RevisionAccessException(
+ "Could not determine title for page ID $pageId and
revision ID $revId"
+ );
}
/**
diff --git a/tests/phpunit/includes/Storage/RevisionStoreTest.php
b/tests/phpunit/includes/Storage/RevisionStoreTest.php
index c9e9978..c8deaf4 100644
--- a/tests/phpunit/includes/Storage/RevisionStoreTest.php
+++ b/tests/phpunit/includes/Storage/RevisionStoreTest.php
@@ -381,6 +381,7 @@
}
/**
+ * @group Addshore
* @covers \MediaWiki\Storage\RevisionStore::getTitle
*/
public function testGetTitle_throwsExceptionAfterFallbacks() {
@@ -418,6 +419,16 @@
)
->willReturn( false );
+ // Fallback call
+ $db->expects( $this->at( 2 ) )
+ ->method( 'selectRow' )
+ ->with(
+ 'page',
+ $this->anything(),
+ [ 'page_id' => 1 ]
+ )
+ ->willReturn( false );
+
$store = $this->getRevisionStore( $mockLoadBalancer );
$this->setExpectedException( RevisionAccessException::class );
--
To view, visit https://gerrit.wikimedia.org/r/404757
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6cf13e6baba354b08533a6151bbbc88a317be9d6
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/core
Gerrit-Branch: wmf/1.31.0-wmf.17
Gerrit-Owner: Addshore <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Thcipriani <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
