Skip to content

fix: [regression] missing placeholder in $wpdb->prepare#3189

Merged
jasonbahl merged 4 commits intowp-graphql:developfrom
justlevine:fix/missing-placeholder-wpdb
Aug 15, 2024
Merged

fix: [regression] missing placeholder in $wpdb->prepare#3189
jasonbahl merged 4 commits intowp-graphql:developfrom
justlevine:fix/missing-placeholder-wpdb

Conversation

@justlevine
Copy link
Collaborator

@justlevine justlevine commented Aug 14, 2024 •
edited
Loading

What does this implement/fix? Explain your changes.

This PR fixes a regression introduced in #3183 , where the lack of placeholders when interpolating the $ids into $wpdb->prepare() was throwing a _doing_it_wrong().

PHP Notice:  Function wpdb::prepare was called <strong>incorrectly</strong>. The query argument of wpdb::prepare() must have a placeholder. Please see <a href="https://developer.wordpress.org/advanced-administration/debug/debug-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 3.9.0.) in /shared/httpd/wptest/htdocs/wp-includes/functions.php on line 6085

Testing Instructions

Run a user query or a post query (since the post model preloads the author), and confirm no PHP Notice is thrown.

query {
  nodeByUri( uri: '/path/to/my-post' ) {
     __typename
  }
}

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

While this is usually just a PHP notice in userland, it causes failing Codeception tests in later versions of wp-browser:

image

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.2)

WordPress Version: 6.6.1

@justlevine justlevine requested a review from jasonbahl August 14, 2024 13:23
@justlevine justlevine added type: bug Issue that causes incorrect or unexpected behavior status: in review Awaiting review before merging or closing needs: reviewer response This needs the attention of a codeowner or maintainer regression Bug that causes a regression to a previously working feature labels Aug 14, 2024
@justlevine justlevine force-pushed the fix/missing-placeholder-wpdb branch from d90648f to f593aba Compare August 14, 2024 13:31
@jasonbahl
Copy link
Collaborator

jasonbahl commented Aug 14, 2024

@justlevine it looks like this change is breaking a few tests

@justlevine
Copy link
Collaborator Author

justlevine commented Aug 14, 2024 •
edited
Loading

Yup, left a message in discord #maintainers. Got called into a meeting but tl;dr non-numbered %s get 'QUOTED' (which breaks the query since $ids need to be treated as ints ) but using a numbered placeholder e.g. %1\$s causes the notice to come back 🤔

@qlty-cloud-legacy
Copy link

qlty-cloud-legacy bot commented Aug 14, 2024

Code Climate has analyzed commit c21332f and detected 0 issues on this pull request.

View more on Code Climate.

@justlevine
Copy link
Collaborator Author

justlevine commented Aug 14, 2024

Fixed in c21332f turns out numbered placeholders is the right move, just that wpdb:prepare() doesn't let you reuse the same inputs like sprint_f() does.

@coveralls
Copy link

coveralls commented Aug 14, 2024

Coverage Status

coverage: 84.174% (+0.002%) from 84.172%
when pulling c21332f on justlevine:fix/missing-placeholder-wpdb
into 66aaf9a on wp-graphql:develop.

@jasonbahl jasonbahl self-assigned this Aug 15, 2024
@jasonbahl jasonbahl merged commit 43fa32c into wp-graphql:develop Aug 15, 2024
@justlevine justlevine deleted the fix/missing-placeholder-wpdb branch August 15, 2024 17:17
@saulirajala
Copy link
Contributor

saulirajala commented Aug 20, 2024

When will there be a release including this fix?

@jasonbahl jasonbahl mentioned this pull request Aug 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonbahl jasonbahl jasonbahl approved these changes

@josephfusco josephfusco josephfusco approved these changes

Assignees

@jasonbahl jasonbahl

Labels

needs: reviewer response This needs the attention of a codeowner or maintainer regression Bug that causes a regression to a previously working feature status: in review Awaiting review before merging or closing type: bug Issue that causes incorrect or unexpected behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@justlevine @jasonbahl @coveralls @saulirajala @josephfusco

Comments