Issue #2016497 by naxoc, jhodgdon | plachance: Search query extender should not use floats directly.

2 files changed, 83 insertions, 13 deletions

diff --git a/modules/search/search.extender.inc b/modules/search/search.extender.inc
index ad4b86e89..67094666e 100644
--- a/modules/search/search.extender.inc
+++ b/modules/search/search.extender.inc
@@ -105,6 +105,8 @@ class SearchQuery extends SelectQueryExtender {
    * Stores score expressions.
    *
    * @var array
+   *
+   * @see addScore()
    */
   protected $scores = array();
 
@@ -116,7 +118,7 @@ class SearchQuery extends SelectQueryExtender {
   protected $scoresArguments = array();
 
   /**
-   * Total value of all the multipliers.
+   * Stores multipliers for score expressions.
    *
    * @var array
    */
@@ -391,21 +393,39 @@ class SearchQuery extends SelectQueryExtender {
   /**
    * Adds a custom score expression to the search query.
    *
-   * Each score expression can optionally use a multiplier, and multiple
-   * expressions are combined.
+   * Score expressions are used to order search results. If no calls to
+   * addScore() have taken place, a default keyword relevance score will be
+   * used. However, if at least one call to addScore() has taken place, the
+   * keyword relevance score is not automatically added.
+   *
+   * Also note that if you call orderBy() directly on the query, search scores
+   * will not automatically be used to order search results. Your orderBy()
+   * expression can reference 'calculated_score', which will be the total
+   * calculated score value.
    *
    * @param $score
-   *   The score expression.
+   *   The score expression, which should evaluate to a number between 0 and 1.
+   *   The string 'i.relevance' in a score expression will be replaced by a
+   *   measure of keyword relevance between 0 and 1.
    * @param $arguments
-   *   Custom query arguments for that expression.
+   *   Query arguments needed to provide values to the score expression.
    * @param $multiply
-   *   If set, the score is multiplied with that value. Search query ensures
-   *   that the search scores are still normalized.
+   *   If set, the score is multiplied with this value. However, all scores
+   *   with multipliers are then divided by the total of all multipliers, so
+   *   that overall, the normalization is maintained.
+   *
+   * @return object
+   *   The updated query object.
    */
   public function addScore($score, $arguments = array(), $multiply = FALSE) {
     if ($multiply) {
       $i = count($this->multiply);
+      // Modify the score expression so it is multiplied by the multiplier,
+      // with a divisor to renormalize.
       $score = "CAST(:multiply_$i AS DECIMAL) * COALESCE(( " . $score . "), 0) / CAST(:total_$i AS DECIMAL)";
+      // Add an argument for the multiplier. The :total_$i argument is taken
+      // care of in the execute() method, which is when the total divisor is
+      // calculated.
       $arguments[':multiply_' . $i] = $multiply;
       $this->multiply[] = $multiply;
     }
@@ -446,8 +466,9 @@ class SearchQuery extends SelectQueryExtender {
     }
 
     if (count($this->multiply)) {
-      // Add the total multiplicator as many times as requested to maintain
-      // normalization as far as possible.
+      // Re-normalize scores with multipliers by dividing by the total of all
+      // multipliers. The expressions were altered in addScore(), so here just
+      // add the arguments for the total.
       $i = 0;
       $sum = array_sum($this->multiply);
       foreach ($this->multiply as $total) {
@@ -456,13 +477,20 @@ class SearchQuery extends SelectQueryExtender {
       }
     }
 
-    // Replace i.relevance pseudo-field with the actual, normalized value.
-    $this->scores = str_replace('i.relevance', '(' . (1.0 / $this->normalize) . ' * i.score * t.count)', $this->scores);
-    // Convert scores to an expression.
+    // Replace the pseudo-expression 'i.relevance' with a measure of keyword
+    // relevance in all score expressions, using string replacement. Careful
+    // though! If you just print out a float, some locales use ',' as the
+    // decimal separator in PHP, while SQL always uses '.'. So, make sure to
+    // set the number format correctly.
+    $relevance = number_format((1.0 / $this->normalize), 10, '.', '');
+    $this->scores = str_replace('i.relevance', '(' . $relevance . ' * i.score * t.count)', $this->scores);
+
+    // Add all scores together to form a query field.
     $this->addExpression('SUM(' . implode(' + ', $this->scores) . ')', 'calculated_score', $this->scoresArguments);
 
+    // If an order has not yet been set for this query, add a default order
+    // that sorts by the calculated sum of scores.
     if (count($this->getOrderBy()) == 0) {
-      // Add default order after adding the expression.
       $this->orderBy('calculated_score', 'DESC');
     }
 
diff --git a/modules/search/search.test b/modules/search/search.test
index 289260026..d3b34a3a4 100644
--- a/modules/search/search.test
+++ b/modules/search/search.test
@@ -2036,3 +2036,45 @@ class SearchNodeAccessTest extends DrupalWebTestCase {
     $this->assertText($node->title);
   }
 }
+
+/**
+ * Tests searching with locale values set.
+ */
+class SearchSetLocaleTest extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Search with numeric locale set',
+      'description' => 'Check that search works with numeric locale settings',
+      'group' => 'Search',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('search');
+
+    // Create a simple node so something will be put in the index.
+    $info = array(
+      'body' => array(LANGUAGE_NONE => array(array('value' => 'Tapir'))),
+    );
+    $this->drupalCreateNode($info);
+
+    // Run cron to index.
+    $this->cronRun();
+  }
+
+  /**
+   * Verify that search works with a numeric locale set.
+   */
+  public function testSearchWithNumericLocale() {
+    // French decimal point is comma.
+    setlocale(LC_NUMERIC, 'fr_FR');
+
+    // An exception will be thrown if a float in the wrong format occurs in the
+    // query to the database, so an assertion is not necessary here.
+    db_select('search_index', 'i')
+      ->extend('searchquery')
+      ->searchexpression('tapir', 'node')
+      ->execute();
+  }
+}