Make WordPress Core

Opened 5 weeks ago

Last modified 5 weeks ago

#66049 new enhancement

Build PHPStan extensions for query-string $args passed to wp_parse_args()-based template tags

Reported by: westonruter Owned by:
Priority: normal Milestone: Awaiting Review
Component: Build/Test Tools Version:
Severity: normal Keywords:
Cc: Focuses: coding-standards

Description

Follow-up to #65817, where r63440 and r63441 gave the dual-mode template tags conditional @phpstan-return annotations so that consuming the result of a call that only prints is reported as an error.

As mentioned in a ​PR comment, those annotations cannot describe one case: an $args passed as a query string. This ticket proposes two PHPStan extensions to close that gap, and reports a survey of how often the query-string form is actually used in core, in themes and in plugins.

The problem

Around two dozen template tags accept $args as either an array or a URL-encoded query string, which wp_parse_args() runs through wp_parse_str() (which also come to find out has a wp_parse_str filter). A conditional return type keys off the static type of the argument at the call site, so an array shape can be matched but a string cannot:

<?php
// Decidable: the shape is visible, the tag prints, the result is void.
wp_list_authors( array( 'echo' => true ) );

// Undecidable: 'echo=0' is a ConstantStringType, never a subtype of any array shape.
$html = wp_list_authors( 'echo=0' );

Every such tag therefore ends its conditional with a branch that leaves the type a union so nothing is reported either way. For most tags that branch reads string|null. For wp_dropdown_languages() it has to read string|void, because that function bails with a bare return; rather than return null; — putting void back inside a union, which is the thing ​previous ​PRs set out to eliminate.

PHPStan offers no hook to customize how X is Y is decided inside a conditional type, so this cannot be fixed in a docblock. It needs an extension.

Survey: core

All 61 functions in src/ documented array|string (or string|array) that call wp_parse_args() were identified, then narrowed to the 24 free functions where argument 1 is the args. Every call site in src/ and tests/phpunit/ was then classified.

First argument Call sites
Array literal 61
Variable or expression 420
No argument 196
String literal 18

The 18 string call sites:

Area Count Detail
Bundled legacy themes 14 13 of them the_title_attribute( 'echo=0' ) in Twenty Ten through Twenty Sixteen, plus wp_get_archives( 'type=monthly' ) in Twenty Ten
Tests 3 get_pages(), get_posts(), wp_list_pages()
src/wp-includes/theme-compat/sidebar.php 1 wp_list_pages( 'title_li=<h2>' . __( 'Pages' ) . '</h2>' )

src/wp-includes proper and src/wp-admin contain no query-string call sites at all. The single non-theme instance is in theme-compat/, itself legacy scaffolding.

The practical consequence is that the undecidable branch costs core's own analysis almost nothing. Its entire effect lands on themes and plugins, which consume these annotations through ​php-stubs/wordpress-stubs.

Survey: themes

Measured across the 14,941 themes in the WordPress.org directory.

Function Array form Query-string form Themes with at least one
the_title_attribute 1,876 4,329 1,280
wp_list_categories 2,114 1,815 1,444
wp_get_archives 2,919 1,696 1,000
wp_list_pages 986 1,482 1,032
wp_list_comments 6,779 726 583
wp_tag_cloud 243 484 445
wp_link_pages 21,327 439 184
wp_list_bookmarks 18 292 272
wp_dropdown_categories 453 223 215
wp_page_menu 375 60 53
paginate_comments_links 158 57 47
wp_list_authors 5 30 24

The idiom is not confined to abandoned themes. It ships today in GeneratePress (500k installs), Lightning (100k), Hestia (80k), Twenty Ten (50k), Ashe (30k), Hueman (20k) and Rife Free (20k), as well as in core's own bundled themes.

Survey: plugins

Measured across the 74,641 plugins in the WordPress.org directory. Roughly 650 query-string call sites across 350 to 400 distinct plugins.

