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.
Related Issues
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.
Comment | File | Size | Author |
---|---|---|---|
#51 | drupal7.search-module.2016497-49.patch | 6.07 KB | naxoc |
#49 | drupal7.search-module.2016497-49-test-only.patch | 1.41 KB | naxoc |
#49 | drupal7.search-module.2016497-49-test-only.patch | 1.41 KB | naxoc |
#45 | 2016497-45.patch | 7.05 KB | jhodgdon |
#45 | interdiff.txt | 4.63 KB | jhodgdon |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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:Comment #2
plachance CreditAttribution: plachance commentedI've found the culprit in one of my modules.
Thanks.
Comment #3
jhodgdonThis is a duplicate of #1145964: search module running into comma as decimal place problem
Comment #4
jhodgdonActually, this module has a better solution, so I'm going to mark the other one as a duplicate.
Comment #5
jhodgdonI 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.
Comment #6
naxoc CreditAttribution: naxoc commentedI'll write a test for this.
Comment #7
naxoc CreditAttribution: naxoc commentedHere is a test and a fix for the broken DB query.
Comment #9
jhodgdonHm. 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?
Comment #10
naxoc CreditAttribution: naxoc commentedI 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.
Comment #11
jhodgdonYeah 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.
Comment #12
naxoc CreditAttribution: naxoc commentedThe 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!
Comment #14
jhodgdonYeah, 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
Comment #15
jhodgdonInteresting. 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. :(
Comment #16
naxoc CreditAttribution: naxoc commentedHm, 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.
Comment #17
jhodgdonOh 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?
Comment #18
jhodgdonOr possibly we need to ask the test bot maintainers to install a language pack.
Comment #19
naxoc CreditAttribution: naxoc commentedThe 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.
Comment #20
naxoc CreditAttribution: naxoc commentedI 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.
Comment #21
jhodgdonLooks good. Let's at least make sure the patch works. Hopefully that issue on the test bots will get resolved sometime soon.
Comment #22
jhodgdonThe 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.
Comment #23
jhodgdon#12: 2016497-12.test-only.patch queued for re-testing.
Comment #24
jhodgdonThat patch in #12 now fails in the way we had hoped, exposing the bug that is reported here. !!
Comment #25
naxoc CreditAttribution: naxoc commentedYay testbot! That was quick :)
Here are both patches again.
Comment #27
jhodgdonThe actual patch is now failing with:
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "comment.manager" does not exist.
Seems a bit odd...
Comment #28
naxoc CreditAttribution: naxoc commentedThat is odd. I'll see if it was a freak fail.
Comment #29
naxoc CreditAttribution: naxoc commented#25: 2016497-25.test-only.patch queued for re-testing.
Comment #30
naxoc CreditAttribution: naxoc commented#25: 2016497-25.patch queued for re-testing.
Comment #32
jhodgdonSame test failure... Is the comment module perhaps not enabled in this new test?
Comment #33
naxoc CreditAttribution: naxoc commentedWeird. 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.
Comment #34
naxoc CreditAttribution: naxoc commented#25: 2016497-25.patch queued for re-testing.
Comment #36
jhodgdonSearchTestBase has:
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:
So how about trying that? It seems likely that the problem here is that comment isn't properly enabled.
Comment #37
naxoc CreditAttribution: naxoc commentedHere 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.
Comment #38
jhodgdonThat 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.
Comment #40
jhodgdonOK. 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.
Comment #41
naxoc CreditAttribution: naxoc commentedHooraay! Thanks for being patient jhodgdon :)
Comment #42
jhodgdonI also just added an issue summary.
Comment #43
catchThis says that the db query will fail without the formatting, but not why (the comma used in some locales?) - could we add that?
Comment #44
naxoc CreditAttribution: naxoc commentedAdded a little more explanation to the comment.
Comment #45
jhodgdonActually... 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.
Comment #46
jhodgdonThe 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?
Comment #47
naxoc CreditAttribution: naxoc commentedI absolutely think the added comments is a win. Setting to RTBC :)
Comment #48
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for backport.
Comment #49
naxoc CreditAttribution: naxoc commentedHere 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.
Comment #50
jhodgdonUm... 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?
Comment #51
naxoc CreditAttribution: naxoc commentedHuh. Must be me that screwed the uploads up. Here is the full patch.
Comment #52
jhodgdonI 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.
Comment #52.0
jhodgdoncreate summary
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/66a58e4