Page MenuHomePhabricator

Change settings to not override those set in LocalSettings.php or other custom setting sources
Closed, DeclinedPublic

Description

Modified files ready to go.

Various fixes to improve this extension. It has not been updated since 2011-09-11 and is current incompatible with Mediawiki 1.19.x and 1.20.x. It also improves the settings interface to allow it to be configured without modifying the extension.

diff --git a/SphinxMWSearch.php b/SphinxMWSearch.php
index edb08b8..e3e7547 100644
--- a/SphinxMWSearch.php
+++ b/SphinxMWSearch.php
@@ -179,14 +179,13 @@ class SphinxMWSearch extends SearchEngine {
        }
 
        /**
-    * Find snippet highlight settings for a given user
+  * Find snippet highlight settings for all users
         *
-    * @param $user User
         * @return Array contextlines, contextchars
         */
-   public static function userHighlightPrefs( &$user ) {
-           $contextlines = $user->getOption( 'contextlines', 2 );
-           $contextchars = $user->getOption( 'contextchars', 75 );
+ public static function userHighlightPrefs() {
+         $contextlines = 2; // Hardcode this. Old defaults sucked. :)
+         $contextchars = 75; // same as above.... :P
                return array( $contextlines, $contextchars );
        }
 
diff --git a/SphinxSearch.i18n.php b/SphinxSearch.i18n.php
index fce1668..45ebb27 100644
--- a/SphinxSearch.i18n.php
+++ b/SphinxSearch.i18n.php
@@ -21,8 +21,6 @@ $messages['en'] = array(
 $messages['qqq'] = array(
        'sphinxsearch-desc' => '{{desc|name=Sphinx Search|url=http://www.mediawiki.org/wiki/Extension:SphinxSearch}}',
        'sphinxPowered' => '$1 is replaced with a text "Sphinx" inside a link.',
-   'sphinxSearchFailed' => 'Unused at this time. Parameters:
-* $1 - ...',
 );
 
 /** Afrikaans (Afrikaans)
diff --git a/SphinxSearch.php b/SphinxSearch.php
index 2632250..d746461 100644
--- a/SphinxSearch.php
+++ b/SphinxSearch.php
@@ -46,16 +46,24 @@ if ( !class_exists( 'SphinxClient' ) ) {
 }
 
 # Host and port on which searchd deamon is running
-$wgSphinxSearch_host = '127.0.0.1';
-$wgSphinxSearch_port = 9312;
+if (!isset($wgSphinxSearch_host)) {
+ $wgSphinxSearch_host = '127.0.0.1';
+}
+if (!isset($wgSphinxSearch_port) and !is_numeric($wgSphinxSearch_port)) {
+ $wgSphinxSearch_port = 9312;
+}
 
 # Main sphinx.conf index to search
-$wgSphinxSearch_index = "wiki_main";
+if (!$wgSphinxSearch_index) {
+ $wgSphinxSearch_index = "wiki_main";
+}
 
 # By default, we search all available indexes
 # You can also specify them explicitly, e.g
 # $wgSphinxSearch_index_list = "wiki_main,wiki_incremental";
-$wgSphinxSearch_index_list = "*";
+if (!$wgSphinxSearch_index_list) {
+ $wgSphinxSearch_index_list = "*";
+}
 
 # If you have multiple index files, you can specify their weights like this
 # See http://www.sphinxsearch.com/docs/current.html#api-func-setindexweights
@@ -101,7 +109,7 @@ $wgSphinxSearchPersonalDictionary = '';
 
 # If true, use SphinxMWSearch for prefix search instead of the core default.
 # This influences results from ApiOpenSearch.
-$wgEnableSphinxPrefixSearch = false;
+$wgEnableSphinxPrefixSearch = true;
 
 function efSphinxSearchPrefixSetup() {
        global $wgHooks, $wgEnableSphinxPrefixSearch;

Attached:

Details

Reference
bz46611

Event Timeline

bzimport raised the priority of this task from to Normal.
bzimport set Reference to bz46611.
Alexia created this task.Mar 27 2013, 6:26 PM

Please put this in Gerrit.

svemir wrote:

This line does not seem right:

+if (!isset($wgSphinxSearch_port) and !is_numeric($wgSphinxSearch_port)) {

Perhaps it should be:

+if (!isset($wgSphinxSearch_port) || !is_numeric($wgSphinxSearch_port)) {

But in general I would rather let the local administrator make sure if they are setting the port at all to make it correct. Quietly ignoring a bad setting is usually not a good idea.

These may cause notices on stricter configurations:

+if (!$wgSphinxSearch_index) {
+if (!$wgSphinxSearch_index_list) {

Should probably use isset like other suggested changes.

Finally, not sure why we would change default for $wgEnableSphinxPrefixSearch. Does that help with compatibility with 1.19 or 1.20 somehow?

(In reply to comment #2)

This line does not seem right:

+if (!isset($wgSphinxSearch_port) and !is_numeric($wgSphinxSearch_port)) {

Perhaps it should be:

+if (!isset($wgSphinxSearch_port) || !is_numeric($wgSphinxSearch_port)) {

But in general I would rather let the local administrator make sure if they
are
setting the port at all to make it correct. Quietly ignoring a bad setting is
usually not a good idea.

These may cause notices on stricter configurations:

+if (!$wgSphinxSearch_index) {
+if (!$wgSphinxSearch_index_list) {

Should probably use isset like other suggested changes.

Finally, not sure why we would change default for
$wgEnableSphinxPrefixSearch.
Does that help with compatibility with 1.19 or 1.20 somehow?

Sorry about that, I forgot to sanitize the code for Mediawiki's syntax rules. It should be the && operator. With || it could allow a setting slip through for the port that is not a numeric number thus passing bad information to the sphinxapi.php library. Granted, you can pass garbage for the port to sphinxapi.php and it will automatically default to 9312 for the port. Having the check at the extension level is so that the extension is acting on proper data.

I pushed out a new change that changes the default back for $wgEnableSphinxPrefixSearch and changes it to a isset() setting check. We found that having this enabled to true improves search performance overall, but it makes more sense to keep it the old default with a setting check.

(In reply to comment #1)

Please put this in Gerrit.

Still learning some things, sorry! Just figured out what I did wrong and why I could get it to push earlier.
https://gerrit.wikimedia.org/r/#/c/56202/

svemir wrote:

Just to add a note to thank Alexia for all the time and effort invested so far.

I was not dealing with the extension in a while, and MW processes changed a in the meantime.

I will try to figure out how the processes work now and will install everything locally again so that I can contribute to the discussion on gerrit.

matej_suchanek removed a project: Patch-For-Review.
matej_suchanek set Security to None.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 2 2015, 4:24 PM
Alexia closed this task as Declined.Dec 2 2015, 5:31 PM

This patch was abandoned in Gerrit.