Hard deprecate new SearchResult() and introduce RevisionSearchResult
authorDavid Causse <dcausse@wikimedia.org>
Thu, 1 Aug 2019 20:38:46 +0000 (22:38 +0200)
committerErik Bernhardson <ebernhardson@wikimedia.org>
Tue, 27 Aug 2019 15:27:28 +0000 (08:27 -0700)
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
autoload.php
includes/search/RevisionSearchResult.php [new file with mode: 0644]
includes/search/RevisionSearchResultTrait.php [new file with mode: 0644]
includes/search/SearchResult.php
includes/search/SqlSearchResult.php
tests/phpunit/includes/api/ApiQuerySearchTest.php
tests/phpunit/mocks/search/MockSearchResult.php

index 620a6f5..73a1f7b 100644 (file)
@@ -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 ===
 * …
index 95297fb..57e46e9 100644 (file)
@@ -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 (file)
index 0000000..f94ea2a
--- /dev/null
@@ -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 );
+       }
+}
diff --git a/includes/search/RevisionSearchResultTrait.php b/includes/search/RevisionSearchResultTrait.php
new file mode 100644 (file)
index 0000000..24370c3
--- /dev/null
@@ -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;
+       }
+}
index 4f91ccb..3d32de7 100644 (file)
  * @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;
-       }
-
 }
index 9804e44..f470dbb 100644 (file)
@@ -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;
        }
 
index 93c5345..cf835ce 100644 (file)
@@ -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 );
index e92eb56..418832e 100644 (file)
@@ -1,6 +1,6 @@
 <?php
 
-class MockSearchResult extends SearchResult {
+class MockSearchResult extends RevisionSearchResult {
        private $isMissingRevision = false;
        private $isBrokenTitle = false;