Page MenuHomePhabricator

search prefix is set to wrong object in WikimediaIncubator extension on hook SpecialSearchSetupEngine
Open, Needs TriagePublic

Description

In the hook code for hook SpecialSearchSetupEngine the display prefix for WikimediaIncubator is set on the special page SpecialSearch

	public static function onSpecialSearchSetupEngine( $search, $profile, $engine ) {
		if ( !isset( $search->prefix ) || !$search->prefix ) {
			$search->prefix = self::displayPrefix();
		}
		return true;
	}

The special page SpecialSearch $search declares a property $mPrefix.
The SearchEngine $engine declares a property $prefix

Is the prefix set on the wrong object or I am missing some magic between $mPrefix and $prefix in SpecialSearch?

I am unsure how to fix this. It looks like it is not working and dead code should be removed. Or it is better to fix and all are suppressed that things working other than before (but now right)?

<file name="includes\WikimediaIncubator.php">
  <error line="951" severity="warning" message="Reference to undeclared property \SpecialSearch-&amp;gt;prefix" source="PhanUndeclaredProperty"/>
  <error line="952" severity="warning" message="Reference to undeclared property \SpecialSearch-&amp;gt;prefix" source="PhanUndeclaredProperty"/>
</file>

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 25 2019, 8:49 PM
Reedy updated the task description. (Show Details)May 25 2019, 8:58 PM
Reedy added a subscriber: Reedy.May 25 2019, 9:10 PM
		$search = $this->getSearchEngine();
		$search->setFeatureData( 'rewrite', $this->runSuggestion );
		$search->setLimitOffset( $this->limit, $this->offset );
		$search->setNamespaces( $this->namespaces );
		$search->setSort( $this->sort );
		$search->prefix = $this->mPrefix;

		Hooks::run( 'SpecialSearchSetupEngine', [ $this, $this->profile, $search ] );
		if ( !Hooks::run( 'SpecialSearchResultsPrepend', [ $this, $out, $term ] ) ) {
			# Hook requested termination
			return;
		}

I think there's been some confusion with passing the prefix between.... $search where the hook is called is a SearchEngine, but in the hook it's a SpecialPage

'SpecialSearchSetupEngine': Allows passing custom data to search engine.
$search: SpecialSearch special page object
$profile: String: current search profile
$engine: the search engine

https://github.com/wikimedia/mediawiki-extensions-WikimediaIncubator/blame/0d6276c/includes/WikimediaIncubator.php#L950-L955
https://github.com/wikimedia/mediawiki/blame/3562f0a/includes/specials/SpecialSearch.php#L317-L324

Looks like it's potentially been broken for years/since implementation...

SpecialSearch->mPrefix is protected. SearchEngine->prefix is public

I'd suggest all should be $engine->, or the params renamed

Change 512499 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/WikimediaIncubator@master] Use the correct object in onSpecialSearchSetupEngine

https://gerrit.wikimedia.org/r/512499

Change 512500 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Rename $search to $engine to match hook docs for SpecialSearchSetupEngine

https://gerrit.wikimedia.org/r/512500

Looks like it's potentially been broken for years/since implementation...

Yah, that's why I ask and not directly fixed it.

The explaination with the var name in the special page and different name in the hook sounds good to me.

Change 512500 merged by jenkins-bot:
[mediawiki/core@master] Rename $search to $engine to match hook docs for SpecialSearchSetupEngine

https://gerrit.wikimedia.org/r/512500