Updated: Comment #42

Problem/Motivation

The core node search query uses PHP print to get floating-point numbers into search queries. This method does not work for some locales, which use commas as the decimal point separator in PHP but not in SQL.

Proposed resolution

Change the search query code so that floating-point numbers are always output with . as the separator so they will work with SQL queries.

Remaining tasks

[All tasks are done.]
1. Make a test that illustrates the problem. This involved also getting a French locale added to the test bot servers.
2. Fix the problem.

User interface changes

Search with rankings will work in non-English locales that use , as the decimal point separator.

API changes

None.

This has been reported several times with duplicate issues, including:
#1145964: search module running into comma as decimal place problem

Issue that added the locale on the testbot:
#2098647: Install locales that use something else than period as decimal separator

Original report by plachance

I'm getting this error message when a visitor use the search bar.

PDOException : SQLSTATE[42883]: Undefined function: 7 ERROR: function sum(record) does not exist LINE 1: SELECT i.type AS type, i.sid AS sid, SUM((40,300317969509 * ... ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts.: SELECT i.type AS type, i.sid AS sid, SUM((40,300317969509 * i.score * t.count)) AS calculated_score FROM {search_index} i INNER JOIN {node} n ON n.nid = i.sid INNER JOIN {search_total} t ON i.word = t.word INNER JOIN {search_dataset} d ON i.sid = d.sid AND i.type = d.type WHERE (n.status = :db_condition_placeholder_0) AND( (i.word = :db_condition_placeholder_1) )AND (i.type = :db_condition_placeholder_2) AND( (d.data ILIKE :db_condition_placeholder_3) ) GROUP BY i.type, i.sid HAVING (COUNT(*) >= :matches) ORDER BY calculated_score DESC LIMIT 10 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => test [:db_condition_placeholder_2] => node [:db_condition_placeholder_3] => % test % [:matches] => 1 ) dans PagerDefault->execute() (ligne 79 dans includes/pager.inc).

If the search return no results, then the usual "no result found" page is shown.

Tested on PostgreSQL 8.3 and 9.1, Drupal 7.17 and 7.22.

Files: 
CommentFileSizeAuthor
#51 drupal7.search-module.2016497-49.patch6.07 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 40,450 pass(es).
[ View ]
#49 drupal7.search-module.2016497-49-test-only.patch1.41 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 40,257 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#49 drupal7.search-module.2016497-49-test-only.patch1.41 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 40,428 pass(es).
[ View ]
#45 2016497-45.patch7.05 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 59,250 pass(es).
[ View ]
#45 interdiff.txt4.63 KBjhodgdon
#44 2016497-44-test-only.patch1.89 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 59,396 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#44 interdiff.txt879 bytesnaxoc
#44 2016497-44.patch3.45 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]
#38 2016497-37-test-only.patch1.89 KBjhodgdon
FAILED: [[SimpleTest]]: [MySQL] 59,105 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#37 2016497-37.patch3.36 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 59,012 pass(es).
[ View ]
#37 interdiff.txt611 bytesnaxoc
#25 2016497-25.test-only.patch1.79 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 58,855 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#25 2016497-25.patch3.26 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 58,553 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#20 drupal8.search-module.2016497-20.patch1.47 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,435 pass(es).
[ View ]
#12 2016497-12.test-only.patch1.99 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 58,842 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#10 2016497-7.test-only.patch3.27 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 59,096 pass(es).
[ View ]
#10 2016497-7.patch1.79 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,577 pass(es).
[ View ]
#10 interdiff.txt1.7 KBnaxoc
#7 2016497-7.test-only.patch1.79 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,780 pass(es).
[ View ]
#7 2016497-7.patch3.26 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 58,974 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

Comments

Title:"function sum(record) does not exist" on search results page with PostgreSQLSearch query extender should not use floats directly
Version:7.22» 8.x-dev
Priority:Major» Normal

This is likely reproducible in Drupal 8, and it is not technically a bug.

There is something in your installation that messes up with the locale settings. Some modules are known to do that (Gallery2?).

We can actually make that a little bit more robust, so leaving this so that we make this code from SearchQuery.php not dependent on the locale settings:

<?php
   
// 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);
?>

I've found the culprit in one of my modules.

<?php
    setlocale
(LC_ALL, 'fr_CA', 'fr_CA.utf8');
?>

Thanks.

Status:Active» Closed (duplicate)

Status:Closed (duplicate)» Active

Actually, this module has a better solution, so I'm going to mark the other one as a duplicate.

Priority:Normal» Major
Issue tags:+Needs tests, +needs backport to D7

I think this is coming up fairly frequently since many foreign locales use commas as decimal points. Bumping status. Should actually be an easy fix... but it will definitely need a test as well.

Assigned:Unassigned» naxoc

I'll write a test for this.

Status:Active» Needs review
StatusFileSize
new3.26 KB
FAILED: [[SimpleTest]]: [MySQL] 58,974 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,780 pass(es).
[ View ]

Here is a test and a fix for the broken DB query.

Status:Needs review» Needs work

The last submitted patch, 2016497-7.patch, failed testing.

Hm. The test-only patch passed, while the fix patch failed (the failures were in a search-related test, so I think they are probably valid).

So:
- That test must not be testing the real problem. We need a test that fails without the patch. Maybe you could try using one of the locale settings that was reported here or on the other issue in #3 to fail, rather than Danish?
- The fix must have some problems in it too. I think the problem is that if you use number_format, you probably do *not* want to have a thousands separator, as that could introduce commas. I think you also want to set the number of decimal points to something like 10, not zero?

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,577 pass(es).
[ View ]
new3.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,096 pass(es).
[ View ]

I think the non-failing test was not failing because Danish locale is not installed on the test server. It worked locally for me, but I changed locale to Canadian French.

The number of decimals should be higher so I set that from 0 to 10. As for thousands separator I think that will never happen. The number should always be between 0 and 1. The reason I supplied the comma value is that PHP is a little weird with that function. The docs for setlocale say: "This function accepts either one, two, or four parameters (not three)"
But if there is a prettier way to get the number formatted we should totally do that.

Yeah you need to supply a parameter, but how about '' for the thousands separator? :)

