Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jasonbahl merged 4 commits into from
Aug 15, 2024
Merged

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

jasonbahl merged 4 commits into from
Aug 15, 2024

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