Sitemaps: backtick column names via %i placeholders #48533
Sitemaps: backtick column names via %i placeholders #48533StefMattana wants to merge 2 commits intotrunkfrom
%i placeholders #48533Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Jetpack’s sitemaps librarian to build post-column SELECT lists with identifier placeholders so sitemap queries keep working when wp_posts contains reserved-word column names. It fits into the sitemaps subsystem by hardening both the main sitemap and news sitemap post-fetch queries.
Changes:
- Reworks
get_sanitized_post_columns()to return%iplaceholder SQL plus the corresponding column names forwpdb::prepare(). - Applies that prepared column list to both
query_posts_after_id()andquery_most_recent_posts(). - Adds a regression test for a reserved-keyword
wp_postscolumn and records the fix in the Jetpack changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
projects/plugins/jetpack/modules/sitemaps/sitemap-librarian.php |
Switches sitemap post queries to prepared identifier placeholders for column names. |
projects/plugins/jetpack/tests/php/modules/sitemaps/Jetpack_Sitemap_Librarian_Test.php |
Adds a regression test that alters wp_posts to include a reserved-word column. |
projects/plugins/jetpack/changelog/fix-48202-sitemap-backtick-columns |
Adds the Jetpack changelog entry for the sitemap SQL fix. |
| // Add a column whose name is a reserved SQL keyword. | ||
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.SchemaChange,WordPress.DB.DirectDatabaseQuery.NoCaching | ||
| $wpdb->query( "ALTER TABLE {$wpdb->posts} ADD COLUMN `order` INT NULL" ); |
| } finally { | ||
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.SchemaChange,WordPress.DB.DirectDatabaseQuery.NoCaching | ||
| $wpdb->query( "ALTER TABLE {$wpdb->posts} DROP COLUMN `order`" ); |
| $columns = $this->get_sanitized_post_columns( $wpdb ); | ||
|
|
||
| // phpcs:disable WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder,WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- WPCS: db call ok; no-cache ok. | ||
| return $wpdb->get_results( | ||
| $wpdb->prepare( | ||
| "SELECT $columns_list | ||
| "SELECT {$columns['placeholders']} |
a799522 to
a1c4b5e
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 🔴 Action required: Please include detailed testing steps, explaining how to test your change, like so: 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so: Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
|
Hey @anomiex! Would you terribly mind taking a look at this PR? I was working on it within our Bug Blitz project - the issue was added in this list on Linear :) TIA! |
anomiex
left a comment
There was a problem hiding this comment.
Looks reasonable. I flagged fixes for the linting errors.
|
|
||
| // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- WPCS: db call ok; no-cache ok. | ||
| return $wpdb->get_results( | ||
| $wpdb->prepare( |
There was a problem hiding this comment.
The lint issue on this line is doubly confused. Best to suppress it.
| $wpdb->prepare( | |
| // phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber -- Sniff is confused by both the interpolation of `$columns['placeholders']` and the spread. | |
| $wpdb->prepare( |
|
|
||
| // phpcs:disable WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder,WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- WPCS: db call ok; no-cache ok. | ||
| return $wpdb->get_results( | ||
| $wpdb->prepare( |
There was a problem hiding this comment.
Same as above
| $wpdb->prepare( | |
| // phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber -- Sniff is confused by both the interpolation of `$columns['placeholders']` and the spread. | |
| $wpdb->prepare( |
Fixes #48202
Jetpack_Sitemap_Librarian::get_sanitized_post_columns()was running column names throughesc_sql(), which is for string values, not identifiers. Ifwp_postshad a column whose name was a reserved SQL keyword (e.g. order), the resulting SELECT would error out.As suggested in the issue thread, switching to
%iidentifier placeholders inwpdb::prepare() is the idiomatic fix.It backtick-quotes and escapes identifiers correctly. The helper now returns both the placeholder string and the column array; both call sites splat the array into prepare().
Includes a regression test that adds an order column to
wp_posts, runs the query, and asserts it succeeds.Reported by @nebbens, with the suggested approach from @anomiex.