I realize the number *should* be between 0 and 1, but in the case from that other issue, and above as well, it wasn't. Some of these things come from contrib modules supplying rankings... so let's not assume anything we don't have to.

Anyway, let's see how the tests do this time. The code here and the test is very clear and has good documentation, so assuming the test-only fails and the test-plus-code passes, I think ... aside from changing the last argument of format_number to '' instead of ',', I think this will be ready to go.

StatusFileSize
new1.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,842 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

The test not failing is totally weird. I wonder if it is still the locales.

This patch has some code that is just testing if locales are set on the testbot. Ignore patch. Sorry bot!

Status:Needs review» Needs work

The last submitted patch, 2016497-12.test-only.patch, failed testing.

Yeah, the test failure message is "Failed to set locale."... It looks like all kinds of things can go wrong:
http://php.net/manual/en/function.setlocale.php

Interesting. When I run "locale -a" on my Ubuntu command line, it tells me the only supported locales are en_* basically. So probably the same thing is true of the test bot server.

We may not be able to do this test. :(

Hm, if we can't set locale the problem can't be reproduced. When I run this locally it works, so I'll see if I can figure out a way to make the testbot set locale.

Oh wait.
cat /usr/share/i18n/SUPPORTED

This gives me a bunch of options, such as:
fr_FR.UTF-8 UTF-8
fr_FR ISO-8859-1
fr_FR@euro ISO-8859-15

So you might try
setlocale(LC_NUMERIC, 'fr_FR', 'fr_FR.UTF-8', 'fr_FR.utf8', 'fr_FR@euro')
[and maybe some other similar options from the php.net page comments] and then check to see that the first 5 characters of the return value match 'fr_FR'. That might work?

Or possibly we need to ask the test bot maintainers to install a language pack.

The patch in #13 uses fr_FR. I'm searching for an English language that uses something else than period for decimals.

But it would probably be an OK idea to have a language pack installed on the testbot either way.

StatusFileSize
new1.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,435 pass(es).
[ View ]

I opened an issue on the testbot: #2098647: Install locales that use something else than period as decimal separator

I couldn't really find an English language with comma as separator and the locale for that is probably not installed anyway.

In the meantime - here is a patch that does not use ',' as thousands separator.

Status:Needs work» Needs review

Looks good. Let's at least make sure the patch works. Hopefully that issue on the test bots will get resolved sometime soon.

The testbot issue has been quickly resolved! I'm going to hit retest on the patch in #12. It should fail, but not on the "failed to set locale" line.

#12: 2016497-12.test-only.patch queued for re-testing.

That patch in #12 now fails in the way we had hoped, exposing the bug that is reported here. !!

StatusFileSize
new3.26 KB
FAILED: [[SimpleTest]]: [MySQL] 58,553 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new1.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,855 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Yay testbot! That was quick :)

Here are both patches again.

Status:Needs review» Needs work

The last submitted patch, 2016497-25.patch, failed testing.

The actual patch is now failing with:
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "comment.manager" does not exist.

Seems a bit odd...

That is odd. I'll see if it was a freak fail.

Status:Needs work» Needs review
Issue tags:-Needs tests, -needs backport to D7

