From 7075ea92d2d0cee04ea5e385a24f6c36c2f3f25b Mon Sep 17 00:00:00 2001 From: Erik Bernhardson Date: Mon, 14 Nov 2016 17:11:43 -0800 Subject: [PATCH] Cleanup execution flow through SpecialSearch::execute() Not a big change, but makes it so there is only one path into SpecialSearch::showResults(). Makes things a little easier to follow. Also moves the code for checking if full text search is disabled into execute(), makes sense to check before even trying to make a search engine. Also moves some setup code out of execute and into the setupPage function Bug: T150393 Change-Id: Ib527fc3a3c39eb2e56985e5d1e4905fc4562353c --- includes/specials/SpecialSearch.php | 278 +++++++++--------- .../includes/specials/SpecialSearchTest.php | 10 +- 2 files changed, 141 insertions(+), 147 deletions(-) diff --git a/includes/specials/SpecialSearch.php b/includes/specials/SpecialSearch.php index 255e618806..66b95d379e 100644 --- a/includes/specials/SpecialSearch.php +++ b/includes/specials/SpecialSearch.php @@ -101,35 +101,29 @@ class SpecialSearch extends SpecialPage { */ public function execute( $par ) { $request = $this->getRequest(); + $out = $this->getOutput(); // Fetch the search term - $search = str_replace( "\n", " ", $request->getText( 'search' ) ); + $term = str_replace( "\n", " ", $request->getText( 'search' ) ); // Historically search terms have been accepted not only in the search query // parameter, but also as part of the primary url. This can have PII implications // in releasing page view data. As such issue a 301 redirect to the correct // URL. - if ( strlen( $par ) && !strlen( $search ) ) { + if ( strlen( $par ) && !strlen( $term ) ) { $query = $request->getValues(); unset( $query['title'] ); // Strip underscores from title parameter; most of the time we'll want // text form here. But don't strip underscores from actual text params! $query['search'] = str_replace( '_', ' ', $par ); - $this->getOutput()->redirect( $this->getPageTitle()->getFullURL( $query ), 301 ); + $out->redirect( $this->getPageTitle()->getFullURL( $query ), 301 ); return; } - $this->setHeaders(); - $this->outputHeader(); - $out = $this->getOutput(); - $out->allowClickjacking(); - $out->addModuleStyles( [ - 'mediawiki.special', 'mediawiki.special.search.styles', 'mediawiki.ui', 'mediawiki.ui.button', - 'mediawiki.ui.input', 'mediawiki.widgets.SearchInputWidget.styles', - ] ); - $this->addHelpLink( 'Help:Searching' ); - + // Need to load selected namespaces before handling nsRemember $this->load(); + // TODO: This performs database actions on GET request, which is going to + // be a problem for our multi-datacenter work. if ( !is_null( $request->getVal( 'nsRemember' ) ) ) { $this->saveNamespaces(); // Remove the token from the URL to prevent the user from inadvertently @@ -141,16 +135,48 @@ class SpecialSearch extends SpecialPage { return; } - $out->addJsConfigVars( [ 'searchTerm' => $search ] ); $this->searchEngineType = $request->getVal( 'srbackend' ); - - if ( $request->getVal( 'fulltext' ) - || !is_null( $request->getVal( 'offset' ) ) + if ( + !$request->getVal( 'fulltext' ) && + $request->getVal( 'offset' ) === null ) { - $this->showResults( $search ); - } else { - $this->goResult( $search ); + $url = $this->goResult( $term ); + if ( $url !== null ) { + // successful 'go' + $out->redirect( $url ); + return; + } + } + + $this->setupPage( $term ); + + if ( $this->getConfig()->get( 'DisableTextSearch' ) ) { + $searchForwardUrl = $this->getConfig()->get( 'SearchForwardUrl' ); + if ( $searchForwardUrl ) { + $url = str_replace( '$1', urlencode( $term ), $searchForwardUrl ); + $out->redirect( $url ); + } else { + $out->addHTML( + "
" . + "" . + $this->msg( 'search-external' )->escaped() . + "" . + "

" . + $this->msg( 'searchdisabled' )->escaped() . + "

" . + $this->msg( 'googlesearch' )->rawParams( + htmlspecialchars( $term ), + 'UTF-8', + $this->msg( 'searchbutton' )->escaped() + )->text() . + "
" + ); + } + + return; } + + $this->showResults( $term ); } /** @@ -209,32 +235,25 @@ class SpecialSearch extends SpecialPage { * If an exact title match can be found, jump straight ahead to it. * * @param string $term + * @return string|null The url to redirect to, or null if no redirect. */ public function goResult( $term ) { - $this->setupPage( $term ); - # Try to go to page as entered. - $title = Title::newFromText( $term ); # If the string cannot be used to create a title - if ( is_null( $title ) ) { - $this->showResults( $term ); - - return; + if ( is_null( Title::newFromText( $term ) ) ) { + return null; } # If there's an exact or very near match, jump right there. $title = $this->getSearchEngine() ->getNearMatcher( $this->getConfig() )->getNearMatch( $term ); - - if ( !is_null( $title ) && - Hooks::run( 'SpecialSearchGoResult', [ $term, $title, &$url ] ) - ) { - if ( $url === null ) { - $url = $title->getFullURL(); - } - $this->getOutput()->redirect( $url ); - - return; + if ( is_null( $title ) ) { + return null; } - $this->showResults( $term ); + $url = null; + if ( !Hooks::run( 'SpecialSearchGoResult', [ $term, $title, &$url ] ) ) { + return null; + } + + return $url === null ? $title->getFullURL() : $url; } /** @@ -243,42 +262,21 @@ class SpecialSearch extends SpecialPage { public function showResults( $term ) { global $wgContLang; + if ( $this->searchEngineType !== null ) { + $this->setExtraParam( 'srbackend', $this->searchEngineType ); + } + $search = $this->getSearchEngine(); $search->setFeatureData( 'rewrite', $this->runSuggestion ); $search->setLimitOffset( $this->limit, $this->offset ); $search->setNamespaces( $this->namespaces ); $search->prefix = $this->mPrefix; $term = $search->transformSearchTerm( $term ); - - Hooks::run( 'SpecialSearchSetupEngine', [ $this, $this->profile, $search ] ); - - $this->setupPage( $term ); - $out = $this->getOutput(); - if ( $this->getConfig()->get( 'DisableTextSearch' ) ) { - $searchFowardUrl = $this->getConfig()->get( 'SearchForwardUrl' ); - if ( $searchFowardUrl ) { - $url = str_replace( '$1', urlencode( $term ), $searchFowardUrl ); - $out->redirect( $url ); - } else { - $out->addHTML( - Xml::openElement( 'fieldset' ) . - Xml::element( 'legend', null, $this->msg( 'search-external' )->text() ) . - Xml::element( - 'p', - [ 'class' => 'mw-searchdisabled' ], - $this->msg( 'searchdisabled' )->text() - ) . - $this->msg( 'googlesearch' )->rawParams( - htmlspecialchars( $term ), - 'UTF-8', - $this->msg( 'searchbutton' )->escaped() - )->text() . - Xml::closeElement( 'fieldset' ) - ); - } - + Hooks::run( 'SpecialSearchSetupEngine', [ $this, $this->profile, $search ] ); + if ( !Hooks::run( 'SpecialSearchResultsPrepend', [ $this, $out, $term ] ) ) { + # Hook requested termination return; } @@ -298,33 +296,6 @@ class SpecialSearch extends SpecialPage { $textMatches = $textStatus->getValue(); } - // did you mean... suggestions - $didYouMeanHtml = ''; - if ( $showSuggestion && $textMatches ) { - if ( $textMatches->hasRewrittenQuery() ) { - $didYouMeanHtml = $this->getDidYouMeanRewrittenHtml( $term, $textMatches ); - } elseif ( $textMatches->hasSuggestion() ) { - $didYouMeanHtml = $this->getDidYouMeanHtml( $textMatches ); - } - } - - if ( !Hooks::run( 'SpecialSearchResultsPrepend', [ $this, $out, $term ] ) ) { - # Hook requested termination - return; - } - - // start rendering the page - $out->addHTML( - Xml::openElement( - 'form', - [ - 'id' => ( $this->isPowerSearch() ? 'powersearch' : 'search' ), - 'method' => 'get', - 'action' => wfScript(), - ] - ) - ); - // Get number of results $titleMatchesNum = $textMatchesNum = $numTitleMatches = $numTextMatches = 0; if ( $titleMatches ) { @@ -338,18 +309,26 @@ class SpecialSearch extends SpecialPage { $num = $titleMatchesNum + $textMatchesNum; $totalRes = $numTitleMatches + $numTextMatches; + // start rendering the page $out->enableOOUI(); $out->addHTML( - # This is an awful awful ID name. It's not a table, but we - # named it poorly from when this was a table so now we're - # stuck with it - Xml::openElement( 'div', [ 'id' => 'mw-search-top-table' ] ) . - $this->shortDialog( $term, $num, $totalRes ) . - Xml::closeElement( 'div' ) . - $this->searchProfileTabs( $term ) . - $this->searchOptions( $term ) . - Xml::closeElement( 'form' ) . - $didYouMeanHtml + Xml::openElement( + 'form', + [ + 'id' => ( $this->isPowerSearch() ? 'powersearch' : 'search' ), + 'method' => 'get', + 'action' => wfScript(), + ] + ) . + # This is an awful awful ID name. It's not a table, but we + # named it poorly from when this was a table so now we're + # stuck with it + "
" . + $this->shortDialog( $term, $num, $totalRes ) . + "
" . + $this->searchProfileTabs( $term ) . + $this->searchOptions( $term ) . + '' ); $filePrefix = $wgContLang->getFormattedNsText( NS_FILE ) . ':'; @@ -358,9 +337,21 @@ class SpecialSearch extends SpecialPage { return; } + // did you mean... suggestions + if ( $textMatches ) { + if ( $textMatches->hasRewrittenQuery() ) { + $out->addHTML( $this->getDidYouMeanRewrittenHtml( $term, $textMatches ) ); + } elseif ( $textMatches->hasSuggestion() ) { + $out->addHTML( $this->getDidYouMeanHtml( $textMatches ) ); + } + } + $out->addHTML( "
" ); $hasErrors = $textStatus && $textStatus->getErrors(); + $hasOtherResults = $textMatches && + $textMatches->hasInterwikiResults( SearchResultSet::INLINE_RESULTS ); + if ( $hasErrors ) { list( $error, $warning ) = $textStatus->splitByErrorType(); if ( $error->getErrors() ) { @@ -379,25 +370,18 @@ class SpecialSearch extends SpecialPage { } } - // prev/next links - $prevnext = null; - if ( $num || $this->offset ) { - // Show the create link ahead - $this->showCreateLink( $title, $num, $titleMatches, $textMatches ); - if ( $totalRes > $this->limit || $this->offset ) { - if ( $this->searchEngineType !== null ) { - $this->setExtraParam( 'srbackend', $this->searchEngineType ); - } - $prevnext = $this->getLanguage()->viewPrevNext( - $this->getPageTitle(), - $this->offset, - $this->limit, - $this->powerSearchOptions() + [ 'search' => $term ], - $this->limit + $this->offset >= $totalRes - ); - } + // Show the create link ahead + $this->showCreateLink( $title, $num, $titleMatches, $textMatches ); + + // If we have no results and have not already displayed an error message + if ( $num === 0 && !$hasErrors ) { + $out->wrapWikiMsg( "

\n$1

", [ + $hasOtherResults ? 'search-nonefound-thiswiki' : 'search-nonefound', + wfEscapeWikiText( $term ) + ] ); } - Hooks::run( 'SpecialSearchResults', [ $term, &$titleMatches, &$textMatches ] ); + + Hooks::run( 'SpecialSearchResults', [ $term, $titleMatches, $textMatches ] ); $out->parserOptions()->setEditSection( false ); if ( $titleMatches ) { @@ -429,23 +413,6 @@ class SpecialSearch extends SpecialPage { } } - $hasOtherResults = $textMatches && - $textMatches->hasInterwikiResults( SearchResultSet::INLINE_RESULTS ); - - // If we have no results and we have not already displayed an error message - if ( $num === 0 && !$hasErrors ) { - if ( !$this->offset ) { - // If we have an offset the create link was rendered earlier in this function. - // This class needs a good de-spaghettification, but for now this will - // do the job. - $this->showCreateLink( $title, $num, $titleMatches, $textMatches ); - } - $out->wrapWikiMsg( "

\n$1

", [ - $hasOtherResults ? 'search-nonefound-thiswiki' : 'search-nonefound', - wfEscapeWikiText( $term ) - ] ); - } - if ( $hasOtherResults ) { foreach ( $textMatches->getInterwikiResults( SearchResultSet::INLINE_RESULTS ) as $interwiki => $interwikiResult ) { @@ -464,10 +431,19 @@ class SpecialSearch extends SpecialPage { $out->addHTML( '
' ); - if ( $prevnext ) { + // prev/next links + if ( $totalRes > $this->limit || $this->offset ) { + $prevnext = $this->getLanguage()->viewPrevNext( + $this->getPageTitle(), + $this->offset, + $this->limit, + $this->powerSearchOptions() + [ 'search' => $term ], + $this->limit + $this->offset >= $totalRes + ); $out->addHTML( "

{$prevnext}

\n" ); } + // Close
$out->addHTML( "
" ); Hooks::run( 'SpecialSearchResultsAppend', [ $this, $out, $term ] ); @@ -622,10 +598,21 @@ class SpecialSearch extends SpecialPage { } /** + * Sets up everything for the HTML output page including styles, javascript, + * page title, etc. + * * @param string $term */ protected function setupPage( $term ) { $out = $this->getOutput(); + + $this->setHeaders(); + $this->outputHeader(); + // TODO: Is this true? The namespace remember uses a user token + // on save. + $out->allowClickjacking(); + $this->addHelpLink( 'Help:Searching' ); + if ( strval( $term ) !== '' ) { $out->setPageTitle( $this->msg( 'searchresults' ) ); $out->setHTMLTitle( $this->msg( 'pagetitle' ) @@ -633,8 +620,13 @@ class SpecialSearch extends SpecialPage { ->inContentLanguage()->text() ); } - // add javascript specific to special:search + + $out->addJsConfigVars( [ 'searchTerm' => $term ] ); $out->addModules( 'mediawiki.special.search' ); + $out->addModuleStyles( [ + 'mediawiki.special', 'mediawiki.special.search.styles', 'mediawiki.ui', 'mediawiki.ui.button', + 'mediawiki.ui.input', 'mediawiki.widgets.SearchInputWidget.styles', + ] ); } /** @@ -671,12 +663,12 @@ class SpecialSearch extends SpecialPage { */ protected function powerSearchOptions() { $opt = []; - if ( !$this->isPowerSearch() ) { - $opt['profile'] = $this->profile; - } else { + if ( $this->isPowerSearch() ) { foreach ( $this->namespaces as $n ) { $opt['ns' . $n] = 1; } + } else { + $opt['profile'] = $this->profile; } return $opt + $this->extraParams; diff --git a/tests/phpunit/includes/specials/SpecialSearchTest.php b/tests/phpunit/includes/specials/SpecialSearchTest.php index 3fa8a9f8ed..e9cf6a3516 100644 --- a/tests/phpunit/includes/specials/SpecialSearchTest.php +++ b/tests/phpunit/includes/specials/SpecialSearchTest.php @@ -121,13 +121,15 @@ class SpecialSearchTest extends MediaWikiTestCase { ] ); # Initialize [[Special::Search]] + $ctx = new RequestContext(); + $term = '{{SITENAME}}'; + $ctx->setRequest( new FauxRequest( [ 'search' => $term, 'fulltext' => 1 ] ) ); + $ctx->setTitle( Title::newFromText( 'Special:Search' ) ); $search = new SpecialSearch(); - $search->getContext()->setTitle( Title::newFromText( 'Special:Search' ) ); - $search->load(); + $search->setContext( $ctx ); # Simulate a user searching for a given term - $term = '{{SITENAME}}'; - $search->showResults( $term ); + $search->execute( '' ); # Lookup the HTML page title set for that page $pageTitle = $search -- 2.20.1