#64130 Fix TypeError in wpdb::load_col_info() when a query fails - #13099
AKSHAT2802 wants to merge 10 commits into
Conversation
…add test for get_col_info after failed query
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks for the PR! The fix looks right to me. I reproduced the TypeError on trunk (PHP 8.3) and confirmed this change fixes it.
Fix
! ( $this->result instanceof mysqli_result )covers every non-result state:false(failed query),true(INSERT/UPDATE/DELETE/SET/etc.) andnull(no query yet, or the early return on invalid data inquery()).- It matches the existing checks in
flush()andquery(), and it's stricter than theis_object()check suggested on the ticket. get_col_info()then returnsnull, which is what callers already get when there are no columns, so valid usage is unaffected.- As a side note, PHPStan level 7 reports both
mysqli_num_fields()andmysqli_fetch_field()here (bool|mysqli_result|null given). Those go away with this change. The project's current level 5 doesn't catch it.
Test suggestions
- The test only covers the failed-query (
false) case. The more common trigger is probably readingcol_infoafter a write query, where$this->resultistrue. Could you use a data provider (astaticone, like the rest of this file) that also coversUPDATEandSET? - Please add
@ticket 64130. - Please save and restore the previous suppression state (
$suppress = $wpdb->suppress_errors( true ); … $wpdb->suppress_errors( $suppress );) instead of hardcodingfalse, as other tests in this file do. - Nit:
wptests_is hardcoded in the table name. Any table name that doesn't exist will work. - Nit:
assertFalse( $wpdb->result )checks a protected property through__get(). Thecol_info/get_col_info()assertions already cover the behavior, so it could be dropped.
Suggested version:
/**
* @ticket 64130
*
* @dataProvider data_get_col_info_without_result_set
*
* @param string $query Query that does not produce a result set.
*/
public function test_get_col_info_without_result_set( $query ) {
global $wpdb;
$suppress = $wpdb->suppress_errors( true );
$wpdb->query( sprintf( $query, $wpdb->options ) );
$wpdb->suppress_errors( $suppress );
$this->assertNull( $wpdb->get_col_info(), 'get_col_info() should return null.' );
$this->assertNull( $wpdb->col_info, 'The col_info property should be null.' );
}
/**
* Data provider.
*
* @return array<string, string[]>
*/
public static function data_get_col_info_without_result_set() {
return array(
'failed query' => array( 'SELECT ID FROM table_that_does_not_exist' ),
'update query' => array( "UPDATE %s SET option_value = option_value WHERE option_name = 'blogname'" ),
'set query' => array( 'SET @wp_test_64130 = 1' ),
);
}I ran this locally. With the fix, all 3 cases pass. Without it, all 3 fail with TypeError: mysqli_num_fields(): Argument #1 ($result) must be of type mysqli_result, {false|true} given.
refactor order Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Trac ticket: https://core.trac.wordpress.org/ticket/64130
PR Description
This PR adds a guard to
load_col_info()so it returns early when$this->resultisn't amysqli_result, consistent with the existinginstanceof mysqli_resultcheck already used inwpdb::flush().A regression test (
test_get_col_info_after_failed_query) is included, which runs an invalid query and then accessescol_info— this reproduces the reportedTypeErroron unpatched code and passes with the fix applied.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Sonnet 5
Used for: Investigating and confirming the bug against the current trunk, and writing/validating the regression test.
All changes were made and verified by reproducing the failure and confirming the fix in the local test environment.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.