wpdb::_weak_escape() is an alias to addslashes only

[hakre]

esc_sql has been introduced for completeness (esc_* named functions; [11490]). time has showed that next to the function naming game (offering kind of an API without an API definition) nothing more has evolved since then. so it's a fact that it does not properly escape SQL, it's just an alias (one with many lines of code for such a thing) for addslashes with it's meaning to add slashes.

the code can better reflect that with a simple change.

[hakre]

same API but easier to read and to improve in the future.

[nacin]

esc_sql() is not an alias for addslashes(). It is an alias for $wpdb->escape(), big difference:

Anyone using a drop-in wpdb replacement would suddenly find their SQL unescaped.

wpdb->escape() supports arrays.

This change might cause some developers to simply use addslashes() instead of esc_sql(). $wpdb->escape() used to be set up to do more than addslashes(), and may evolve in the future. (Also take a look at the _escape and _real_escape methods.) Thus, definitely not easier to improve in the future.

[nacin]

Anyone using a drop-in wpdb replacement would suddenly find their SQL unescaped.

Should be:
... would suddenly find their SQL escaped in what may be a method that differs from what their drop-in specifies.

[dd32]

Side note: Array support was added to wpdb::escape() in WordPress 2.8

[nacin]

Additional side note:

mysql_escape_string was removed in [1545]. mysql_real_escape_string was added in [2684], removed in [2737], conditionally added in [10597], partially removed in [10604].

[hakre]

FYI 1: from ​wordpress codex:

esc_sql( $text ) (since 2.8) 
$wpdb->escape( $text ) 
Escapes a single string for use in a SQL query. Glorified addslashes().

FYI 2: from esc_sql function documentation:

/**
 * Escapes data for use in a MySQL query
 *
 * This is just a handy shortcut for $wpdb->escape(), for completeness' sake
 *
 * @since 2.8.0
 * @param string $sql Unescaped SQL data
 * @return string The cleaned $sql
 */

I must admit that I did not whink about the possibility to replace WPDB with an own implementation right. So the error is "only" in the default implementation and therefore changing esc_sql() is wrong.

It is naturally in the default (not overwritten) implementation, function wpdb::_weak_escape() is the alias to addslashes(). So that's the place where it must be corrected then.

[hakre]

Updated patch, reflects the correct position where the alias was in place. Preserves compability with third-party WPDB implementations.

Additionally the function has benefitted from the re-work, unnecessary if clause could be removed.

[hakre]

And the next iteration. I do not see much more this function can benefit from....

[hakre]

Updated patch, I know it would be easier to read to have this in 4 lines, but it's so long ago we had a oneliner in core... .

[nacin]

-1.

Escaping in wpdb is abstracted into escape, _escape, _weak_escape and _real_escape for very good reasons.

It is naturally in the default (not overwritten) implementation, function wpdb::_weak_escape() is the alias to addslashes().

You said it yourself.

As an example, you are again adversely affecting drop-ins, which can extend and rewrite the wpdb class and replace methods. i.e. bbPress extends wpdb and adds one method. A drop-in could extend wpdb and replace _weak_escape, which suddenly would cease to be called by wpdb::escape. The point here is the concept of abstraction, not even whether there exists a drop-in that does this.

Suggest closing as invalid.

[Denis-de-Bernardy]

Escaping in wpdb is abstracted into escape, _escape, _weak_escape and _real_escape for very good reasons.

Err, I'd say that these various functions exist for extremely bad reasons. We've banged our head into the table, for years, in order to support completely obsolete versions of PHP and MySQL. We end up with workarounds and workarounds around the workarounds.

It's like, heck, escape() should do exactly that: escape. Not weak, or real, or maybe, or maybe not, or anything else; plain, simple escape. There should only be a single method and it should do its job properly.

WP 3.0 probably isn't the right time to clean this mess up, though. Maybe when we switch to PHP 5.1 and decide whether we start using PDO?

[hakre]

I'm pretty shure those function-names start with _ to signal that they are inteded for private use. That is an idiom in PHP 4 code in the lack of private members. Since the accesibility of the functions you refrain is not documented in any way, the extending class can not rely on the call from escape() to _weak_escape.

@Nacin: Just ask, I can provide you references for that _ in function names and PHP 4.

I'll clean the patch that it is better readable. Maybe it was too hard to read :)

@Denis: This ticket is not about the jam-packed treasure chest by the god of escape WP has to offer (this word seems to have some special magic, just do a fulltext search, if there is a problem, you only need to escape it :) ). I've just run over this optimization, I'm sure there pretty much more but when it's left to be fixed, why not improve it? Otherwise this will never happen... (<- I'm ready-minded to be proven wrong).

[hakre]

@nacin: I'm not pedantic (than I would take a look when that comment went in), but if a developer writes it's own implementation of wpdb without taking the default function definition into account, I think that guy is pretty lost.

* Escapes content for insertion into the database using addslashes(), for security

Guess where this line is taken from. But that only as a sidenote, I do not want to argue about that any longer. You can update the patch that it does the additional function call to the private member _weak_escape (which otherwise could just be deleted, because it's not needed any longer) and then at least the escape function benefitted from the other improvements the patch has.

[nacin]

Replying to hakre:

I'm pretty shure those function-names start with _ to signal that they are inteded for private use.

In this case, I would argue that if anything, they are for protected use, not private. Many drop-ins replace wpdb::_real_escape() with a method that calls, say, pg_escape_string() or sqllite_escape_string().

Can we simplify this? Sure, we can change all references of wpdb::_weak_escape() to addslashes(), and maybe even remove wpdb::_weak_escape() when we're done. But unless we take it further as Denis said and overhaul how wpdb escapes SQL, what is truly necessary?

[hakre]

Replying to nacin:

Replying to hakre:

I'm pretty shure those function-names start with _ to signal that they are inteded for private use.

In this case, I would argue that if anything, they are for protected use, not private. Many drop-ins replace wpdb::_real_escape() with a method that calls, say, pg_escape_string() or sqllite_escape_string().

You are so damn smart that I want you to get even smarter. Please tell me which programming related book you would like to get, I'll send it to you. PHP 4 an protected function, I've never been so amused lately.

Can we simplify this? Sure, we can change all references of wpdb::_weak_escape() to addslashes(), and maybe even remove wpdb::_weak_escape() when we're done. But unless we take it further as Denis said and overhaul how wpdb escapes SQL, what is truly necessary?

Well, not much. I've updated the patch to reflect the needed changes for the whole core code: 11605.5.patch

[Denis-de-Bernardy]

I like 11605.5.patch. does the same thing, except that it's simpler to read. could we get this checked in?

[hakre]

I added those changes of 11605.5.patch to #11799 as well.

[hakre]

Related: #11819

[miqrogroove]

Replying to Denis-de-Bernardy:

I like 11605.5.patch. does the same thing, except that it's simpler to read. could we get this checked in?

I like the simplification of escape(), but I'm confused by what's going on with the deletes in that patch?

[miqrogroove]

Also, pardon the dumb questions, why does escape() allow array input?

[Denis-de-Bernardy]

See also #12402

[dd32]

We're close to a beta, This is no longer the time to be making database escaping decisions/changes.

[hakre]

Replying to dd32:

We're close to a beta, This is no longer the time to be making database escaping decisions/changes.

Last patch is not changing database escaping. It calls the exact same functions as before, it only does it better. Please review the code again.

[nacin]

Replying to dd32:

We're close to a beta, This is no longer the time to be making database escaping decisions/changes.

Those concerns are still valid. We've also made more than enough structural changes to 3.0 for this cycle. Database dropins will have a hard time adapting as it is.