| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
This is a spin-off from #279851: Replace LOWER() with db_select() and LIKE() where possible:
The following can be used to do a case-insensitive search for the specified value with the query builder:->condition('foo', $foo, 'LIKE')
However, if $foo contains a wildcard character, % or _ we need to escape these characters. In MySQL, we can do this by inserting a backslash in front of the wildcard character, e.g. using addcslashes($foo, '\%_'). AFAICT SQL-92 says that there is no escape character and that backslash should be treated like any other character, but this is only enforced in the NO_BACKSLASH_ESCAPES mode.
In order to ensure consistent behaviour across databases we need to explicitly specify an escape character in our SQL, i.e. like this:SELECT * FROM bar WHERE foo LIKE 'abc\\_def' ESCAPE '\\'
Though any character can be used, I suggest using backslash as this is the default on MySQL and PostgreSQL.
This patch has only been tested with MySQL.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| like-escape-1.patch | 7.53 KB | Idle | Passed on all environments. | View details |
Comments
#1
Is explicitly specifying the escape character a part of the SQL standard or just a MySQL/Postgres/SQLite thing?
#2
It is part of SQL-92 (see section 8.2):
http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt
It is also supported on Oracle and MS SQL:
http://www.praetoriate.com/oracle_tips_escape_characters.htm
http://msdn.microsoft.com/en-us/library/ms179859.aspx
#3
Hm, OK. Yay for standards.
1) Is that the right escaping? Aren't we then using \\ as the escape character rather than \? (On Postgres, since MySQL isn't being modified here.)
2) The unit test should be split into two tests. The double-insert can be folded into a single query, too. (Not a huge deal but may as well.)
3) Are we really talking about making "use addcslashes() first" part of the DB API? That makes me very uneasy. It also has a nice subtle change in PHP 5.2.5. (http://us.php.net/addcslashes). Can we not resolve that in a cleaner fashion? I can easily see that being something that people forget on a regular basis and introduce subtle bugs or security holes.
#4
Re: 1
I think \\\\ is right. If the ESCAPE string is > 1 one character, I get following error: SQLSTATE[HY000]: General error: 1210 Incorrect arguments to ESCAPE.
The following outputs a single backslash:
print db_query("SELECT '\\\\'")->fetchField();Re: 2
Done.
Re: 3
I have created db_escape_like() for this purpose. AFAICT the change in 5.2.5 is only of concern if the second argument for addclashes() contains "\v" or "\f", and that is not the case here. If people forget to call this function, I don't think there is much we can do about it if we still want to allow wildcard searches. At least not without making a query-builder-like mechanism for LIKE patterns.
#5
db_escape_like() should be a wrapper around a driver method, like the rest of the system. Any actual logic belongs in a method that can be overridden if necessary.
I'd personally prefer db_like() as a function name to match db_and(), db_or(), and so forth. Not a deal breaker, I suppose, just personal preference. The method could arguably be escapeLike().
SQL is weird. :-) Actually it's escaping stuff that's weird, because we're still escaping stuff a dozen times. Bah. At least it's then handled mostly internally rather than exposing triple escaping to each module dev like we did before with %%%s%% and similar silliness.
I wonder how we should document this, though? It should be put in a docblock somewhere, but I'm not really sure where.
#6
Subscribing. Since this blocks the LOWER() issue, also bumping priority.
#7
Renamed to db_like() and added escapeLike(). I added an example to the docblock.
#8
I like (no pun intended).
#9
I glanced through this briefly. According to the tests, this should be a transparent change to module developers, since it's just switching around how the condition is done internally.
Committed to HEAD. Thanks!
#10
I have found a problem with drupal-7 & sqlite. The error is: "1 ESCAPE expression must be a single character."
I have done a patch.
Regards,
Toletum.
#11
Toletum, this issue is fixed. Please file it as a new issue with an appropriate title. Thanks.
#12
Automatically closed -- issue fixed for 2 weeks with no activity.