#25: 2016497-25.test-only.patch queued for re-testing.

#25: 2016497-25.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

The last submitted patch, 2016497-25.patch, failed testing.

Same test failure... Is the comment module perhaps not enabled in this new test?

Weird. The test works fine for me locally. More locale weirdness?

The comment module should have nothing to do with this. The test inherits from SearchTestBase like most of the other search tests.

Sorry testbot -you must hate me, but have one more go.

Status:Needs work» Needs review
Issue tags:-Needs tests, -needs backport to D7

#25: 2016497-25.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

The last submitted patch, 2016497-25.patch, failed testing.

SearchTestBase has:

public static $modules = array('node', 'search', 'dblog');

Maybe it needs to have 'comment' put in explicitly -- could you try putting that into the Locale test? Actually, one of the other search tests that uses comments (SearchCommentCountToggleTest) has:
  /**
   * Modules to enable.
   *
   * @var array
   */
  public static $modules = array('comment');
  // Requires node types, comment config, filter formats.
  protected $profile = 'standard';

So how about trying that? It seems likely that the problem here is that comment isn't properly enabled.

Status:Needs work» Needs review
StatusFileSize
new611 bytes
new3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,012 pass(es).
[ View ]

Here is a test that enables the comment module. Let's see if that is enough. The tests run fine on my local machine without the comments module enabled. Not sure if I should start worrying about the my machine instead.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.89 KB
FAILED: [[SimpleTest]]: [MySQL] 59,105 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

That seems to have done it!

I guess for completeness we should upload a test-only patch with the change in #37 and verify that it still fails. Here's that part of the patch.

Assuming that the test-only patch attached here still fails, we definitely have a valid test for the actual bug people are seeing, and an easily-understandable fix for the bug that makes the test pass. So, I'm setting this to RTBC in anticipation. Of course, after this test fails we will need to fix the status again.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 2016497-37-test-only.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

OK. The test-only patch in #38 failed with the error that reproduces the bug reported here, and the with-fix patch in #37 passes. I have carefully reviewed both and recommend that we commit it ASAP to fix this long-standing bug.

Hooraay! Thanks for being patient jhodgdon :)

I also just added an issue summary.

Status:Reviewed & tested by the community» Needs review

+++ b/core/modules/search/lib/Drupal/search/SearchQuery.php
@@ -440,8 +441,12 @@ public function execute()
+    // query will not fail.

This says that the db query will fail without the formatting, but not why (the comma used in some locales?) - could we add that?

StatusFileSize
new3.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]
new879 bytes
new1.89 KB
FAILED: [[SimpleTest]]: [MySQL] 59,396 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Added a little more explanation to the comment.

StatusFileSize
new4.63 KB
new7.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,250 pass(es).
[ View ]

Actually... This comment doesn't really explain why we are printing out a float in the first place to put in a query, which is kind of an odd thing to do. So, I've added a bunch of documentation.

I did not alter the test in any way, so I didn't upload the test-only patch this time.

The code in the last two patches is still RTBC, as it has not changed since our earlier patches.

naxoc - I think if you review my newly-added documentation in #45, you can set the issue back to RTBC or needs work, depending on your thoughts?

Status:Needs review» Reviewed & tested by the community

I absolutely think the added comments is a win. Setting to RTBC :)

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,428 pass(es).
[ View ]
new1.41 KB
FAILED: [[SimpleTest]]: [MySQL] 40,257 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Here is a reroll for D7. The SearchQuery class has not changed too much between D7 and D8, so the documentation comments jhodgdon put in the D8 were relevant for D7 too, so I put them in the patch.

Status:Needs review» Needs work

Um... Where's the real patch? Both of these currently say "test-only", and both seem only to have a test in them. Weird file upload glitch?

Status:Needs work» Needs review
StatusFileSize
new6.07 KB
PASSED: [[SimpleTest]]: [MySQL] 40,450 pass(es).
[ View ]

Huh. Must be me that screwed the uploads up. Here is the full patch.

Status:Needs review» Reviewed & tested by the community

I doubt it was you, because the identical test-only patch supposedly failed once and passed once. I have seen this type of glitch on drupal.org uploads a lot recently, like patches ending up garbled or with nothing in them. I wouldn't blame yourself. :)

Anyway, assuming that the test bot comes back green in #51 for Drupal 7, this looks like a straightforward port of the D8 patch, and we should get this into D7. The test-only patch failed as we expected in #49, illustrating the reported problem, and assuming this test passes, it confirms the bug has gone away.

Issue summary:View changes

create summary

Issue summary:View changes
Status:Reviewed & tested by the community» Fixed
Issue tags:+7.25 release notes

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.