r96824 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r96823‎ | r96824 | r96825 >
Date:01:39, 12 September 2011
Author:robin
Status:reverted (Comments)
Tags:
Comment:
SpecialSearch: add two hooks, one to modify the power search box, one to change the message pointing to the page title searched for

WikimediaIncubator:
* use these hooks and SpecialSearchSetupEngine to improve search usability
* use a different message for the language code in preferences, per suggestion of Amir
Modified paths:
  • /trunk/extensions/WikimediaIncubator/IncubatorTest.php (modified) (history)
  • /trunk/extensions/WikimediaIncubator/WikimediaIncubator.i18n.php (modified) (history)
  • /trunk/extensions/WikimediaIncubator/WikimediaIncubator.php (modified) (history)
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/specials/SpecialSearch.php (modified) (history)

Diff [purge]

Index: trunk/phase3/docs/hooks.txt
@@ -1741,6 +1741,11 @@
17421742 &$query_options: array of options for the database request
17431743 &$select: Array of columns to select
17441744
 1745+'SpecialSearchCreateLink': called when making the message to create a page or
 1746+go to the existing page
 1747+$t: title object searched for
 1748+&$params: an array of the default message name and page title (as parameter)
 1749+
17451750 'SpecialSearchGo': called when user clicked the "Go"
17461751 &$title: title object generated from the text entered by the user
17471752 &$term: the search term entered by the user
@@ -1749,6 +1754,12 @@
17501755 target doesn't exist
17511756 &$title: title object generated from the text entered by the user
17521757
 1758+'SpecialSearchPowerBox': the equivalent of SpecialSearchProfileForm for
 1759+the advanced form, a.k.a. power search box
 1760+&$showSections: an array to add values with more options to
 1761+$term: the search term (not a title object)
 1762+$opts: an array of hidden options (containing 'redirs' and 'profile')
 1763+
17531764 'SpecialSearchProfiles': allows modification of search profiles
17541765 &$profiles: profiles, which can be modified.
17551766
Index: trunk/phase3/includes/specials/SpecialSearch.php
@@ -411,8 +411,11 @@
412412 $messageName = 'searchmenu-new-nocreate';
413413 }
414414 }
 415+ $params = array( $messageName, wfEscapeWikiText( $t->getPrefixedText() ) );
 416+ wfRunHooks( 'SpecialSearchCreateLink', array( $t, &$params ) );
 417+
415418 if( $messageName ) {
416 - $this->getOutput()->wrapWikiMsg( "<p class=\"mw-search-createlink\">\n$1</p>", array( $messageName, wfEscapeWikiText( $t->getPrefixedText() ) ) );
 419+ $this->getOutput()->wrapWikiMsg( "<p class=\"mw-search-createlink\">\n$1</p>", $params );
417420 } else {
418421 // preserve the paragraph for margins etc...
419422 $this->getOutput()->addHtml( '<p></p>' );
@@ -871,13 +874,17 @@
872875 }
873876 $namespaceTables .= Xml::closeElement( 'table' );
874877 }
 878+
 879+ $showSections = array( 'namespaceTables' => $namespaceTables );
 880+
875881 // Show redirects check only if backend supports it
876 - $redirects = '';
877882 if( $this->getSearchEngine()->supports( 'list-redirects' ) ) {
878 - $redirects =
 883+ $showSections['redirects'] =
879884 Xml::checkLabel( wfMsg( 'powersearch-redir' ), 'redirs', 'redirs', $this->searchRedirects );
880885 }
881886
 887+ wfRunHooks( 'SpecialSearchPowerBox', array( &$showSections, $term, $opts ) );
 888+
882889 $hidden = '';
883890 unset( $opts['redirs'] );
884891 foreach( $opts as $key => $value ) {
@@ -913,9 +920,8 @@
914921 )
915922 ) .
916923 Xml::element( 'div', array( 'class' => 'divider' ), '', false ) .
917 - $namespaceTables .
918 - Xml::element( 'div', array( 'class' => 'divider' ), '', false ) .
919 - $redirects . $hidden .
 924+ implode( Xml::element( 'div', array( 'class' => 'divider' ), '', false ), $showSections ) .
 925+ $hidden .
920926 Xml::closeElement( 'fieldset' );
921927 }
922928
Index: trunk/extensions/WikimediaIncubator/WikimediaIncubator.php
@@ -16,7 +16,7 @@
1717 'path' => __FILE__,
1818 'name' => 'Wikimedia Incubator',
1919 'author' => 'SPQRobin',
20 - 'version' => '4.4',
 20+ 'version' => '4.5',
