WordPress-Coding-Standards

WordPress-Coding-Standards copied to clipboard

Ticket: https://core.trac.wordpress.org/ticket/52506 Change: https://core.trac.wordpress.org/changeset/53575

With WordPress 6.1 (October 2022), the %i placeholder will provide a safe way to escape Identifiers (e.g. table/field names).

Technically it's not needed if the variable only contains permitted characters, is not one of the ~286 reserved words, and is a literal-string (a value defined by the developer)... but that's not always possible, or can be hard to prove.

@craigfrancis Please add some tests.

Yep, sorry Juliette... will need to do later (thought I could make a quick change before going out).

I've added a few tests, happy to add more, or make other tweaks/changes :-)

Not complete yet... I've got to look at the array_fill() test.

---

Thanks @jrfnl, your summary is correct.

To confirm, you're right, Identifiers are always backtick-quoted (%i, %1$i, %10i, etc); and that's on purpose... my long term intention (~10 years) is to deprecate and remove the risky feature where values aren't quoted when modifiers are used, because it relies on every developer correctly adding the quote characters themselves (mistakes are made, WPCS is good at trying to find these, but not everyone uses WPCS).

I want wpdb::prepare() to do all the quoting/escaping work for the developer (correctly and consistently), and for the SQL to be provided as a literal-string (a developer defined string, which is something that's easier to test)... the next step to support this will be via #54042 so we can use $wpdb->prepare('WHERE id IN (%...d)', $ids).

---

As to the changes, I've go though the inline ones separately (feel free to resolve/comment as needed), and the rest...

I've updated the class docblock, added the @link, and @since.

The suggested additional checks, could they be in a separate Issue/PR, as they could be fairly complicated, and might need further discussion (I'm happy to start those as new Issues).

You're right about versioning, I doubt many developers will write 2 versions of the same query based on has_cap()... I'm not too familiar with WPCS, does it have a way to change its recommendations based on a version number? maybe get_wp_version_from_cli()?

And thank you for your feedback/suggestions, they were really helpful.

Finishing off from yesterday, I've:

• Added a comment on #1903 about how the UnquotedComplexPlaceholder warning might not need to be ignored.
• Started Issue #2078 to verify that the %i placeholders are only used for identifiers.
• Started Issue #2079 to recommend the use of %i.
• Started Issue #103 on the Coding Standards Handbook.

I might need to make some tweaks regarding the recommendations based on the MinimumWPVersionTrait... and/or adding a new check that will report "The %i modifier is only supported in WP 6.1 or higher".

Not complete yet, just updating the comments (thanks for the suggestions).

I need to re-look at the RegEx one, as my original one checked for a quote prefix, but allowed the developer to accidentally forget the end quote... off the top of my head "%i something" (admittedly that would probably result in a parse error, but I'll look at it properly later).

I think that's most of the suggestions.

I've not squashed all the commits together yet, in case you want to see what I've changed since.

---

The exception is the use of MinimumWPVersionTrait, which probably needs a separate discussion/PR... where I got it partially working with the following diff:

--- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php
+++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php
@@ -13,6 +13,7 @@ use PHP_CodeSniffer\Util\Tokens;
 use PHPCSUtils\Utils\PassedParameters;
 use PHPCSUtils\Utils\TextStrings;
 use WordPressCS\WordPress\Helpers\TextStringHelper;
+use WordPressCS\WordPress\Helpers\MinimumWPVersionTrait;
 use WordPressCS\WordPress\Sniff;
 
 /**
@@ -43,9 +44,13 @@ use WordPressCS\WordPress\Sniff;
  *
  * @since 0.14.0
  * @since 3.0.0 Support for the %i placeholder has been added
+ *
+ * @uses  \WordPressCS\WordPress\Helpers\MinimumWPVersionTrait::$minimum_supported_version
  */
 class PreparedSQLPlaceholdersSniff extends Sniff {
 
+	use MinimumWPVersionTrait;
+
 	/**
 	 * These regexes copied from http://php.net/manual/en/function.sprintf.php#93552
 	 * and adjusted for limitations in `$wpdb->prepare()`.
@@ -415,6 +420,7 @@ class PreparedSQLPlaceholdersSniff extends Sniff {
 			/*
 			 * Analyse the query for quoted identifier placeholders.
 			 */
+			if ( version_compare( $this->minimum_supported_version, '6.1', '>=' ) ) {

FYI and quite aside from the PR still needing a final review: the patch adding support for %i as a placeholder has just been reverted for WP 6.1 as it apparently caused some side-effects for plugins which were clearly not using WPCS and doing things wrong in the first place.

❗ Until the %i patch gets re-committed to WP Core, this PR should be considered "on hold". ❗

For those people now wondering: the "doing it wrong" is having LIKE '%%%s%%' in SQL queries provided to WPDB::prepare(), which is one of the code patterns this sniff throws warnings/errors for. The correct way would be to have LIKE %s in the query with the replacement containing the %% delimiters, as is also documented with the function: https://developer.wordpress.org/reference/classes/wpdb/prepare/ (and is exactly what this sniff tells people to do).

Thanks Juliette, and yep, I'm re-looking at my WP patch for 6.2 (and, on the positive side, I'll be able to include your suggestions this time).

Also, the example can be cut back even further, to "%%%s" - I'd not seen this being used before, I'd have thought the first "%%" would be converted to the literal "%" (as documented), but I didn't think the "%s" (that followed) would be affected by this (it takes the dangerous approach of accepting and escaping the value, but not quoting it, which the documentation says should only happen with "numbered or formatted string placeholders").

As WP 6.2 has now been released and includes the change this PR is about, I've removed the "Requirements not met" label and would love to see some movement on this PR, so it can be included in WPCS 3.0.0.

Thanks @jrfnl, I should have some time over the next week.

I've just re-read the comments, and I think my summary on the Sep 20 lists the remaining bits?

• MinimumWPVersionTrait (maybe)
• Squash all the commits together

And a "final review" (ref Oct 31)?

@craigfrancis Just checking - will this PR be ready for a final review over the next few days or not ? I'm working on another PR for this sniff and they are likely to conflict, so I've been holding off on pulling the other PR...

Sorry @jrfnl, I've not looked at the MinimumWPVersionTrait bit yet (don't know if that's a complicated thing, hopefully not, where I did note some code above)... but other than that and a squash of the commits, I think it's ready?

but other than that and a squash of the commits, I think it's ready?

Sounds like you are asking me ? I'm sorry, but I'm not going to look at this PR again until it's ready for a final review. If you want to know what needs to be done, please read the conversation above (as I will do for the final review, but not before).

I've added the MinimumWPVersionTrait, and squashed commits... I was just checking they were the final things that needed to be done, I can only assume they are correct now, and this patch is ready for the final review.

@craigfrancis I've (finally) done that final review. Turns out I'm really not good at re-reviewing PRs which have been open for a while and have had multiple reviews already.

Either way, to prevent further delays, I've now made some final fixes myself so this PR can get merged. Unfortunately, even though the "Maintainers are allowed to edit this pull request." option is turned on, I get a "permission denied" when I try to push my changes to your branch.

Would you mind giving me push access to your fork of WPCS so I can push that extra commit and merge this PR ?

FYI: This is the commit message for the additional changes:

DB/PreparedSQLPlaceholders: final fixes for %i support

* Add sniff to the list of sniffs using the `MinimumWPVersionTrait`
* Fix incorrect implementation of the minimum WP version check.
    This check was never supposed to hide the `QuotedIdentifierPlaceholder` check when the minimum supported WP version is below 6.2.
    Instead, the check is needed to flag the `%i` modifier as an "unsupported placeholder" when used with a minimum supported WP version below 6.2.

Includes tests safeguarding the new `UnsupportedIdentifierPlaceholder` error.
Note: I've left the `phpcs:set WordPress.DB.PreparedSQLPlaceholders minimum_wp_version 6.2` before the original set of test in place to prevent having to up the error/warning nrs for all those tests, while this particular error is safeguarded via separate tests anyway.

Includes a few minor doc tweaks.
Includes a minor efficiency fix (condition order in an `if`).

Hey Juliette, thank you for looking at this, I really appreciate your time looking at this (and everything else you do; my to do list is embarrassing).

I've added you as a collaborator, and happy to make any other changes.

Hey Juliette, thank you for looking at this, I really appreciate your time looking at this (and everything else you do; my to do list is embarrassing).

I've added you as a collaborator, and happy to make any other changes.

Thanks for the invite. I've pushed up my changes now. (and I think I've also figured out why that didn't work originally - some branch name mismatch by the looks of it - you may need to delete a stray branch in your fork now)

If you like, have a look at the changes I made - they are in a separate commit so you can easily see what I updated.

Once the CI build has passed, I will merge the PR now.

Thanks Juliette :-)

you may need to delete a stray branch in your fork now

I'll probably delete my branch/fork, considering it's called "patch-1", I suspect I messed up the branching at some point (I daren't look out of fear of embarrassment).

@craigfrancis Thanks for working with me on this and I'm glad we got this in and managed to avoid this PR turning 1 (year old).