City Search breaks when spaces are in cities names

This is a very huge issue and I can confirm this on the latest 3.0 version of the module. It affects any city with a space in it's name!

Marking as critical since it breaks the normal operation of the location_search module.

retitling

I can confirm that spaces in cities cause the 'key' method of searching to fail, as the location_search module uses the search module to do the search in this mode I suspect that the problem lies in the the search_query_extract() function which treats a space as the end of the query (or that part of it). Quotes both single and double have no effect. The simple search method works better ;-(

Looking at the search_query_extract() function the preg_match function does indeed end the key-value on space or end-of-line

So this in my opinion is a core module problem I'm sorry to say.

Perhaps the search module maintainers should be notified of this bug.

It is true that search_expression_insert/extract (D7) and the D6 versions search_query_insert/extract have no provision for spaces.

Making them take spaces is not going to happen in Drupal 6 for sure, and it's probably also too late for Drupal 7.

On the other hand, the Location module could be encoding its query without spaces, in order to use these functions.

Oh, and just as a note, there's proposed new documentation for these functions (which will make it clear that the value cannot contain a space) on this issue for Drupal 7:
#419388: search_query_insert breaks down when the value of key is 0
Once D7 is fixed, we'll port to D6 as well.

Thanks!

FYI I closed #635198: City search values return bogus results with multi-word Cities e.g (Fort Worth) as it was a direct duplicate of this issue.

So just to confirm - city search will not work if there are spaces in the city, correct?

Yes I can confirm that was what I found in my testing.

I have found a quick fix for this but would like input as to whether this is a good approach? The fix is to convert all spaces to underscores before inserting a search key:

 // location_search.module
function location_search_validate($form, &$form_state) {
// ... line 294
  if (!empty($values['city'])) {
    $keys = search_query_insert($keys, 'city', str_replace(' ', '_', $values['city']));
  }

Then reverse the process when extracting the key

 // location_search.module
function _location_search($op = 'search', $keys = null) {
// ... line 93
      if ($city = search_query_extract($keys, 'city')) {
        $city = str_replace('_', ' ', city);
        $conditions1 .= " AND (l.city = '%s')";
        $arguments1[] = $city;
        $keys = search_query_insert($keys, 'city');
      }

If others think this is a viable solution I would be happy to roll a patch. Underscore seems to be the only character that does not need to be encoded, is not used in city names, and does not break anything.

I was rolling patches for other issues so I figured I would roll one for this and bump the issue to needs review.

Oops.. Patch is against 6.x-3.x-dev so I guess the issue should be too.

tinker, why underscores? Couldn't we do something more elegant, like HTML Entity or URL Encode/Decode? This solution probably doesn't account for other character issues is my guess. Although this issue is for "spaces" in particular, I bet other characters can be troublesome too.

Actually, spaces are special. The problem is that the search module assumes that a space denotes the end of a special query thing added via search_query_insert(). See comments above for more details.

Well if that is the case, your patch should account for people trying to put underscores into the location name, by disallowing it shouldn't it? Otherwise we could end up with unintentional replacements.

HTML entity does not encode spaces so does not work. URL Encode does not work because query keys are already url encoded and decoded when put or grabbed from the URL. URL encode cannot be used twice as the 2nd time does nothing. This means that encoded spaces end up decoded into normal spaces by the time search_query_extract gets them.

'-' does not work since it is used in city names
'+' is decoded to space before search_query_extract so does not work
':' is already used as a key identifier, could use '::' but that could lead to problems
All other characters need encoding or are already used.

@nicholas.alipaz You are correct; there could be a potential problem if underscores are used in the city name. This would result in them being converted to spaces. This would not break the query key insert or extract but might not return the correct results. I have looked at a few global city databases and did not find underscores in any of the city names. Currently the city search does not work in a large number of cases. Using underscores would fail in a few cases. This would still be a major improvement. I agree that city names should be filtered to not allow underscores but don't have the time to do this. This patch is only two lines so could be included without breaking anything. The filtering could be added later.

IMHO this is a bug in search_query_extract() but getting that changed will not be easy. Limiting search keys to one word does not seem to make sense? On top of that search_query_insert() doesn't check if the key name or value contains colons or spaces which would mess everything up.

If key names were forced to have no colons or spaces then regex could be changed to (one space removed in 2nd preg match parenthesis).

function search_query_extract($keys, $option) {
//if (preg_match('/(^| )'. $option .':([^: ]*)( |$)/i', $keys, $matches)) {
  if (preg_match('/(^| )'. $option .':([^:]*)( |$)/i', $keys, $matches)) {
    return $matches[2];
  }
}

$keys = 'foo bar city:San Francisco province:CA';
$city =  search_query_extract($keys, 'city');
// $city = 'San Francisco'

bot recall

bot

#12 commited pushed to 6.x-3.x-dev
Thanks!