2121 'url' => 'http://www.mediawiki.org/wiki/Extension:WikimediaIncubator',
2222 'descriptionmsg' => 'wminc-desc',
2323 );
@@ -149,3 +149,8 @@
150150 $wgHooks['SpecialListusersHeaderForm'][] = 'ListUsersTestWiki::onSpecialListusersHeaderForm';
151151 $wgHooks['SpecialListusersQueryInfo'][] = 'ListUsersTestWiki::onSpecialListusersQueryInfo';
152152 $wgHooks['SpecialListusersHeader'][] = 'ListUsersTestWiki::onSpecialListusersHeader';
 153+
 154+/* Search in test wiki */
 155+$wgHooks['SpecialSearchCreateLink'][] = 'IncubatorTest::onSpecialSearchCreateLink';
 156+$wgHooks['SpecialSearchPowerBox'][] = 'IncubatorTest::onSpecialSearchPowerBox';
 157+$wgHooks['SpecialSearchSetupEngine'][] = 'IncubatorTest::onSpecialSearchSetupEngine';
Index: trunk/extensions/WikimediaIncubator/IncubatorTest.php
@@ -35,7 +35,7 @@
3636 $prefinsert[$wmincPref . '-code'] = array(
3737 'type' => 'text',
3838 'section' => 'personal/i18n',
39 - 'label-message' => 'wminc-testwiki',
 39+ 'label-message' => 'wminc-testwiki-code',
4040 'id' => $wmincPref . '-code',
4141 'maxlength' => (int)$wmincLangCodeLength,
4242 'size' => (int)$wmincLangCodeLength,
@@ -695,6 +695,62 @@
696696 return true;
697697 }
698698
 699+ /**
 700+ * Search: Adapt the default message to show a more descriptive one,
 701+ * along with an adapted link.
 702+ * @return true
 703+ */
 704+ public static function onSpecialSearchCreateLink( $title, &$params ) {
 705+ if( $title->isKnown() ) {
 706+ return true;
 707+ }
 708+ global $wmincProjectSite, $wmincTestWikiNamespaces;
 709+ $prefix = self::displayPrefix();
 710+
 711+ $newNs = $title->getNamespace();
 712+ $newTitle = $title->getText();
 713+ if( $prefix == $wmincProjectSite['short'] ) {
 714+ $newNs = NS_PROJECT;
 715+ } else {
 716+ if( !in_array( $title->getNamespace(), $wmincTestWikiNamespaces ) ) {
 717+ $newNs = $wmincTestWikiNamespaces[0]; # no "valid" NS, should be main NS
 718+ }
 719+ $newTitle = $prefix . '/' . $newTitle;
 720+ }
 721+
 722+ $t = Title::newFromText( $newTitle, $newNs );
 723+ if( $t->isKnown() ) {
 724+ # use the default message if the suggested title exists
 725+ $params[0] = 'searchmenu-exists';
 726+ $params[1] = wfEscapeWikiText( $t->getPrefixedText() );
 727+ return true;
 728+ }
 729+ $params[] = wfEscapeWikiText( $t->getPrefixedText() );
 730+ $params[0] = $prefix ? 'wminc-search-nocreate-suggest' :'wminc-search-nocreate-nopref';
 731+ return true;
 732+ }
 733+
 734+ /**
 735+ * Search: Add an input form to enter a test wiki prefix.
 736+ * @return true
 737+ */
 738+ public static function onSpecialSearchPowerBox( &$showSections, $term, $opts ) {
 739+ $showSections['testwiki'] = Xml::label( wfMsg( 'wminc-testwiki' ), 'testwiki' ) . ' ' .
 740+ Xml::input( 'testwiki', 20, self::displayPrefix(), array( 'id' => 'testwiki' ) );
 741+ return true;
 742+ }
 743+
 744+ /**
 745+ * Search: Search by default in the test wiki of the user's preference (or url &testwiki).
 746+ * @return true
 747+ */
 748+ public static function onSpecialSearchSetupEngine( $search, $profile, $engine ) {
 749+ if( !isset( $search->prefix ) || !$search->prefix ) {
 750+ $search->prefix = self::displayPrefix();
 751+ }
 752+ return true;
 753+ }
 754+
699755 private static function preg_quote_slash( $str ) {
700756 return preg_quote( $str, '/' );
701757 }
Index: trunk/extensions/WikimediaIncubator/WikimediaIncubator.i18n.php
@@ -16,6 +16,7 @@
1717 'wminc-manual' => 'Manual',
1818 'wminc-listwikis' => 'List of wikis',
1919 'wminc-testwiki' => 'Test wiki:',
 20+ 'wminc-testwiki-code' => 'Test wiki language:',
