From bf9a7cad616a905e66f6a4db8593468bb37a1570 Mon Sep 17 00:00:00 2001 From: David Causse Date: Thu, 1 Aug 2019 22:38:46 +0200 Subject: [PATCH] Hard deprecate new SearchResult() and introduce RevisionSearchResult Transitional step for the transformation of SearchResult into an abstract base class: - RevisionSearchResult is introduced to behave like SearchResult - methods are currently shared between RevisionSearchResult and SearchResult in the RevisionSearchResultTrait Bug: T228626 Change-Id: I13d132de50f6c66086b7f9055d036f2e76667b27 --- RELEASE-NOTES-1.34 | 4 + autoload.php | 2 + includes/search/RevisionSearchResult.php | 18 ++ includes/search/RevisionSearchResultTrait.php | 200 ++++++++++++++++ includes/search/SearchResult.php | 213 ++---------------- includes/search/SqlSearchResult.php | 4 +- .../includes/api/ApiQuerySearchTest.php | 2 +- .../phpunit/mocks/search/MockSearchResult.php | 2 +- 8 files changed, 246 insertions(+), 199 deletions(-) create mode 100644 includes/search/RevisionSearchResult.php create mode 100644 includes/search/RevisionSearchResultTrait.php diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 620a6f51de..73a1f7b8ad 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -463,6 +463,10 @@ because of Phabricator reports. * Constructing MovePage directly is deprecated. Use MovePageFactory. * TempFSFile::factory() has been deprecated. Use TempFSFileFactory instead. * wfIsBadImage() is deprecated. Use the BadFileLookup service instead. +* Building a new SearchResult is hard-deprecated, always call + SearchResult::newFromTitle(). This class is being refactored into an abstract + class. If you extend this class please be sure to override all its methods + or extend RevisionSearchResult. === Other changes in 1.34 === * … diff --git a/autoload.php b/autoload.php index 95297fbc64..57e46e9199 100644 --- a/autoload.php +++ b/autoload.php @@ -1285,6 +1285,8 @@ $wgAutoloadLocalClasses = [ 'RevisionItemBase' => __DIR__ . '/includes/revisionlist/RevisionItemBase.php', 'RevisionList' => __DIR__ . '/includes/revisionlist/RevisionList.php', 'RevisionListBase' => __DIR__ . '/includes/revisionlist/RevisionListBase.php', + 'RevisionSearchResult' => __DIR__ . '/includes/search/RevisionSearchResult.php', + 'RevisionSearchResultTrait' => __DIR__ . '/includes/search/RevisionSearchResultTrait.php', 'RiffExtractor' => __DIR__ . '/includes/libs/RiffExtractor.php', 'RightsLogFormatter' => __DIR__ . '/includes/logging/RightsLogFormatter.php', 'RollbackAction' => __DIR__ . '/includes/actions/RollbackAction.php', diff --git a/includes/search/RevisionSearchResult.php b/includes/search/RevisionSearchResult.php new file mode 100644 index 0000000000..f94ea2a1f8 --- /dev/null +++ b/includes/search/RevisionSearchResult.php @@ -0,0 +1,18 @@ +mTitle = $title; + $this->initFromTitle( $title ); + } +} diff --git a/includes/search/RevisionSearchResultTrait.php b/includes/search/RevisionSearchResultTrait.php new file mode 100644 index 0000000000..24370c3d25 --- /dev/null +++ b/includes/search/RevisionSearchResultTrait.php @@ -0,0 +1,200 @@ +mTitle = $title; + $services = MediaWikiServices::getInstance(); + if ( !is_null( $this->mTitle ) ) { + $id = false; + Hooks::run( 'SearchResultInitFromTitle', [ $title, &$id ] ); + $this->mRevision = Revision::newFromTitle( + $this->mTitle, $id, Revision::READ_NORMAL ); + if ( $this->mTitle->getNamespace() === NS_FILE ) { + $this->mImage = $services->getRepoGroup()->findFile( $this->mTitle ); + } + } + } + + /** + * Check if this is result points to an invalid title + * + * @return bool + */ + public function isBrokenTitle() { + return is_null( $this->mTitle ); + } + + /** + * Check if target page is missing, happens when index is out of date + * + * @return bool + */ + public function isMissingRevision() { + return !$this->mRevision && !$this->mImage; + } + + /** + * @return Title + */ + public function getTitle() { + return $this->mTitle; + } + + /** + * Get the file for this page, if one exists + * @return File|null + */ + public function getFile() { + return $this->mImage; + } + + /** + * Lazy initialization of article text from DB + */ + protected function initText() { + if ( !isset( $this->mText ) ) { + if ( $this->mRevision != null ) { + $content = $this->mRevision->getContent(); + $this->mText = $content !== null ? $content->getTextForSearchIndex() : ''; + } else { // TODO: can we fetch raw wikitext for commons images? + $this->mText = ''; + } + } + } + + /** + * @param string[] $terms Terms to highlight (this parameter is deprecated and ignored) + * @return string Highlighted text snippet, null (and not '') if not supported + */ + public function getTextSnippet( $terms = [] ) { + return ''; + } + + /** + * @return string Highlighted title, '' if not supported + */ + public function getTitleSnippet() { + return ''; + } + + /** + * @return string Highlighted redirect name (redirect to this page), '' if none or not supported + */ + public function getRedirectSnippet() { + return ''; + } + + /** + * @return Title|null Title object for the redirect to this page, null if none or not supported + */ + public function getRedirectTitle() { + return null; + } + + /** + * @return string Highlighted relevant section name, null if none or not supported + */ + public function getSectionSnippet() { + return ''; + } + + /** + * @return Title|null Title object (pagename+fragment) for the section, + * null if none or not supported + */ + public function getSectionTitle() { + return null; + } + + /** + * @return string Highlighted relevant category name or '' if none or not supported + */ + public function getCategorySnippet() { + return ''; + } + + /** + * @return string Timestamp + */ + public function getTimestamp() { + if ( $this->mRevision ) { + return $this->mRevision->getTimestamp(); + } elseif ( $this->mImage ) { + return $this->mImage->getTimestamp(); + } + return ''; + } + + /** + * @return int Number of words + */ + public function getWordCount() { + $this->initText(); + return str_word_count( $this->mText ); + } + + /** + * @return int Size in bytes + */ + public function getByteSize() { + $this->initText(); + return strlen( $this->mText ); + } + + /** + * @return string Interwiki prefix of the title (return iw even if title is broken) + */ + public function getInterwikiPrefix() { + return ''; + } + + /** + * @return string Interwiki namespace of the title (since we likely can't resolve it locally) + */ + public function getInterwikiNamespaceText() { + return ''; + } + + /** + * Did this match file contents (eg: PDF/DJVU)? + * @return bool + */ + public function isFileMatch() { + return false; + } +} diff --git a/includes/search/SearchResult.php b/includes/search/SearchResult.php index 4f91ccbbb9..3d32de776d 100644 --- a/includes/search/SearchResult.php +++ b/includes/search/SearchResult.php @@ -21,36 +21,29 @@ * @ingroup Search */ -use MediaWiki\MediaWikiServices; - /** - * @todo FIXME: This class is horribly factored. It would probably be better to - * have a useful base class to which you pass some standard information, then - * let the fancy self-highlighters extend that. + * NOTE: this class is being refactored into an abstract base class. + * If you extend this class directly, please implement all the methods declared + * in RevisionSearchResultTrait or extend RevisionSearchResult. + * + * Once the hard-deprecation period is over (1.36?): + * - all methods declared in RevisionSearchResultTrait should be declared + * as abstract in this class + * - RevisionSearchResultTrait body should be moved to RevisionSearchResult and then removed without + * deprecation + * - caveat: all classes extending this one may potentially break if they did not properly implement + * all the methods. * @ingroup Search */ class SearchResult { use SearchResultTrait; + use RevisionSearchResultTrait; - /** - * @var Revision - */ - protected $mRevision = null; - - /** - * @var File - */ - protected $mImage = null; - - /** - * @var Title - */ - protected $mTitle; - - /** - * @var string - */ - protected $mText; + public function __construct() { + if ( self::class === static::class ) { + wfDeprecated( __METHOD__, '1.34' ); + } + } /** * Return a new SearchResult and initializes it with a title. @@ -60,180 +53,10 @@ class SearchResult { * @return SearchResult */ public static function newFromTitle( $title, ISearchResultSet $parentSet = null ) { - $result = new static(); - $result->initFromTitle( $title ); + $result = new RevisionSearchResult( $title ); if ( $parentSet ) { $parentSet->augmentResult( $result ); } return $result; } - - /** - * Initialize from a Title and if possible initializes a corresponding - * Revision and File. - * - * @param Title $title - */ - protected function initFromTitle( $title ) { - $this->mTitle = $title; - $services = MediaWikiServices::getInstance(); - if ( !is_null( $this->mTitle ) ) { - $id = false; - Hooks::run( 'SearchResultInitFromTitle', [ $title, &$id ] ); - $this->mRevision = Revision::newFromTitle( - $this->mTitle, $id, Revision::READ_NORMAL ); - if ( $this->mTitle->getNamespace() === NS_FILE ) { - $this->mImage = $services->getRepoGroup()->findFile( $this->mTitle ); - } - } - } - - /** - * Check if this is result points to an invalid title - * - * @return bool - */ - public function isBrokenTitle() { - return is_null( $this->mTitle ); - } - - /** - * Check if target page is missing, happens when index is out of date - * - * @return bool - */ - public function isMissingRevision() { - return !$this->mRevision && !$this->mImage; - } - - /** - * @return Title - */ - public function getTitle() { - return $this->mTitle; - } - - /** - * Get the file for this page, if one exists - * @return File|null - */ - public function getFile() { - return $this->mImage; - } - - /** - * Lazy initialization of article text from DB - */ - protected function initText() { - if ( !isset( $this->mText ) ) { - if ( $this->mRevision != null ) { - $content = $this->mRevision->getContent(); - $this->mText = $content !== null ? $content->getTextForSearchIndex() : ''; - } else { // TODO: can we fetch raw wikitext for commons images? - $this->mText = ''; - } - } - } - - /** - * @param string[] $terms Terms to highlight (this parameter is deprecated and ignored) - * @return string Highlighted text snippet, null (and not '') if not supported - */ - public function getTextSnippet( $terms = [] ) { - return ''; - } - - /** - * @return string Highlighted title, '' if not supported - */ - public function getTitleSnippet() { - return ''; - } - - /** - * @return string Highlighted redirect name (redirect to this page), '' if none or not supported - */ - public function getRedirectSnippet() { - return ''; - } - - /** - * @return Title|null Title object for the redirect to this page, null if none or not supported - */ - public function getRedirectTitle() { - return null; - } - - /** - * @return string Highlighted relevant section name, null if none or not supported - */ - public function getSectionSnippet() { - return ''; - } - - /** - * @return Title|null Title object (pagename+fragment) for the section, - * null if none or not supported - */ - public function getSectionTitle() { - return null; - } - - /** - * @return string Highlighted relevant category name or '' if none or not supported - */ - public function getCategorySnippet() { - return ''; - } - - /** - * @return string Timestamp - */ - public function getTimestamp() { - if ( $this->mRevision ) { - return $this->mRevision->getTimestamp(); - } elseif ( $this->mImage ) { - return $this->mImage->getTimestamp(); - } - return ''; - } - - /** - * @return int Number of words - */ - public function getWordCount() { - $this->initText(); - return str_word_count( $this->mText ); - } - - /** - * @return int Size in bytes - */ - public function getByteSize() { - $this->initText(); - return strlen( $this->mText ); - } - - /** - * @return string Interwiki prefix of the title (return iw even if title is broken) - */ - public function getInterwikiPrefix() { - return ''; - } - - /** - * @return string Interwiki namespace of the title (since we likely can't resolve it locally) - */ - public function getInterwikiNamespaceText() { - return ''; - } - - /** - * Did this match file contents (eg: PDF/DJVU)? - * @return bool - */ - public function isFileMatch() { - return false; - } - } diff --git a/includes/search/SqlSearchResult.php b/includes/search/SqlSearchResult.php index 9804e44d7d..f470dbb008 100644 --- a/includes/search/SqlSearchResult.php +++ b/includes/search/SqlSearchResult.php @@ -22,7 +22,7 @@ * @ingroup Search */ -class SqlSearchResult extends SearchResult { +class SqlSearchResult extends RevisionSearchResult { /** @var string[] */ private $terms; @@ -32,7 +32,7 @@ class SqlSearchResult extends SearchResult { * @param string[] $terms list of parsed terms */ public function __construct( Title $title, array $terms ) { - $this->initFromTitle( $title ); + parent::__construct( $title ); $this->terms = $terms; } diff --git a/tests/phpunit/includes/api/ApiQuerySearchTest.php b/tests/phpunit/includes/api/ApiQuerySearchTest.php index 93c534583d..cf835ced0b 100644 --- a/tests/phpunit/includes/api/ApiQuerySearchTest.php +++ b/tests/phpunit/includes/api/ApiQuerySearchTest.php @@ -119,7 +119,7 @@ class ApiQuerySearchTest extends ApiTestCase { */ private function mockResultClosure( $title, $setters = [] ) { return function () use ( $title, $setters ){ - $result = MockSearchResult::newFromTitle( Title::newFromText( $title ) ); + $result = new MockSearchResult( Title::newFromText( $title ) ); foreach ( $setters as $method => $param ) { $result->$method( $param ); diff --git a/tests/phpunit/mocks/search/MockSearchResult.php b/tests/phpunit/mocks/search/MockSearchResult.php index e92eb5660c..418832e449 100644 --- a/tests/phpunit/mocks/search/MockSearchResult.php +++ b/tests/phpunit/mocks/search/MockSearchResult.php @@ -1,6 +1,6 @@