Using %i for table/field names in wpdb::prepare()

[craigfrancis]

Now wpdb::prepare() supports %i for Identifiers (e.g. table/field names), via commit [53575], and ticket #52506.

Queries within WP Core should use this, to ensure variables are always quoted, and avoid static analysis tools flagging unescaped SQL input (a non-literal-string) for the $query parameter:

<?php
$wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE post_type = %s", $post_type );

$wpdb->prepare( "SELECT ID FROM %i WHERE post_type = %s", $wpdb->posts, $post_type );

I'll write a patch for the first set, but I suspect there will be a lot of changes, and they should be checked carefully.

First patch, only affecting SQL that is used directly by $wpdb->get_col(), $wpdb->get_var(), etc.

Trac ticket: 56091

Created ​PR 2072 for WordPress-Coding-Standards (ref "Unsupported placeholder used in $wpdb->prepare()").

[craigfrancis]

First patch: Parameterise some table names.

It will help Static Analysis / Sniffs, especially when IN(%...d) is supported (via #54042), as makes it easy to check all variables are escaped correctly.

But I found some of these changes make the SQL a bit harder to read, as it can take a second to work out which table is being used (maybe that will get easier once I'm used to it?)

This patch only affects a few queries. I do have more, where I've been able to get most of WordPress working with the $table_prefix set to 'wp ' (with a space), but some changes get a bit complicated, and I doubt many (any?) developers need a $table_prefix that contains characters other than [a-zA-Z0-9_].

---

A different approach, specifically for $table_prefix, would use the literal-string type (supported in ​Psalm and ​PHPStan). This ensures a property/variable only contains a "developer defined string", and it supports concatenation.

This means properties like ​$wpdb->posts could use this type, and continue to be concatenated directly into the SQL string without escaping (ish). Then static analsysis tools (that trace variables from source to sink) could check the first argument to wpdb::prepare() is still a literal-string. Admittedly this won't work for variables that cannot be defined as a literal-string, or when they contain any ​non-permitted characters, or any of the ​~286 reserved words (which is why we need %i).

[davidbaumwald]

This still needs some work, and I think this should go in early in a cycle.

Moving this to 6.2 for continued work after r53575 has some time out in the wild.

[uzumymw]

Is it impossible to fight with this change for version 6.1?

[craigfrancis]

Hi @uzumymw, and anyone who's interested, I agree with David, this patch needs to be done carefully, and with discussion.

Just to note, "%i" will be available in 6.1 (assuming I've not made a massive mistake somewhere), where it ensures variable identifiers (e.g. table/field names) are escaped correctly... but this patch is about using "%i" in core, and in some cases I think it can make the code harder to read, for little to no benefit (I want to avoid those situations).

I'm happy to chat about this on WP Slack, or email (craig [at] craigfrancis [dot] co [dot] uk).

[audrasjb]

As per today's bug scrub, it looks like this ticket still needs review/testing. Moving to Future Release. @craigfrancis @davidbaumwald feel free to move it back to 6.2 if you think it can be committed in time (early tickets should be committed by next week).

[SergeyBiryukov]

This came up in a coding session with @afercia, @aristath, and @poena, where we tried to address the existing WPCS warnings for missing or incorrect usage of $wpdb->prepare() in core, so I'm moving this to 6.4 for review.

We have also noticed that the %i placeholder is not currently recognized by WPCS, as seen on the PR:

Unsupported placeholder used in $wpdb->prepare(). Found: "%i".

This ​appears to be fixed in an upcoming version of WPCS.

In our testing, this silences the error for now, until WPCS is updated in core:

// The placeholder ignores can be removed when %i is supported by WPCS.
// phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.UnsupportedPlaceholder, WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber

[oglekler]

Because this is an early ticket, I am moving it into 6.5 milestone.

[audrasjb]

Hey there, it looks like the ​needed change was released with WPCS 3.0.0.

However, let's move this ticket to Future Release as 6.5 beta 1 is pretty close, now.
@SergeyBiryukov if you believe this ticket is ready to ship, feel free to move it back to 6.5.

[johnbillion]

@craigfrancis Do you want to refresh this for 6.6? I'd love to remove ​this PHPCS config so unprepared queries trigger an error instead of a warning. Implementing %i gets us a step closer to that.