2021 'wminc-testwiki-none' => 'None/All',
2122 'wminc-recentchanges-all' => 'All recent changes',
2223
@@ -57,6 +58,10 @@
5859
5960 # Special:ListUsers
6061 'wminc-listusers-testwiki' => 'You are viewing users who have set their test wiki preference to $1.',
 62+
 63+ # Search
 64+ 'wminc-search-nocreate-nopref' => 'You searched for "$1". Please set your [[Special:Preferences|test wiki preference]] so we can tell you which page you can create!',
 65+ 'wminc-search-nocreate-suggest' => 'You searched for "$1". You can create a page in your wiki at <b>[[$2]]</b>!',
6166 );
6267
6368 /** Message documentation (Message documentation)

Follow-up revisions

RevisionCommit summaryAuthorDate
r97097Revert r96824: causes fatal errors when search query doesn't happen to be a v...brion20:34, 14 September 2011
r97173Merged revisions 97087,97091-97092,97094,97096-97098,97100-97101,97103,97136,...dantman16:19, 15 September 2011
r97175Re-do reverted r96824, but in SpecialSearch first return if the title object ...robin16:26, 15 September 2011
r97538merging in changes r96824:97516. Not rebranching since we have hacks for 1.17...neilk18:53, 19 September 2011

Comments

#Comment by SPQRobin (talk | contribs)   01:41, 12 September 2011

It would be very nice if the changes in SpecialSearch may be backported to either REL1_18 or the soon-to-be 1.18wmf1 branch, so that I wouldn't have to wait for 1.19 deployment :-S

#Comment by Catrope (talk | contribs)   16:54, 13 September 2011

I'm untagging this for 1.18 and tagging it for 1.18wmf1. I will include this the next time I update WikimediaIncubator, some time after the 1.18 deployment.

#Comment by Siebrand (talk | contribs)   07:39, 12 September 2011

Added 1.18 label.

When adding UI messages, please add documentation in the qqq language. It much harder to do that afterwards.

#Comment by SPQRobin (talk | contribs)   12:40, 12 September 2011

Thanks. Ok, I'll add documentation on TWN now, but in the future I'll do it directly.

#Comment by Siebrand (talk | contribs)   14:09, 12 September 2011

*hugs!*

#Comment by Brion VIBBER (talk | contribs)   20:30, 14 September 2011

Fatal error when input is not a valid title:

[Wed Sep 14 13:29:15 2011] [error] [client 192.168.38.113] PHP Fatal error: Call to a member function getPrefixedText() on a non-object in /var/www/trunk/includes/specials/SpecialSearch.php on line 414, referer: http://stormcloud.local/trunk/index.php/Main_Page

Needs revert.

#Comment by SPQRobin (talk | contribs)   15:38, 15 September 2011

What was the input? I tried a lot but did not ever get a fatal error (and I don't see how it's possible in the code).

#Comment by Catrope (talk | contribs)   15:45, 15 September 2011

The function start with:

	protected function showCreateLink( $t ) {
		// show direct page/create link if applicable
		// Check DBkey !== '' in case of fragment link only.
		$messageName = null;
		if( !is_null($t) && $t->getDBkey() !== '' ) {
			if( $t->isKnown() ) {
				$messageName = 'searchmenu-exists';
			} elseif( $t->userCan( 'create' ) ) {
				$messageName = 'searchmenu-new';
			} else {
				$messageName = 'searchmenu-new-nocreate';
			}
		}

and below that you are adding (outside of the if block!):

+		$params = array( $messageName, wfEscapeWikiText( $t->getPrefixedText() ) );
+		wfRunHooks( 'SpecialSearchCreateLink', array( $t, &$params ) );

So you're calling $t->getPrefixedText() unconditionally, right after the end of an if statement that checks that $t isn't null before messing with it.

#Comment by SPQRobin (talk | contribs)   15:55, 15 September 2011

I see. I will do if( is_null($t) || $t->getDBkey() === ) { ... return; } and then do the rest of the code.

#Comment by Brion VIBBER (talk | contribs)   16:37, 15 September 2011

for an easy example throw some [ ] or { } or < > in - these are pretty consistently disallowed kn titles, so good to catch assumptions about the search query.

#Comment by Catrope (talk | contribs)   17:08, 15 September 2011

Another nice one is the pipe (|) which is used as a separator in a lot of places, specifically because it is disallowed in titles.

#Comment by Brion VIBBER (talk | contribs)   20:31, 14 September 2011

Removing 1.18wmf1 merge tag as this is broken.

#Comment by Brion VIBBER (talk | contribs)   20:35, 14 September 2011

Reverted this from trunk in r97097.

Status & tagging log