Hi all!
I recently found that it is less than clear how numbers should be quoted/escaped in SQL queries. Should DatabaseBase::addQuotes() be used, or rather just inval(), to make sure it's really a number? What's the best practice?
Looking at DatabaseBase::makeList(), it seems that addQuotes() is used on all values, string or not. So, what does addQuotes() actually do? Does it always add quotes, turning the value into a string literal, or does it rather apply whatever quoting/escaping is appropriate for the given data type?
addQuotes' documentation sais:
* If it's a string, adds quotes and backslashes * Otherwise returns as-is
That's a plain LIE. Here's the code:
if ( $s === null ) { return 'NULL'; } else { # This will also quote numeric values. This should be harmless, # and protects against weird problems that occur when they really # _are_ strings such as article titles and string->number->string # conversion is not 1:1. return "'" . $this->strencode( $s ) . "'"; }
So, it actually always returns a quoted string literal, unless $s is null.
But is it true what the comment sais? Is it really always harmless to quote numeric values? Will all database engines always magically convert them to numbers before executing the query? If not, this may be causing table scans. That would be bad - but I suppose someone would have noticed by now...
So... at the very least, addQuotes' documentation needs fixing. And perhaps it would be nice to have an additional method that only applies the appropriate quoting, e.g. escapeValue or some such - that's how addQuotes seems to be currently used, but that's not what it actually does... What do you think?
-- daniel
PS: There's more fun. The DatabaseMssql class overrides addQuotes to support Blob object. For the case $s is a Blob object, this code is used:
return "'" . $s->fetch( $s ) . "'";
The value is used raw, without any escaping. Looks like if there's a ' in the blob, fun things will happen. Or am I missing something?