Function Array form Query-string form
the_title_attribute 276 289
wp_dropdown_categories 850 95
wp_list_pages 60 90
wp_get_archives 23 50
wp_list_categories 82 41
wp_dropdown_pages 647 28
wp_list_comments 146 14
wp_tag_cloud 32 11
wp_dropdown_users 280 10
wp_link_pages 183 8
wp_list_authors 16 8
wp_list_bookmarks 4 4
wp_page_menu 2 0

That is around 0.5% of the whole directory, but roughly 10 to 20% of the plugins that call these functions at all. Actively maintained, high-install plugins still use it: GenerateBlocks (200k), YARPP (100k), VK All in One Expansion Unit (100k), WPtouch (60k), Easy Digital Downloads (40k).

Method and caveats

  • Core figures come from a scan of src/ and tests/phpunit/, excluding bundled third-party libraries. The scan is multi-line aware; a second line-based pass produced identical totals.
  • Theme and plugin figures were gathered by regex search over indexed copies of both directories, matching the function name followed by ( and then either a quote (query-string form) or array(/[ (array form).
  • Those searches are line-scoped, so a call whose array( sits on the following line is counted in neither column. The leakage was measured for two functions: 247 additional multi-line array calls for wp_dropdown_categories() and 405 for wp_dropdown_pages(). The percentages above therefore overstate the query-string share; the absolute counts are sound.
  • Args passed through a variable are counted in neither column, so query-string figures are a floor rather than a ceiling.
  • Only the latest indexed version of each theme and plugin was searched.
  • Query-string matches were manually sampled to discard commented-out code, docblock examples, hook and callback names, and calls whose first parameter is not the args.

Proposal: two extensions

These are two distinct PHPStan extension points, best implemented together because they share one table of metadata: for each supported function, which parameter carries the args and what its $defaults array contains.

1. A dynamic return type extension

A PHPStan\Type\DynamicFunctionReturnTypeExtension, the same interface already used in this repo by tests/phpstan/ApplyFiltersDynamicFunctionReturnTypeExtension.php. For a supported call it inspects the argument type via Scope::getType() and:

  • ConstantStringType — runs parse_str() on the literal during analysis, merges the result over the function's defaults, evaluates the same branch logic the docblock encodes, and returns VoidType, StringType or whatever that branch resolves to.
  • ConstantArrayType — the same merge and the same decision, replacing what the conditional does today.
  • anything else — returns null so the docblock type stands.

This makes the_title_attribute( 'echo=0' ) resolve to exactly string and wp_dropdown_languages( 'id=0' ) resolve to exactly void, removing the undecidable branch for literal arguments.

2. A rule forbidding non-array $args

A PHPStan\Rules\Rule that reports passing anything other than an array to these parameters. Unlike the inference extension it does not need the value to be knowable, so it also covers concatenated and interpolated strings.

👉 Functions which are passed a query string could trigger a _doing_it_wrong() to encourage passing args arrays as they can more readily be statically-analyzed.

Why both

Only a bare literal is a ConstantStringType. Anything built up loses its value:

Expression Type PHPStan sees Extension can resolve?
'echo=0' 'echo=0' yes
'title_li=' . $x non-falsy-string no
"type=$foo" non-falsy-string no

The dominant list-tag idiom is concatenation ('title_li=&echo=0&child_of=' . $id), so inference alone leaves much of the ecosystem untyped. The rule covers those. Conversely the rule cannot improve a type, so it does nothing for the annotations. They are complementary.

Both of core's own non-theme string call sites are concatenations or interpolations, so the inference extension would do nothing for either; only the rule reaches them.

Side finding: a live bug the rule would catch

28 themes still call wp_link_pages() with its pre-2.1 positional signature:

<?php
wp_link_pages( '<p>Pages: ', '</p>', 'number' );

wp_link_pages() has no positional back-compat handling. The second and third arguments are discarded, and parse_str( '<p>Pages: ' ) yields the single junk key '<p>Pages:_' => '', so every default survives and the theme silently renders default markup instead of its own. Inference can never flag this, since the string is not a valid args string in the first place. A rule forbidding non-array args reports it immediately.

Open questions

  1. Where should these live? In tests/phpstan/ they only help core analyze itself, where the survey shows there is almost nothing to find. To reach the themes and plugins that actually use the idiom they would need to ship in ​szepeviktor/phpstan-wordpress or alongside php-stubs/wordpress-stubs. That argues for developing them outside core, with core consuming them.
  2. Soundness. wp_parse_str() applies a wp_parse_str filter to the parsed result, and a few tags filter $args before parsing — wp_get_archives() runs apply_filters( 'wp_get_archives_args', $args ) ahead of wp_parse_args(), and get_calendar() filters afterwards. Only two plugins in the directory filter wp_parse_str at all, so that hook is not a practical concern, but wp_get_archives() and get_calendar() may warrant exclusion from the inference extension.
  3. Defaults drift. The metadata table duplicates each function's $defaults. A test asserting the table matches the source would prevent it going stale, unless the defaults can be extracted from the call to wp_parse_args() at runtime.
  4. Should the rule be adopted by core at all? It would flag one file in theme-compat/, three test call sites and the bundled legacy themes. That is cheap, but it mostly enforces a state core has already reached.

Not proposed here

A wp_parse_args() return type extension, typing $parsed_args inside these functions as a precise shape rather than array<string, mixed>, would remove a good deal of mixed-related noise in their bodies. That is a separate concern and does not affect call-site resolution, since the conditional keys off the caller's argument.


🤖 This ticket was drafted with help of Claude Opus 5.

Change History (4)

#1 @westonruter
5 weeks ago

In 63487:

Code Quality: Refine the dual-mode template tag annotations.

  • Restore void to the @return unions of wpdb::print_error() and wpdb::check_database_version(), which r62177 replaced with null.
  • Add the null that fifteen tags describe in prose but omit from a @return listing only string|void.
  • Match every falsy spelling of a display flag, and every argument shape reaching a bail, so those calls resolve to void; a query string still cannot be read, tracked in #66049.
  • Narrow the deprecated param for trackback_url() so that static analysis catches the obsolete usage.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/13371.
Follow-up to r32568, r61766, r61768, r62177, r62178, r63440, r63441.

Props marian1, westonruter.
See #64703, #64704, #65817, #66049.

#2 @westonruter
5 weeks ago

In 63488:

Taxonomy: Fix wp_get_object_terms() when requesting a count.

Requesting a count passed the numeric string get_terms() returns straight to array_merge(), which warns and yields null on PHP 7.4 and throws a TypeError on PHP 8.0 and later. The count is now cast before it is merged, and summed once every taxonomy has been queried, so the function returns the numeric string its documentation has described since r49947. An empty object or taxonomy list returns '0' for the same reason, rather than the empty array that contradicted the documented type.

A taxonomy registered with an args array is queried by a separate recursive call, and those results were merged with array_merge() unconditionally. For the id=> values of fields the term IDs are integer array keys, which array_merge() renumbers, so such a taxonomy came back keyed from zero. The recursive branch now uses the union operator for those, as the merge for the remaining taxonomies has done since r41809.

The functions get_terms(), wp_get_object_terms(), wp_count_terms(), wp_get_post_categories(), wp_get_post_tags() and wp_get_post_terms() each gain a conditional return type that resolves the result from the fields value, falling back to what that function's own default answers with, as was done for WP_Term_Query::query() in r63358.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/7278.
Follow-up to r38667, r40513, r41809, r49947, r63358.

Props marian1, westonruter, swissspidy.
See #65817, #66049.
Fixes #61936.

Note: See TracTickets for help on using tickets.