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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Title: "function sum(record) does not exist" on search results page with PostgreSQL » Search 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:

    // 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);
plachance’s picture

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

	setlocale(LC_ALL, 'fr_CA', 'fr_CA.utf8');

Thanks.

jhodgdon’s picture

Status: Active » Closed (duplicate)
jhodgdon’s picture

Status: Closed (duplicate) » Active

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

jhodgdon’s picture

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.

naxoc’s picture

Assigned: Unassigned » naxoc

I'll write a test for this.

naxoc’s picture

Status: Active » Needs review
FileSize
3.26 KB
1.79 KB

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.

jhodgdon’s picture

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?

naxoc’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
1.79 KB
3.27 KB

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.

jhodgdon’s picture

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.

naxoc’s picture

FileSize
1.99 KB

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.

jhodgdon’s picture

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

jhodgdon’s picture

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. :(

naxoc’s picture

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.

jhodgdon’s picture

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?

jhodgdon’s picture

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

naxoc’s picture

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.

naxoc’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

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

jhodgdon’s picture

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

naxoc’s picture

Yay testbot! That was quick :)

Here are both patches again.

Status: Needs review » Needs work

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

jhodgdon’s picture

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

Seems a bit odd...

naxoc’s picture

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

naxoc’s picture

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

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

naxoc’s picture

#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.

jhodgdon’s picture

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

naxoc’s picture

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.

naxoc’s picture

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.

jhodgdon’s picture

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.

naxoc’s picture

Status: Needs work » Needs review
FileSize
611 bytes
3.36 KB

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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.89 KB

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.

jhodgdon’s picture

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.

naxoc’s picture

Hooraay! Thanks for being patient jhodgdon :)

jhodgdon’s picture

I also just added an issue summary.

catch’s picture

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?

naxoc’s picture

Added a little more explanation to the comment.

jhodgdon’s picture

FileSize
4.63 KB
7.05 KB

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.

jhodgdon’s picture

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?

naxoc’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

naxoc’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.41 KB
1.41 KB

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.

jhodgdon’s picture

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?

naxoc’s picture

Status: Needs work » Needs review
FileSize
6.07 KB

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

jhodgdon’s picture

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.

jhodgdon’s picture

Issue summary: View changes

create summary

David_Rothstein’s picture

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.