Opened 2 years ago
Last modified 8 months ago
#56091 assigned enhancement
Using %i for table/field names in wpdb::prepare()
Reported by: | craigfrancis | Owned by: | craigfrancis |
---|---|---|---|
Milestone: | Future Release | Priority: | low |
Severity: | minor | Version: | 6.1 |
Component: | Database | Keywords: | has-patch early |
Focuses: | Cc: |
Description (last modified by )
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.
Change History (16)
This ticket was mentioned in PR #3016 on WordPress/wordpress-develop by craigfrancis.
2 years ago
#2
- Keywords has-patch added; needs-patch removed
craigfrancis commented on PR #3016:
2 years ago
#3
Created PR 2072 for WordPress-Coding-Standards (ref "Unsupported placeholder used in $wpdb->prepare()").
#4
@
2 years ago
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
).
#5
@
2 years ago
- Keywords early added
- Milestone changed from 6.1 to 6.2
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.
#7
@
2 years ago
- Summary changed from Use %i for table/field names in wpdb::prepare() to Using %i for table/field names in wpdb::prepare()
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).
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
23 months ago
#9
@
23 months ago
- Milestone changed from 6.2 to Future Release
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).
#11
@
17 months ago
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
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
16 months ago
#13
@
15 months ago
- Milestone changed from 6.4 to 6.5
Because this is an early ticket, I am moving it into 6.5 milestone.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
10 months ago
#15
@
10 months ago
- Milestone changed from 6.5 to Future Release
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.
#16
@
8 months ago
@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.
First patch, only affecting SQL that is used directly by
$wpdb->get_col()
,$wpdb->get_var()
, etc.Trac ticket: 56091