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

core/modules/search/lib/Drupal/search/SearchQuery.php

@@ -112,6 +112,8 @@ class SearchQuery extends SelectExtender {
    * Stores score expressions.
    *
    * @var array
+   *
+   * @see addScore()
    */
   protected $scores = array();
 
@@ -123,7 +125,7 @@ class SearchQuery extends SelectExtender {
   protected $scoresArguments = array();
 
   /**
-   * Total value of all the multipliers.
+   * Stores multipliers for score expressions.
    *
    * @var array
    */
@@ -372,16 +374,26 @@ class SearchQuery extends SelectExtender {
   /**
    * 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.
@@ -389,7 +401,12 @@ class SearchQuery extends SelectExtender {
   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;
     }
@@ -413,6 +430,7 @@ class SearchQuery extends SelectExtender {
    */
   public function execute()
   {
+
     if (!$this->executedFirstPass) {
       $this->executeFirstPass();
     }
@@ -430,8 +448,9 @@ class SearchQuery extends SelectExtender {
     }
 
     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) {
@@ -440,13 +459,20 @@ class SearchQuery extends SelectExtender {
       }
     }
 
-    // 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');
     }
 
@@ -455,7 +481,6 @@ class SearchQuery extends SelectExtender {
       ->addTag('search_' . $this->type)
       ->addMetaData('normalize', $this->normalize)
       ->fields('i', array('type', 'sid'));
-
     return $this->query->execute();
   }
 

core/modules/search/lib/Drupal/search/Tests/SearchSetLocaleTest.php

@@ -0,0 +1,61 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\search\Tests\SearchSetLocaleTest.
+ */
+
+namespace Drupal\search\Tests;
+
+/**
+ * Tests searching with locale values set.
+ */
+class SearchSetLocaleTest extends SearchTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array('comment');
+
+  /**
+   * A node search plugin instance.
+   *
+   * @var \Drupal\search\Plugin\SearchInterface
+   */
+  protected $nodeSearchPlugin;
+
+  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();
+
+    // Create a plugin instance.
+    $this->nodeSearchPlugin = $this->container->get('plugin.manager.search')->createInstance('node_search');
+    // Create a node with a very simple body.
+    $this->drupalCreateNode(array('body' => array(array('value' => 'tapir'))));
+    // Update the search index.
+    $this->nodeSearchPlugin->updateIndex();
+    search_update_totals();
+  }
+
+  /**
+   * Verify that search works with a numeric locale set.
+   */
+  public function testSearchWithNumericLocale() {
+    // French decimal point is comma.
+    setlocale(LC_NUMERIC, 'fr_FR');
+    $this->nodeSearchPlugin->setSearch('tapir', array(), array());
+    // The call to execute will throw an exception if a float in the wrong
+    // format is passed in the query to the database, so an assertion is not
+    // necessary here.
+    $this->nodeSearchPlugin->execute();
+  }
+}