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

@@ -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 ===
 * …

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',

includes/search/RevisionSearchResult.php

@@ -0,0 +1,18 @@
+<?php
+
+/**
+ * SearchResult class based on the Revision information.
+ * This class is suited for search engines that do not store a specialized version of the searched
+ * content.
+ */
+class RevisionSearchResult extends SearchResult {
+	use RevisionSearchResultTrait;
+
+	/**
+	 * @param Title|null $title
+	 */
+	public function __construct( $title ) {
+		$this->mTitle = $title;
+		$this->initFromTitle( $title );
+	}
+}

includes/search/RevisionSearchResultTrait.php

@@ -0,0 +1,200 @@
+<?php
+
+use MediaWiki\MediaWikiServices;
+
+/**
+ * Transitional trait used to share the methods between SearchResult and RevisionSearchResult.
+ * All the content of this trait can be moved to RevisionSearchResult once SearchResult is finally
+ * refactored into an abstract class.
+ * NOTE: This trait MUST NOT be used by something else than SearchResult and RevisionSearchResult.
+ * It will be removed without deprecation period once SearchResult
+ */
+trait RevisionSearchResultTrait {
+	/**
+	 * @var Revision
+	 */
+	protected $mRevision = null;
+
+	/**
+	 * @var File
+	 */
+	protected $mImage = null;
+
+	/**
+	 * @var Title
+	 */
+	protected $mTitle;
+
+	/**
+	 * @var string
+	 */
+	protected $mText;
+
+	/**
+	 * 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;
+	}
+}

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;
-	}
-
 }

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;
 	}
 

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 );

tests/phpunit/mocks/search/MockSearchResult.php

@@ -1,6 +1,6 @@
 <?php
 
-class MockSearchResult extends SearchResult {
+class MockSearchResult extends RevisionSearchResult {
 	private $isMissingRevision = false;
 	private $isBrokenTitle = false;