Repository navigation
Fatal error in wp_insert_term() when the term name is not a string - #13521
josephscott wants to merge 21 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. 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. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
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. |
Tested and Verified ✅Environment:
Verification:
Regarding the discussion on JSON Schema adoption for XML-RPC: while a future schema-driven sanitization layer for XML-RPC is a worthy initiative, this PR addresses a real PHP 8+ fatal error in |
|
|
||
| $term_names = $post_data['terms_names'][ $taxonomy ]; | ||
| foreach ( $term_names as $term_name ) { | ||
| if ( ! is_string( $term_name ) ) { |
There was a problem hiding this comment.
Good. This addresses an existing PHPStan error on code below:
| if ( '' === trim( $term ) ) { | ||
| if ( ! is_scalar( $term ) || '' === trim( $term ) ) { |
There was a problem hiding this comment.
Is this just a hardening? Is it needed for the above fix. PHPStan isn't complaining here because $term comes from the return value of the pre_insert_term filter which is typed as string|WP_Error. It is first checked to see if it is a WP_Error in which it returns, so then the type narrows to string.
I think this is a case where the callers should be fixing the types that they pass into the wp_insert_term() function.
There was a problem hiding this comment.
For the XML-RPC case it is not strictly needed.
It is first checked to see if it is a WP_Error in which it returns, so then the type narrows to string.
I don't think that is accurate, which is why we have an is_int() call as well - https://github.com/WordPress/wordpress-develop/blame/trunk/src/wp-includes/taxonomy.php#L2595
Anything that calls wp_insert_term() directly can cause a problem. Here is an example without this fix:
$ npm run env:cli --silent -- eval 'var_dump( wp_insert_term( array( "foo" ), "post_tag" ) );'
Fatal error: Uncaught TypeError: trim(): Argument #1 ($string) must be of type string, array given in /var/www/src/wp-includes/taxonomy.php:2599
Stack trace:
#0 /var/www/src/wp-includes/taxonomy.php(2599): trim(Array)
#1 phar:///usr/local/bin/wp/vendor/wp-cli/eval-command/src/Eval_Command.php(39) : eval()'d code(1): wp_insert_term(Array, 'post_tag')
#2 phar:///usr/local/bin/wp/vendor/wp-cli/eval-command/src/Eval_Command.php(39): eval()
#3 [internal function]: Eval_Command->__invoke(Array, Array)
#4 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php(100): call_user_func(Array, Array, Array)
#5 [internal function]: WP_CLI\Dispatcher\CommandFactory::WP_CLI\Dispatcher\{closure}(Array, Array)
#6 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/Subcommand.php(497): call_user_func(Object(Closure), Array, Array)
#7 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(470): WP_CLI\Dispatcher\Subcommand->invoke(Array, Array, Array)
#8 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(493): WP_CLI\Runner->run_command(Array, Array)
#9 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(136): WP_CLI\Runner->run_command_and_exit()
#10 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1228): WP_CLI\Runner->do_early_invoke('before_wp_load')
#11 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start()
#12 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/bootstrap.php(84): WP_CLI\Bootstrap\LaunchRunner->process(Object(WP_CLI\Bootstrap\BootstrapState))
#13 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php(35): WP_CLI\bootstrap()
#14 phar:///usr/local/bin/wp/php/boot-phar.php(20): include('phar:///usr/loc...')
#15 /usr/local/bin/wp(4): include('phar:///usr/loc...')
#16 {main}
thrown in /var/www/src/wp-includes/taxonomy.php on line 2599
Error: There has been a critical error on this website.Learn more about troubleshooting WordPress. There has been a critical error on this website.
And here is the same call with this fix:
$ npm run env:cli --silent -- eval 'var_dump( wp_insert_term( array( "foo" ), "post_tag" ) );'
object(WP_Error)#2540 (3) {
["errors"]=>
array(1) {
["empty_term_name"]=>
array(1) {
[0]=>
string(33) "A name is required for this term."
}
}
["error_data"]=>
array(0) {
}
["additional_data":protected]=>
array(0) {
}
}
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Code review findings are left inline. The first two (in _insert_post()) are regressions introduced by this change and are the most important; the rest are API consistency, scope, and test-docs points.
|
|
||
| $term_names = $post_data['terms_names'][ $taxonomy ]; | ||
| foreach ( $term_names as $term_name ) { | ||
| if ( ! is_string( $term_name ) ) { |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The is_string() guard rejects integer term names that previously worked. An XML-RPC client sending a numeric tag name as <int> (e.g. Python's xmlrpc.client with 2024) used to get a working lookup via get_term_by( 'name', 2024 ) and, failing that, wp_insert_term( 2024 ). Now the post fails with 403 "Invalid term name." This also disagrees with wp_insert_term(), which this same PR explicitly accepts ints and floats for, and the 'integer' case in the new newPost test locks the regression in.
Allowing is_int() (and possibly is_float()) and casting to string would keep numeric names working while still rejecting arrays and objects.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in f216122. Ints and floats are now cast to strings, and only other types (such as arrays and booleans) return the 403 "Invalid term name." error. The 'integer' invalid case was replaced with 'boolean', and test_terms_names_numeric_term_name asserts that 2024 and 1.5 are assigned as the tags "2024" and "1.5".
| return new IXR_Error( 403, __( 'Invalid term name.' ) ); | ||
| } | ||
|
|
||
| if ( in_array( $term_name, $ambiguous_terms, true ) ) { |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Ambiguous numeric term names cannot be detected here. $ambiguous_terms comes from array_keys( array_count_values( ... ) ), which converts numeric-string names like "2024" into int keys. Since $term_name is now guaranteed to be a string, the strict in_array( '2024', array( 2024 ), true ) is always false, so a hierarchical taxonomy with two terms named "2024" silently resolves to whichever get_term_by() returns first. (The string case predates this PR, but an int input used to match, and now no input can trigger the check.) Mapping $ambiguous_terms through strval would fix it.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in 5f65187. $ambiguous_terms is now mapped through strval, so numeric names match the (string) term name in the strict in_array() check. test_terms_names_ambiguous_numeric_term_name covers a hierarchy with two categories named "2024", sent both as '2024' and 2024; both cases failed before the fix.
| @@ -1710,6 +1710,10 @@ protected function _insert_post( $user, $content_struct ) { | |||
|
|
|||
| $term_names = $post_data['terms_names'][ $taxonomy ]; | |||
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
$term_names itself is not checked to be an array. Sending terms_names => array( 'post_tag' => 'foo' ) makes the foreach emit a "foreach() argument must be of type array|object" warning, and the terms are silently ignored instead of returning an error. The $term_ids branch has the same gap. Possibly out of scope, but it sits right next to the new per-item guard.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in cb770c2. Both the terms and terms_names branches now check that the per-taxonomy value is an array, returning the existing "Invalid term ID." (403) or "Invalid term name." (401) error respectively, instead of emitting a foreach warning and silently dropping the terms. test_terms_not_array covers a string and an int for terms and a string for terms_names; all three cases hit the warning before the fix. Since wp.editPost shares _insert_post(), it is covered too.
| $term_names = $post_data['terms_names'][ $taxonomy ]; | ||
| foreach ( $term_names as $term_name ) { | ||
| if ( ! is_string( $term_name ) ) { | ||
| return new IXR_Error( 403, __( 'Invalid term name.' ) ); |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Minor: every other validation error in this terms_names block uses code 401 (e.g. "Ambiguous term name…"), while this one uses 403. There is precedent for 403 with "Invalid term ID." in the IDs branch, so this is a judgment call, but it is worth a deliberate choice.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in f406733. The invalid term name error now uses 401, consistent with the other validation errors in the terms_names block. The term ID block keeps 403, matching its existing "Invalid term ID." error.
| } | ||
|
|
||
| if ( '' === trim( $term ) ) { | ||
| if ( ! is_scalar( $term ) || '' === trim( $term ) ) { |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The same trim()-on-non-string TypeError remains in other term sinks: wp_update_term() (the trim( $args['name'] ) handling), wp_set_object_terms(), wp_remove_object_terms(), and term_exists(). For example, wp_update_term( $id, 'post_tag', array( 'name' => array( 'x' ) ) ) or wp_set_object_terms( $post_id, array( array( 'x' ) ), 'post_tag' ) still fatals. These could be addressed here or in a follow-up ticket.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The XML-RPC paths that still reached this trim() fatal were checked, and five were confirmed to crash with an array name (each requires an authenticated user with the relevant capability):
| Method | Input | Crash site |
|---|---|---|
metaWeblog.newPost |
nested array in mt_keywords |
wp_set_object_terms() (via tags_input) |
metaWeblog.editPost |
same | same |
wp.newTerm |
array name |
the server's own trim(), before wp_insert_term() |
wp.editTerm |
array name |
the server's own trim(), before wp_update_term() |
wp.newCategory |
array name |
wp_insert_category() |
These are now fixed:
- 7ac3e7e:
wp.newTermandwp.editTermreturn a 403 "Invalid term name." error for a name that is not a string or number. - 7f728ca:
wp.newCategorydoes the same. It had no tests, so a test class was added. - 7a115d9:
wp_set_object_terms()validates every term up front and returns aninvalid_term_nameerror before any terms or relationships change.nullandfalseare still skipped, as before, and are now cast to string beforetrim(), which avoids a PHP 8.1 deprecation notice. ThemetaWeblogmethods ignore the tag-setting result, so such a post is saved without the requested tags rather than crashing. - 88f20e1:
escape()now declares a template type for its by-reference parameter, so$argskeeps its docblock-derived shape after escaping. This was needed for the newwp.newTermguard to pass PHPStan, and drops the file's level 10 error count from 659 to 448.
The remaining sinks, wp_update_term(), wp_remove_object_terms(), and term_exists(), are not reachable over XML-RPC with a malformed name (wp.editTerm now rejects it before calling wp_update_term()), so they are left for a follow-up ticket.
| } | ||
|
|
||
| if ( '' === trim( $term ) ) { | ||
| if ( ! is_scalar( $term ) || '' === trim( $term ) ) { |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Two points about the guard itself:
- A non-scalar
$termreturnsempty_term_name("A name is required for this term."), which misdescribes the problem: the name was provided, just with the wrong type. The function already has aninvalid_term_namecode a few lines below, which would let callers distinguish "missing" from "malformed". is_scalar()accepts booleans, sotrueis silently inserted (or matched) as a term named "1".is_string( $term ) || is_int( $term ) || is_float( $term )would reject it.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Both points are addressed:
- 4188078 replaces
is_scalar()withis_string() || is_int() || is_float(), sotrueis rejected rather than inserted as "1". - 19ef32b returns
invalid_term_name("Invalid term name.") for arrays, objects, andtrue.nullandfalsestill returnempty_term_name, as they did before this PR (they reduced to an empty string undertrim()), so no existing error code changes for them.
test_wp_insert_term_invalid_term_name_type now covers all five types with their expected error codes.
| * @since 2.3.0 | ||
| * @since 7.2.0 A non-scalar `$term` now returns a WP_Error instead of causing a fatal error. | ||
| * | ||
| * @param string $term The term name to add. |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The docblock still declares @param string $term, while the new tests assert that ints and floats are supported (stored as their string form). That mismatch is why the tests need @phpstan-ignore argument.type. Widening the type to string|int|float would make the contract explicit in the developer docs and remove the ignores in the numeric test.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in 1f8bcc4. The $term type is now string|int|float, noting that a number is stored as its string form, and the @phpstan-ignore in the numeric test was removed. The pre_insert_term filter docblock still says string|WP_Error and was left unchanged.
| * @param mixed $name The term name to insert. | ||
| */ | ||
| public function test_wp_insert_term_non_scalar_term_name( $name ): void { | ||
| $found = wp_insert_term( $name, 'post_tag' ); // @phpstan-ignore argument.type (Intentionally passing scalars.) |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The ignore justification says "Intentionally passing scalars.", but this test passes non-scalars (an array and an object). It appears to have been copied from the numeric test below, where it is accurate.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in 4188078. The test was renamed to test_wp_insert_term_invalid_term_name_type (it now also covers true, false, and null, which are scalars), and the ignore comment now reads "Intentionally passing invalid types."
| } | ||
|
|
||
| /** | ||
| * Data provider. |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Nit: this provider docblock could follow the same conventions as the providers in wpInsertTerm.php: a {@see} reference to the test method it feeds, and a narrowed key type (e.g. array<non-falsy-string, array{ mixed }>).
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Addressed in 51ab927. All three data providers in this file now reference their test with {@see} and use truthy-string keys.
The is_string() guard rejected term names sent as <int> or <double>, such as a tag named 2024, which previously worked via get_term_by() and wp_insert_term(). Cast ints and floats to strings instead, and reject only other types such as arrays and booleans. Replace the 'integer' invalid case with 'boolean', and add a test asserting numeric names are assigned using their string form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ambiguous names list comes from array_keys(), which turns numeric names like "2024" into integer keys. Since term names are now always strings by the time they are checked, the strict in_array() never matched them, so a hierarchical taxonomy with two terms named "2024" silently resolved to whichever term get_term_by() returned first. Cast the keys back to strings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The is_scalar() guard let booleans through, so true was inserted or matched as a term named "1". Accept only strings, ints, and floats. Rename the test to reflect that it covers invalid types rather than just non-scalars, add true, false, and null cases, and correct the ignore comment, which said scalars were being passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The function accepts ints and floats and stores them as strings, which the tests now assert, but the docblock declared only string. Widen the $term type to string|int|float so the contract is explicit, which also makes the PHPStan ignore in the numeric test unnecessary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A term name of the wrong type, such as an array, an object, or true,
returned empty_term_name ("A name is required for this term."), which
misdescribes the problem: a name was given, just not a usable one.
Return the existing invalid_term_name code instead, so callers can
tell a missing name from a malformed one.
Null and false still return empty_term_name, as they did before this
change, since they reduced to an empty string under trim().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reference the test each provider feeds with {@see}, and narrow the
key type to truthy-string, as done for the wp_insert_term() providers.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every other validation error in the terms_names block, such as the ambiguous term name error, uses fault code 401. Use the same code for an invalid term name, so clients see consistent codes for this input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A per-taxonomy value sent as a single string or int, rather than a list, reached foreach unvalidated. That emitted a "foreach() argument must be of type array|object" warning, and the terms were silently ignored instead of an error being returned. Return the existing "Invalid term ID." (403) or "Invalid term name." (401) error instead, matching the codes already used for invalid items in each list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
escape() takes its argument by reference with a string|array type, so after every `$this->escape( $args )` call PHPStan widened `$args` to string|array, and each `$args[N]` read became mixed. That discarded the argument shapes derived from the method docblocks and produced hundreds of errors across the class. Declare escape() with a template type for both the parameter and its by-reference output, so callers keep their argument shapes. This drops the file's level 10 error count from 659 to 448. PHPStan cannot prove that the by-reference loop preserves the shape, so that one error is ignored with a justification. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both methods passed the name straight to trim(), so an array name caused a fatal TypeError before wp_insert_term() or wp_update_term() was ever reached, and true was trimmed to "1". Return a 403 "Invalid term name." error unless the name is a string or number, matching the code these methods already use for an empty name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The category name was passed to wp_insert_category(), whose trim() caused a fatal TypeError for an array name. Return a 403 "Invalid term name." error unless the name is a string or number, matching the term methods. There were no tests for wp.newCategory, so add a test class covering both a valid name and invalid name types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A term that was not a string or number, such as a nested array in a metaWeblog.newPost or metaWeblog.editPost mt_keywords list, reached trim() and caused a fatal TypeError. Validate every term up front and return an invalid_term_name error before any terms or relationships are changed, so a bad item cannot leave the object half updated. Null and false are still skipped, as before. Cast to string before the existing trim() so skipping null no longer emits a PHP 8.1 deprecation notice. The metaWeblog methods ignore the result of setting tags, so the post is still saved, without the requested tags, rather than crashing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@josephscott Pushed up some commits that were made to apply feedback from some rounds of Claude reviews. |
|
@westonruter thank you - I've updated it with a few more small adjustments to try and keep the behavior mostly consistent with trunk. |
https://core.trac.wordpress.org/ticket/66108
AI assistance: Yes
Tool(s): Claude Code
Model(s): Fable 5.1
Used for: Research and experimentation. Guided directions on writing the code changes. It wrote the tests.
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.