Code Quality: Improve type specificity for comments#12606
Open
westonruter wants to merge 23 commits into
Open
Conversation
Add PHPStan conditional return types, array shapes, and narrowed property/parameter types across the comment APIs. * Define a `CommentArray` shape alias for `WP_Comment::to_array()`, describing the full key set of a comment row. The shape is open (trailing `...`) because `WP_Comment` is `#[AllowDynamicProperties]`, so sealing it would produce false "offset does not exist" errors. Integer-ID string fields (`comment_ID`, `comment_post_ID`, `comment_karma`, `comment_parent`, `user_id`) come from integer DB columns and default to '0', so they are typed as `numeric-string`, and the NOT NULL DATETIME fields as `non-empty-string`. Fields that only look enumerable but are extensible by plugins (`comment_type`, `comment_approved`) are intentionally left as `string` so the shape does not over-narrow and mask real values. * Add conditional return types for `get_comment()` and `get_comments()` so callers get the right type per `$output` / `$args`. `get_comment()` returns null when the comment cannot be resolved regardless of the output format, so `|null` is included in every branch. The aliases are class-scoped, so the free functions cannot reference `CommentArray` in their conditional returns; once phpstan/phpstan#9164 lands they can adopt it as well. * Make `WP_Comment::get_children()` return type conditional on the 'format' arg: 'flat' returns a flattened list re-indexed by `array_merge()`, while 'tree' returns the `WP_Comment` map keyed by comment ID. Annotate `$_args` with the shape resulting from merging `$args` over `$defaults`, since PHPStan cannot infer `wp_parse_args()` merge semantics, and narrow the `get_comments()` result to the map expected by the `$children` property (with the known fields/count caveat noted in a TODO). * Spell out the key type as `int|numeric-string` wherever comment arrays are keyed by comment ID. PHPStan already normalizes numeric-string array keys this way, since PHP casts canonical numeric-string keys to integers at runtime, so this makes the runtime reality explicit rather than implying string keys that will not actually exist. Document on `WP_Comment::$children` that the ID map only holds for the default 'threaded' hierarchical argument, and that a 'flat' or false override stores a sequentially-keyed array instead. * Fix latent issues surfaced by the analysis: `get_comment()` now only calls `WP_Comment::get_instance()` for numeric input instead of passing through arbitrary values; the `__get()` magic getter always returns a value; and `__isset()` casts `$comment_post_ID` to int and guards against a null `get_post()` result before dereferencing it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Annotate `get_approved_comments()` with a PHPStan conditional return type so static analysis can resolve the concrete result at each call site, mirroring the treatment already given to `get_comments()`. The outermost condition is keyed on `$post_id` rather than `$args`, because the function returns early with an empty array whenever `$post_id` is falsey - before `$args` is ever parsed. That case applies to every mode, including `count`, where it is the only branch whose declared type would not otherwise admit an empty array. The remaining conditions narrow on `count`, `fields`, and `hierarchical` in the same order as `get_comments()`. The non-prefixed `@return` tag is left as the less-specific `WP_Comment[]|int[]|int`, with its description extended to document the empty-array case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the `array<int|numeric-string, WP_Comment>` key type used across the comment APIs with `array<int, WP_Comment>`. A `numeric-string` array key is not representable in PHP: keys that are numeric strings are coerced to integers on assignment, so the union could never describe a real array. Both sites that build these arrays - `WP_Comment::add_child()` and `WP_Comment_Query::fill_descendants()` - key on `comment_ID`, which is a numeric string, and therefore always produce integer keys. `WP_Comment::add_child()` now casts `comment_ID` to `int` explicitly when indexing, so the key type is established by the code rather than asserted by the annotation. This is behavior-preserving, since PHP already performed the same coercion implicitly. Comment ID arrays are narrowed from `int[]` to `non-negative-int[]`, and `WP_Comment_Query::get_comment_ids()` gains a `@phpstan-return` of `non-negative-int|list<non-negative-int>`, reflecting that `comment_ID` is an unsigned column. `list` is used only for `get_comment_ids()`, whose result is built directly from `wpdb::get_col()` with no filter in between; the filterable public entry points keep the weaker array types, since `comments_pre_query` and `the_comments` can return arbitrarily keyed arrays. Also correct `WP_Comment::$children`, which is null until `get_children()` populates it, and add a `void` return type to `WP_Comment::add_child()`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Enumerate the values core actually uses for `WP_Comment::$comment_approved` and `WP_Comment::$comment_type`, and type the former as `non-empty-string`. `comment_approved` is backed by a `varchar(20) NOT NULL default '1'` column and is never assigned an empty string, so `non-empty-string` holds. Its documented values now include 'post-trashed', which `wp_trash_post()` writes to every comment on a post being trashed, and which `wp_count_comments()` reports as a distinct bucket. `comment_type` gains the 'pingback' and 'trackback' values, which predate 'note' and were previously undocumented here. It is deliberately left as a plain `string` rather than narrowed alongside `comment_approved`: although the column has defaulted to 'comment' since 5.5.0, comments created before then may still store an empty string, which is why `get_comment_type()` normalizes that case on read. A note records this so the type is not tightened later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`WP_Comment::__get()` proxies the 21 field names listed in `$post_fields` to the comment's post, but none of them were declared on the class, so they were invisible to static analysis, IDE completion, and the generated documentation. Add a `@property-read` for each, typed to match the corresponding `WP_Post` property: `post_author` is `numeric-string|''`, the always-populated `post_status`, `comment_status`, `ping_status`, and `post_type` slugs are `non-empty-string`, `post_parent` is `non-negative-int`, `comment_count` is `numeric-string`, and `menu_order` stays a plain `int` since it can be negative. Every entry is nullable, because the getter returns null when the comment's post no longer exists. `ID` and `post_password` are deliberately absent: they are `WP_Post` properties but are not in `$post_fields`, so the getter does not proxy them. The `-read` variant is used because there is no corresponding `__set()`. Assigning to one of these names creates a real dynamic property that shadows the getter for the rest of that object's lifetime, so they cannot be treated as writable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the native `array` return type added to `WP_Comment::get_children()`, which made the method fatal whenever it was asked for a count. `get_comments()` returns an integer for a `count` query and a list of comment IDs for a `fields` query, and `get_children()` passes both straight through, so an unconditional `array` return type does not describe it. Core relies on this. `WP_REST_Comments_Controller::prepare_links()` calls the method with `count => true` to test cheaply whether a comment has children, and `wp_delete_comment()` and `wp_trash_comment()` call it with `fields => 'ids'` when removing the children of a top-level note. The first of these raised `TypeError: WP_Comment::get_children(): Return value must be of type array, int returned`, failing 102 of the REST comment tests. The `count` and `fields` queries now return their result directly rather than storing it in the object's children cache. That cache holds `WP_Comment` objects and is read back by `add_child()`, `get_child()`, and the 'flat' format, so writing an integer or a list of IDs into it left the object in a state where a later call returned the wrong thing. The recursive call that flattens descendants sets `count` and `fields` explicitly, since that path always merges objects. Document every argument core or the wider ecosystem passes: `fields`, `count`, `type`, `number`, `post_id`, and `order`, alongside a note that any other `WP_Comment_Query` argument is forwarded and that `parent` is always overridden. Several of these were already in use but undocumented. The `@phpstan-return` type is now conditional on `count`, then `fields`, then `format`, so each mode resolves at the call site. Follow-up to f9db5f5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Split the guard that short-circuits `count` and `fields` queries into two branches so that each is narrowed independently, and drop the inline `@var` that was previously asserting the result type. The combined condition could not be narrowed: because the two disjuncts are only provable together, the argument shape never resolved to `count: true` or to `fields: 'ids'`, and the conditional return type of `get_comments()` fell back to its full union, `array<non-negative-int|WP_Comment>|non-negative-int`. The inline `@var` hid that by overriding the inferred type rather than establishing it, which left the branch looking verified when it was not. With the branches separated, the count query is inferred as `non-negative-int` and the `fields` query as `non-negative-int[]|non-negative-int`. `WP_Comment` is eliminated from both, which is what the guard exists to guarantee, so the annotation is no longer needed. The duplicated `return` is what makes this work, and is commented as such: folding the two branches back into a single condition reinstates the wider union. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Set `count` to false before running an 'ids' query, so that the conditional return type of `get_comments()` resolves to `non-negative-int[]` rather than to `non-negative-int[]|non-negative-int`. The `$args` shape is open, since callers may pass any argument accepted by `WP_Comment_Query`. An open shape can always carry keys beyond the ones it names, so the absence of `count` cannot be established: neither the enclosing `elseif` nor an `unset()` is enough to rule out the `count` arm of the conditional. Assigning `false` states the same thing positively, which does contradict `count: true`, and the count arm drops out. This is a no-op at runtime, as `WP_Comment_Query` returns comment IDs before it consults `count`. Note in the comment that the two branches must not be merged. They carry identical return statements and invite being folded together, but each is narrowed on its own, and overwriting `count` is only correct in the branch where a count was not asked for. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the inline `@var` that typed the result of the children query with the same positive assignment used by the 'ids' branch, and correct the key type that made the annotation necessary in the first place. `get_comments()` declared its non-threaded result as `WP_Comment[]`, which admits string keys, while `WP_Comment::$children` holds `array<int, WP_Comment>`. That mismatch, together with the `count` and `fields` arms of the conditional that an open argument shape can never rule out, is what the annotation was suppressing. Every path in `WP_Comment_Query::get_comments()` produces integer keys: a threaded query keys by `comment_ID`, a flat query returns the assembled list of comments and their descendants, and a non-hierarchical query maps over a list of IDs. Declaring the branch as `array<int, WP_Comment>` makes the `hierarchical` test resolve to the same type either way, so it collapses. The same conditional in `get_approved_comments()` is updated to match. Setting `count` and `fields` explicitly once, after the guard, then covers the rest. This also makes the per-iteration assignments in the 'flat' loop redundant, and leaves `$child_comments` with nothing to do, since it existed only to carry the annotation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Correct `WP_Comment_Query::$comments`, which has no default value and so is null until a query has been run. `WP_Comment_Query::__construct()` only runs a query when it is given one, so a bare `new WP_Comment_Query()` leaves the property unset, as `get_approved_comments()` and `get_comments()` both rely on before calling `query()` separately. Its value type is also narrowed to `array<int, WP_Comment>`, matching the key type now declared by `get_comments()`. `$found_comments` and `$max_num_pages` become `non-negative-int`. The first is `FOUND_ROWS()`, a row count, and the second is `ceil()` of that count over a positive page size, so neither can be negative. The two `non-negative-int` types are not provable from the assignments, which cast with `(int)` rather than `absint()`, and each adds one error inside `WP_Comment_Query::get_comments()`. Those join the existing errors of the same kind in that method, all of which stem from the same two causes: `(int)` not expressing what the schema guarantees, and the comment ID cache being read back as `mixed`. They are left for a follow-up that addresses both together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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. |
westonruter
commented
Jul 20, 2026
Comment on lines
+394
to
+412
| /* | ||
| * A 'count' or 'ids' query returns an integer or a list of comment IDs rather than | ||
| * WP_Comment objects. Neither may be written to the children cache, which holds | ||
| * WP_Comment objects and is read back by add_child(), get_child(), and the 'flat' | ||
| * format below. Return the result directly and leave the cache untouched. The two | ||
| * branches must stay separate: each is narrowed independently, and `count` is only | ||
| * safe to overwrite in the 'ids' branch. | ||
| */ | ||
| if ( ! empty( $_args['count'] ) ) { | ||
| return get_comments( $_args ); | ||
| } elseif ( isset( $_args['fields'] ) && 'ids' === $_args['fields'] ) { | ||
| $_args['count'] = false; // For static analysis of the conditional return type. | ||
| return get_comments( $_args ); | ||
| } | ||
|
|
||
| // Only WP_Comment objects are returned past this point. Stated positively for static analysis. | ||
| $_args['count'] = false; | ||
| $_args['fields'] = ''; | ||
|
|
Member
Author
There was a problem hiding this comment.
The count and fields params have been passed core and in the ecosystem but without documentation or explicit support. In particular, wp_trash_comment() was updated in 27aa761 to pass 'fields' => 'ids' to $comment->get_children(). But this caused integers to be stored in $comment->children when it is supposed to be an array of WP_Comment instances.
Similarly, WP_REST_Comments_Controller::prepare_links() passes count => true to get_children(), and this was erroneously causing an integer to be stored as $comment->children.
So that is why this code is added here, to prevent corrupting the children cache.
westonruter
commented
Jul 20, 2026
| public function add_child( WP_Comment $child ) { | ||
| $this->children[ $child->comment_ID ] = $child; | ||
| public function add_child( WP_Comment $child ): void { | ||
| $this->children[ (int) $child->comment_ID ] = $child; |
Member
Author
There was a problem hiding this comment.
Note that using a numeric-string and an int are identical when working with array keys: https://3v4l.org/AU3Qr#veol
westonruter
commented
Jul 20, 2026
| * @param bool $set Whether the comment's children have already been populated. | ||
| */ | ||
| public function populated_children( $set ) { | ||
| public function populated_children( $set ): void { |
Member
Author
There was a problem hiding this comment.
The class is final so it is safe to add a return type.
westonruter
commented
Jul 20, 2026
| $post = get_post( $this->comment_post_ID ); | ||
| return property_exists( $post, $name ); | ||
| $post = get_post( (int) $this->comment_post_ID ); | ||
| return $post && property_exists( $post, $name ); |
Member
Author
There was a problem hiding this comment.
The $post check prevents an error when get_post() returns null.
westonruter
commented
Jul 20, 2026
Comment on lines
+509
to
+511
| if ( ! $post ) { | ||
| return null; | ||
| } |
Member
Author
There was a problem hiding this comment.
As above wit __isset(), this check prevents an error when attempting to get a property of null.
westonruter
commented
Jul 20, 2026
Comment on lines
+243
to
+244
| } elseif ( is_numeric( $comment ) ) { | ||
| $_comment = WP_Comment::get_instance( (int) $comment ); |
Member
Author
There was a problem hiding this comment.
This is moved up from the else, ensuring only an int or a numeric-string is passed into WP_Comment::get_instance().
westonruter
marked this pull request as ready for review
July 20, 2026 18:41
|
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. |
Align the magic getter with __isset(), which already requires a non-zero comment_post_ID: get_post() falls back to the global post when given an empty ID, so reading a post field on an unattached comment returned an unrelated post's value. Now null is returned, matching the contract documented for the proxied post fields. Also add @SInCE 7.1.0 entries to both magic methods covering this and the earlier hardening against the comment's post no longer existing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ARRAY_N branch returns array_values(), which always produces a sequentially-keyed list, so non-empty-list<mixed> is sharper than non-empty-array<int, mixed>. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ment() WP_Comment::get_children() no longer stores the result of a count or fields query in the children cache, and get_comment() now treats only numeric values as comment IDs rather than casting any unrecognized value to an integer. Record both changes with @SInCE 7.1.0 entries, matching the entries already added for the magic methods. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cover the behavior changes made on this branch: * WP_Comment::get_children() returns a count or a list of IDs directly, without poisoning the children cache, so a subsequent default query still returns WP_Comment objects. * WP_Comment::__isset() returns false rather than fataling when the comment's post has been deleted, and __get() returns null. * WP_Comment::__get() returns null for an unattached comment rather than reading the field from the global post via get_post( 0 ). * get_comment() treats only numeric values as comment IDs, so malformed numeric strings and other unrecognized values return null instead of being cast to an integer ID. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves type specificity across the comment APIs, continuing the work done for posts in r62437, r62648, r62694, and r62717.
WP_CommentData_Arrayarray shape describing the keys returned byWP_Comment::to_array(), and narrows the properties it covers:comment_approvedbecomesnon-empty-string, and the values core uses for it and forcomment_typeare documented.comment_typeis deliberately not narrowed, because comments created before 5.5.0 may store an empty string rather thancomment, which is whyget_comment_type()normalizes that case on read.WP_Comment::__get()proxies to the comment's post as@property-read, typed to match the correspondingWP_Postproperty. These were previously invisible to static analysis, IDE completion, and the generated documentation.$children, which is null untilget_children()populates it, and replaces thearray<int|numeric-string, WP_Comment>key type used across these APIs witharray<int, WP_Comment>. Anumeric-stringarray key cannot exist in PHP, since numeric string keys are coerced to integers on assignment.WP_Comment::get_children()count,fields, andformatmodes, and documents every argument core or the plugin ecosystem actually passes:fields,count,type,number,post_id, andorder. Several were already in use but undocumented.countorfieldsquery now returns its result directly rather than storing it in the children cache. That cache holdsWP_Commentobjects and is read back byadd_child(),get_child(), and theflatformat, so writing an integer or a list of IDs into it left the object returning the wrong thing on a subsequent call. This resolves aTODOleft in the method.One commit in this branch, "Add native return types", added an
arrayreturn type toget_children(), which made the method fatal when asked for a count.WP_REST_Comments_Controller::prepare_links()calls it withcount => true, so this raisedTypeError: WP_Comment::get_children(): Return value must be of type array, int returnedand failed 102 of the REST comment tests. It is reverted later in the branch, but is left in the history rather than rebased away, since the fix is easier to follow with the mistake visible.WP_Comment_Queryandget_comments()get_comments()andget_approved_comments(), keyed oncountandfields.get_approved_comments()is keyed on$post_idfirst, since it returns an empty array when that is falsey, before$argsis parsed at all.non-negative-int[], and typesWP_Comment_Query::$commentsas null until a query is run.WP_Comment_Query::__construct()only runs a query when given one, so a barenew WP_Comment_Query()leaves the property unset.get_comment()and runtime hardeningget_comment()now hands only numeric values toWP_Comment::get_instance(). Previously every unrecognized truthy value was cast to an integer insideget_instance(), soget_comment( '5abc' )resolved to comment 5 andget_comment( true )resolved to comment 1. Both now returnnull. Flagging for a dev note in case anything relies on the old coercion.WP_Comment::get_instance()ignores a non-object read from the comment cache rather than passing it to theWP_Commentconstructor, whereget_object_vars()would raise aTypeError.WP_Comment::__isset()returns false rather than raising aTypeErrorwhen the comment's post no longer exists, since it calledproperty_exists()with thenullreturned byget_post().WP_Comment::__get()returnsnullwhen the comment's post no longer exists (previously it raised a warning while reading a property onnull), and also when the comment is not attached to a post at all. In the unattached case,get_post( 0 )falls back to the global$post, so the getter returned an unrelated post's field even though__isset()reported the same property as unset.@since 7.1.0entry, as are the changes toWP_Comment::get_children()described above.PHPStan impact
Measured at rule level 10, with the result cache cleared before each pass.
trunk349 error signatures are resolved, concentrated in the files that consume the comment APIs:
property.nonObjectargument.typereturn.typeforeach.nonIterablemethod.nonObjectmissingType.iterableValueassign.propertyTypebinaryOp.invalidsrc/wp-includes/comment.phpsrc/wp-includes/comment-template.phpsrc/wp-admin/comment.phpsrc/wp-admin/includes/comment.phpsrc/wp-admin/includes/ajax-actions.phpsrc/wp-includes/class-wp-comment-query.phpsrc/wp-includes/pluggable.phpsrc/wp-admin/edit-form-comment.phpsrc/wp-includes/class-wp-comment.phpsrc/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.phpA raw diff of the two runs reports 349 fixed against 118 introduced. That overstates both columns: errors are matched on file, identifier, and message, so when a type gets sharper the same defect produces a different message and is counted once in each column. For example,
expects WP_Comment, mixed givenin the REST controller becomesexpects WP_Comment, int<0, max>|WP_Comment givenon the same line. The net figure is unaffected.Collapsing to per-file, per-identifier counts, only four pairs actually gain errors:
class-wp-comment-query.phpreturn.typeclass-wp-comment-query.phpassign.propertyTypewp-admin/includes/comment.phpassign.propertyTypecomment-template.phpforeach.nonIterableThe three in
wp-admin/includes/comment.phpare a genuine pre-existing problem this branch exposes rather than causes.get_comment_to_edit()has castcomment_IDandcomment_post_IDtointsince r5543, and writes them back onto properties documented as numeric strings, so it returns aWP_Commentthat does not match its own class. Every consumer inedit-form-comment.phpescapes on output and casts tointwhere it needs one, so the casts no longer serve their original purpose. Left alone here, since correcting it belongs in Administration rather than in this ticket.The one in
comment-template.phpfollows from$commentsbecoming nullable:comments_template()reads the property straight after constructing the query, and static analysis cannot see that the constructor ran it. An empty argument array would fail on the preceding line regardless, so the null is not reachable.Known gaps
Seven of the newly reported errors are inside
WP_Comment_Query, from two causes:(int)andintval()do not express what the unsigned schema guarantees, and the comment ID cache is read back asmixed. Both are fixable together in a follow-up. Switching the casts toabsint()resolves the first half, but is a runtime change and is out of scope here.The
comments_pre_queryfilter can return an arbitrarily shaped array that is assigned straight to$comments, and theget_commentfilter can return aWP_Commentwhose properties have been replaced, so these types describe the intended contract rather than a guarantee. That is the same posture taken by theWP_Postwork. See #12022.Testing
phpstan-diffis clean on the changed lines, PHPCS reports nothing new, and the comment, note, and REST comment suites pass locally (540, 78, and 200 tests respectively). The full suite has not been run locally and is left to CI.Trac ticket: https://core.trac.wordpress.org/ticket/64898
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: A review of the already-committed typing work, which surfaced most of the issues fixed here, and drafting of the annotations, docblocks, and commit messages. Every change was reviewed and frequently rewritten by me, and several of the better solutions are mine rather than the model's, including splitting the
get_children()guard into separate branches so each narrows independently, and assertingcountas present-and-false rather than absent so the conditional return type resolves. The model also asserted at one point that core's REST controller passed only documented arguments, which was wrong and which I caught; verifying that claim is what uncovered theTypeErrordescribed above.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.