Page MenuHomePhabricator

Deprecate Skin::getSearchPageTitle and Skin::setSearchPageTitle
Closed, ResolvedPublicBUG REPORT

Description

While [[ [[ https://gerrit.wikimedia.org/r/c/mediawiki/core/+/730618 | URL ]] | refactoring skins ]], we have encountered a problem with two existing Skin methods, which were introduced to support Special:MediaSearch in adda9c0555 (T273879) which is blocking the work there.

It's used in one place here: https://gerrit.wikimedia.org/g/mediawiki/extensions/MediaSearch/+/4d96a28b3f735f54c29526eec4ed0c1432878471/src/MediaSearchHooks.php#46

What the search page is out of the scope of concerns of the Skin and we would like to remove this as part of the refactor task.

The way we typically allow alteration of pages, is

  1. via message keys via the WikimediaMessages extension (See MediaWiki:portal-url and MediaWiki:privacypage ) or
  2. configuration variable (where the setting is made on site level).
  3. Alternatively, this should be a method higher up the change e.g. OutputPage, Title or SpecialSearch. The Title::newMainPage method exists, so perhaps what's needed is Title::newSearchPage

Happy to work with you to find a better approach to doing this.

Event Timeline

Jdlrobson updated the task description. (Show Details)

Change 738459 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Use MediaWiki message for working out what to use for the default search experience

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

Change 738463 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/MediaSearch@master] Set special search page by message key

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

@Cparle let me know how you would like to proceed with achieving the goal of this task (timeline - hoping to work this out by the first week of December)

What the search page is out of the scope of concerns of the Skin

I feel like the patch here doesn't really address the problem described - now the Skin contains more rather than less logic concerning the search page. IMO option 3 above (Title::newSearchPage(), probably containing a hook that MediaSearch can call) is cleaner. What do you think @Jdlrobson ?

This comment was removed by Jdlrobson.

Change 738459 abandoned by Jdlrobson:

[mediawiki/core@master] Use MediaWiki message for working out what to use for the default search experience

Reason:

Per https://phabricator.wikimedia.org/T295616#7503253 we'll be taking a different approach

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

@Cparle Title::newSearchPage with a hook seems fine to me. Would you be able to provide a patch or code review (if I provide a patch)?

@Jdlrobson sure ... pretty busy atm, but I don't think this will be that difficult. If I was to do a patch when do you want it for?

by the end of December would work great. Also happy to write the patch, if code review is easier for you then patch authorship right now.

Jdlrobson triaged this task as Medium priority.Dec 4 2021, 1:30 AM

I was wondering if a new hook is needed, and it seems like MediaSearch only needs to vary the search title by the user preference sdms-specialsearch-default
What if we were to add a user preference searchpage that allowed users to set their search page, would that work for your use case?

diff --git a/includes/Title.php b/includes/Title.php
index 07d18c80be..2a54739ed9 100644
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -744,6 +744,49 @@ class Title implements LinkTarget, PageIdentity, IDBAccessObject {
 		return $title;
 	}
 
+	/**
+	 * Create a new Title for the search page.
+	 *
+	 * This uses the 'mainpage' interface message, which could be specified in
+	 * `$wgForceUIMsgAsContentMsg`. If that is the case, then calling this method
+	 * will use the user language, which would involve initialising the session
+	 * via `RequestContext::getMain()->getLanguage()`. For session-less endpoints,
+	 * be sure to pass in a MessageLocalizer (such as your own RequestContext,
+	 * or ResourceloaderContext) to prevent an error.
+	 *
+	 * @note The Title instance returned by this method is not guaranteed to be a fresh instance.
+	 * It may instead be a cached instance created previously, with references to it remaining
+	 * elsewhere.
+	 *
+	 * @since 1.38
+	 * @param User $user Search page can be customized by user preference.
+	 * @param MessageLocalizer|null $localizer An optional context to use
+	 * @return Title
+	 */
+	public static function newSearchPage( User $user, MessageLocalizer $localizer = null ) {
+		// Try user preference first
+		if ( $user ) {
+			$userOptionsManager = MediaWikiServices::getInstance()->getUserOptionsManager();
+			$title = $userOptionsManager->getOption( $user, 'searchpage' );
+		}
+
+		// if no user preference set, then try the interface messages.
+		if ( !$title ) {
+			if ( $localizer ) {
+				$msg = $localizer->msg( 'search' );
+			} else {
+				$msg = wfMessage( 'search' );
+			}
+			$title = self::newFromText( $msg->inContentLanguage()->text() );
+		}
+
+		// If still no title, assume Special:Search
+		if ( !$title ) {
+			$title = self::newFromText( 'Search' );
+		}
+		return $title;
+	}
+
 	/**
 	 * Get a regex character class describing the legal characters in a link
 	 *
diff --git a/includes/TitleFactory.php b/includes/TitleFactory.php
index 29dae28c40..5265d74c68 100644
--- a/includes/TitleFactory.php
+++ b/includes/TitleFactory.php
@@ -176,4 +176,13 @@ class TitleFactory {
 		return Title::newMainPage( $localizer );
 	}
 
+	/**
+	 * @see Title::newSearchPage
+	 * @param MessageLocalizer|null $localizer
+	 * @return Title
+	 */
+	public function newSearchPage( MessageLocalizer $localizer = null ): Title {
+		return Title::newSearchPage( $localizer );
+	}
+
 }

That doesn't really work @Jdlrobson I don't think - we want the default search page on commons to be MediaSearch, and allow power-users to opt out (and then obvs on other wikis we want the default to be Special:Search)

@Cparle why doesn't that work? What am I misunderstanding?

It seems to me that the search page title varies by user preference only (currently the sdms-specialsearch-default preference key). Is that not correct? Does it vary on other things?

What I'm suggesting is it can still vary by user preference, but a core preference.

If set as a user preference we can specify a default value - MediaSearch for Commons and Search for all other wikis. Power users can opt out of MediaSearch by changing that preference to the default value.

Note: We can control how we present that preference. It could be a checkbox or dropdown for example (or it could be internal with MediaSearch setting it inside PreferencesFormPreSave) and we can limit the accepted values.

Hmmm I guess you're right - so we'd remove sdms-specialsearch-default, and just have a core preference that's set by default that the user can change? We'd only have one option on non-commons wikis though (at least for now) - might that be a problem?

Hmmm I guess you're right - so we'd remove sdms-specialsearch-default, and just have a core preference that's set by default that the user can change? We'd only have one option on non-commons wikis though (at least for now) - might that be a problem?

I can look into the preferences code today and get back to you.

Change 744899 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Add SpecialPage::newSearchPage to replace Skin::setSearchPageTitle

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

Change 745339 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Hard deprecate Skin:setSearchPageTitle

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

Change 738463 abandoned by Jdlrobson:

[mediawiki/extensions/MediaSearch@master] Set special search page by message key

Reason:

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

Change 744899 merged by jenkins-bot:

[mediawiki/core@master] Add SpecialPage::newSearchPage to replace Skin::setSearchPageTitle

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

Jdlrobson claimed this task.

Follow up work in T297484