wpcs-docs icon indicating copy to clipboard operation
wpcs-docs copied to clipboard

WPDB to support %i in 6.1

Open craigfrancis opened this issue 3 years ago • 1 comments

The Formatting SQL Statements section currently includes a paragraph on string and integer placeholders:

%s is used for string placeholders and %d is used for integer placeholders. Note that they are not 'quoted'! $wpdb->prepare() will take care of escaping and quoting for us. The benefit of this is that it is easy to see at a glance whether something has been escaped or not because it happens right when the query happens.

Is it worth adding that (when WordPress 6.1 is available) developers will be able to use %i for Identifiers (e.g. field and table names)?

Maybe this should be a separate paragraph? and maybe only added when 6.1 is released in October?


The intention is to avoid the risky feature where WordPress does not quote string/integer/float placeholders when they contain argnum/format, something that can be used today when (incorrectly) trying to use a variable for field/table names, e.g.

$field = 'bad`quoting';
$value = '5 AND 1=1';

$wpdb->prepare( 'WHERE `%1$s` = %2$s', $field, $value ); // WHERE `bad`quoting` = 5 AND 1=1

This relates to WordPress Ticket #52506, Changeset #53575, and Pull Request #2072 for WordPress-Coding-Standards (thanks to Juliette, who has done a nice writeup about it, and pointed me to this repo).

craigfrancis avatar Aug 16 '22 09:08 craigfrancis

Maybe this should be a separate paragraph?

Sounds like a good idea 💯

and maybe only added when 6.1 is released in October?

I agree, probably best for the paragraph to be added around the same time as the release (to prevent confusing people).

There's a lot of ongoing work on the PHP standards page at the moment, however, I don't think that section will get further changes between now and then, so a PR could already be opened with a note not to merge it until end of October. Would run a (small) risk of getting in a conflict state, but that shouldn't be much of a problem to resolve if needs be.

jrfnl avatar Aug 16 '22 12:08 jrfnl

As WP 6.1 has been released, I'm wondering if this action item should possibly get a PR by now ?

jrfnl avatar Jul 18 '23 23:07 jrfnl

Would something like PR 132 work?

I've simply taken the list of placeholders from the wpdb::prepare documentation, which also notes the use of %f.

I'm using list formatting, and trimmed/tweaked the last couple of sentences, to hopefully make it a bit easier to read.

craigfrancis avatar Jul 22 '23 01:07 craigfrancis

@craigfrancis It would IMO. I've left a small remark, but other than that, it looks good.

jrfnl avatar Jul 22 '23 01:07 jrfnl

That's fair to emphasise the warning, have committed your suggestion.

craigfrancis avatar Jul 22 '23 09:07 craigfrancis