- Patch #419388 by jhodgdon, David Lesieur, Garrett Albright: search_query_insert() breaks down when the value of key is 0.

2 files changed, 105 insertions, 12 deletions

diff --git a/modules/search/search.module b/modules/search/search.module
index 981256dc4..7f0402c42 100644
--- a/modules/search/search.module
+++ b/modules/search/search.module
@@ -846,26 +846,59 @@ function search_comment_unpublish($comment) {
 }
 
 /**
- * Extract a module-specific search option from a search query. e.g. 'type:book'
+ * Extracts a module-specific search option from a search expression.
+ *
+ * Search options are added using search_expression_insert(), and retrieved
+ * using search_expression_extract(). They take the form option:value, and
+ * are added to the ordinary keywords in the search expression.
+ *
+ * @param $expression
+ *   The search expression to extract from.
+ * @param $option
+ *   The name of the option to retrieve from the search expression.
+ *
+ * @return
+ *   The value previously stored in the search expression for option $option,
+ *   if any. Trailing spaces in values will not be included.
  */
-function search_expression_extract($keys, $option) {
-  if (preg_match('/(^| )' . $option . ':([^ ]*)( |$)/i', $keys, $matches)) {
+function search_expression_extract($expression, $option) {
+  if (preg_match('/(^| )' . $option . ':([^ ]*)( |$)/i', $expression, $matches)) {
     return $matches[2];
   }
 }
 
 /**
- * Return a query with the given module-specific search option inserted in.
- * e.g. 'type:book'.
+ * Adds a module-specific search option to a search expression.
+ *
+ * Search options are added using search_expression_insert(), and retrieved
+ * using search_expression_extract(). They take the form option:value, and
+ * are added to the ordinary keywords in the search expression.
+ *
+ * @param $expression
+ *   The search expression to add to.
+ * @param $option
+ *   The name of the option to add to the search expression.
+ * @param $value
+ *   The value to add for the option. If present, it will replace any previous
+ *   value added for the option. Cannot contain any spaces or | characters, as
+ *   these are used as delimiters. If you want to add a blank value $option: to
+ *   the search expression, pass in an empty string or a string that is composed
+ *   of only spaces. To clear a previously-stored option without adding a
+ *   replacement, pass in NULL for $value or omit.
+ *
+ * @return
+ *   $expression, with any previous value for this option removed, and a new
+ *   $option:$value pair added if $value was provided.
  */
-function search_expression_insert($keys, $option, $value = '') {
-  if (search_expression_extract($keys, $option)) {
-    $keys = trim(preg_replace('/(^| )' . $option . ':[^ ]*/i', '', $keys));
-  }
-  if ($value != '') {
-    $keys .= ' ' . $option . ':' . $value;
+function search_expression_insert($expression, $option, $value = NULL) {
+  // Remove any previous values stored with $option.
+  $expression = trim(preg_replace('/(^| )' . $option . ':[^ ]*/i', '', $expression));
+
+  // Set new value, if provided.
+  if (isset($value)) {
+    $expression .= ' ' . $option . ':' . trim($value);
   }
-  return $keys;
+  return $expression;
 }
 
 /**
diff --git a/modules/search/search.test b/modules/search/search.test
index ded36a993..df76e4a23 100644
--- a/modules/search/search.test
+++ b/modules/search/search.test
@@ -847,6 +847,66 @@ class SearchCommentTestCase extends DrupalWebTestCase {
 }
 
 /**
+ * Tests search_expression_insert() and search_expression_extract().
+ *
+ * @see http://drupal.org/node/419388 (issue)
+ */
+class SearchExpressionInsertExtractTestCase extends DrupalUnitTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Search expression insert/extract',
+      'description' => 'Tests the functions search_expression_insert() and search_expression_extract()',
+      'group' => 'Search',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('search');
+  }
+
+  /**
+   * Tests search_expression_insert() and search_expression_extract().
+   */
+  function testInsertExtract() {
+    $base_expression = "mykeyword";
+    // Build an array of option, value, what should be in the expression, what
+    // should be retrieved from expression.
+    $cases = array(
+      array('foo', 'bar', 'foo:bar', 'bar'), // Normal case.
+      array('foo', NULL, '', NULL), // Empty value: shouldn't insert.
+      array('foo', ' ', 'foo:', ''), // Space as value: should insert but retrieve empty string.
+      array('foo', '', 'foo:', ''), // Empty string as value: should insert but retrieve empty string.
+      array('foo', '0', 'foo:0', '0'), // String zero as value: should insert.
+      array('foo', 0, 'foo:0', '0'), // Numeric zero as value: should insert.
+    );
+
+    foreach ($cases as $index => $case) {
+      $after_insert = search_expression_insert($base_expression, $case[0], $case[1]);
+      if (empty($case[2])) {
+        $this->assertEqual($after_insert, $base_expression, "Empty insert does not change expression in case $index");
+      }
+      else {
+        $this->assertEqual($after_insert, $base_expression . ' ' . $case[2], "Insert added correct expression for case $index");
+      }
+
+      $retrieved = search_expression_extract($after_insert, $case[0]);
+      if (!isset($case[3])) {
+        $this->assertFalse(isset($retrieved), "Empty retrieval results in unset value in case $index");
+      }
+      else {
+        $this->assertEqual($retrieved, $case[3], "Value is retrieved for case $index");
+      }
+
+      $after_clear = search_expression_insert($after_insert, $case[0]);
+      $this->assertEqual(trim($after_clear), $base_expression, "After clearing, base expression is restored for case $index");
+
+      $cleared = search_expression_extract($after_clear, $case[0]);
+      $this->assertFalse(isset($cleared), "After clearing, value could not be retrieved for case $index");
+    }
+  }
+}
+
+/**
  * Tests that comment count display toggles properly on comment status of node
  * 
  * Issue 537278