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.

Files: 
CommentFileSizeAuthor
#10 database-sqlite.patch575 bytesToletum
#7 like-escape-3.patch8.26 KBc960657
Passed on all environments.
[ View ]
#4 like-escape-2.patch6.66 KBc960657
Passed on all environments.
[ View ]
like-escape-1.patch7.53 KBc960657
Passed on all environments.
[ View ]

Comments

Crell’s picture

Is explicitly specifying the escape character a part of the SQL standard or just a MySQL/Postgres/SQLite thing?

c960657’s picture

Crell’s picture

Status:Needs review» Needs work

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.

c960657’s picture

Status:Needs work» Needs review
StatusFileSize
new6.66 KB
Passed on all environments.
[ View ]

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.

Crell’s picture

Status:Needs review» Needs work

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.

catch’s picture

Priority:Normal» Critical

Subscribing. Since this blocks the LOWER() issue, also bumping priority.

c960657’s picture

Status:Needs work» Needs review
StatusFileSize
new8.26 KB
Passed on all environments.
[ View ]

Renamed to db_like() and added escapeLike(). I added an example to the docblock.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

I like (no pun intended).

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

Toletum’s picture

StatusFileSize
new575 bytes

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.

Crell’s picture

Toletum, this issue is fixed. Please file it as a new issue with an appropriate title. Thanks.

Status:Fixed» Closed (fixed)

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