Make WordPress Core

Opened 17 months ago

Last modified 17 months ago

#57299 new enhancement

Implement array key type notation

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: 2nd-opinion
Focuses: docs, coding-standards Cc:

Description (last modified by johnbillion)

I'd like to propose that array key type notation is introduced into PHP docblocks where appropriate. This notation uses the syntax array<key-type, value-type> for arrays, for example a list containing strings is documented thus:

/**
 * @param array<int, string> $foo
 */

An associative array of booleans (where the shape is not known) is documented thus:

/**
 * @param array<string, bool> $foo
 */

The benefit of this syntax over, for example, string[] or array<string> is it allows the types of the array keys to be specified. This allows both lists and associative arrays to be documented more completely even when their shape is not known.

When used in combination with a static analysis tool such as PHPStan this allows for greater type safety and more accurate analysis of structures such as array access and array iteration. It allows developers looking at the documentation to understand the type of the array keys, and thus whether an array is a list or associative. That said, I appreciate that this syntax is comparatively rare within the WordPress ecosystem and therefore can be foreign to developers who've not seen it elsewhere.

This notation is supported by all of the static analysis tools and code editors (either natively or via a PHP add-on) that I could find, including VS Code, PHPStorm, Sublime Text, PHPStan, Psalm, and Phan, and it's used by countless other frameworks and libraries such as Symfony, Laravel, and PHP Parser. It's not a new syntax, it's just new to WordPress core.

Benefits

  • Increased awareness of whether an array is a list or an associative array for developers reading inline documentation
  • Increased accuracy provided to static analysis tools
  • Increased accuracy in editors and IDEs that either natively support this syntax or support the PHP implementation of the language server protocol

Concerns

  • Syntax that can be jarring for developers who've not seen it before
  • Not part of a phpDocumentor or PSR-5 PHPDoc standard (although PSR-5 has been stalled for 9 years so probably not a concern)

Implementation

Much like the general ongoing improvements to inline docs, this will be a gradual process. There won't be a patch or PR that updates all the existing type[] notation at once.

Any objections?

Change History (6)

#1 @johnbillion
17 months ago

  • Type changed from defect (bug) to enhancement

#2 @johnbillion
17 months ago

Oh I forgot to add there's also a shorter list<string> syntax for lists (arrays with sequential integer keys), and while this is well supported in static analysis tools it's much less supported in editors and IDEs. The Intelephense extension for VS Code doesn't support it, for example, although PHPStorm does appear to. I recommend for now that we don't implement list<>.

#3 follow-up: @jrf
17 months ago

How would this play together with the WP array annotations used for parameters/hooks ?
How would one go about documenting arrays with mixed types, i.e. different types for different/specific associative array keys ?

Would this also apply to return and hook types ?

For the record: the WP array format used for parameters/hooks is used ONLY in WP and nowhere else in the PHP world, which also means that virtually no tooling supports it unless WP specific extensions have been written for them. The format proposed by @johnbillion is a far more common and widely accepted format.

Also missing any mention of PHP_CodeSniffer in your list of tools, while that is the typical tooling used to check docs styling in CI in the WP world.

P.S.: this is not about inline documentation, but about docblocks. You may want to change that in the description.

#4 in reply to: ↑ 3 @johnbillion
17 months ago

  • Description modified (diff)

Replying to jrf:

How would this play together with the WP array annotations used for parameters/hooks ?

I think they can go hand-in-hand as they partially do at the moment with the existing type[] syntax. Given this example, this is the before:

* @return string[] {
*     Array containing JOIN and WHERE SQL clauses to append to the main query.
*
*     @type string $join  SQL fragment to append to the main JOIN clause.
*     @type string $where SQL fragment to append to the main WHERE clause.
* }

And this is the after:

* @return array<string, string> {
*     Array containing JOIN and WHERE SQL clauses to append to the main query.
*
*     @type string $join  SQL fragment to append to the main JOIN clause.
*     @type string $where SQL fragment to append to the main WHERE clause.
* }

The array<key-type, value-type> syntax allows for nesting. Whether or not that's necessary or appropriate for multi-dimensional arrays can be made on a case by case basis.

How would one go about documenting arrays with mixed types, i.e. different types for different/specific associative array keys ?

mixed is a valid type. There's currently three instances of mixed[] in core. These can either remain as mixed[] if the key types are unknown, or converted to array<int, mixed>, array<string, mixed>, or array<int|string, mixed> as appropriate to be more explicit.

Alternatively, union types are valid in this notation just as they are without it, although I am not sure if there's any instances of this in core currently, for example (string|int)[] could become array<int, string|int> if it's a list.

Would this also apply to return and hook types ?

IMO it should apply to @param, @return, and @var tags, which therefore includes hook documentation, and also @type tags inside the WP-specific object hash notation. I will need to check whether the WP-Parser library used to generate the documentation for developer.wordpress.org needs any updates too.

Also missing any mention of PHP_CodeSniffer in your list of tools, while that is the typical tooling used to check docs styling in CI in the WP world.

I did include PHPCS in my list originally, but then it crossed my mind that I wasn't aware of explicit support for this notation in PHPCS so I left it out. If it is supported then great!

P.S.: this is not about inline documentation, but about docblocks. You may want to change that in the description.

👍

#5 @GaryJ
17 months ago

When the keys are known, you can also use Array Shapes:

It's a step further than the regular array<string, array> format (which itself is more descriptive than just array), but relatively well supported amongst tooling.

As an example, the add_settings_field() DocBlock would have a last param tag go from the custom WP format of:

 * @param array    $args {
 *     Optional. Extra arguments that get passed to the callback function.
 *
 *     @type string $label_for When supplied, the setting title will be wrapped
 *                             in a `<label>` element, its `for` attribute populated
 *                             with this value.
 *     @type string $class     CSS Class to be added to the `<tr>` element when the
 *                             field is output.
 * }

to:

 * @param array{label_for: string, class: string} $args

... which can then be used by IDEs for autocompletion and validated against with static analysis tooling.

One concern might be that there's no longer room for a description by each key. It would need testing in the tooling, but it may be that the rest of the param tag description is unaffected and so the following may work for every reader:

 * @param array{label_for: string, class: string} $args {
 *     Optional. Extra arguments that get passed to the callback function.
 *
 *     @type string $label_for When supplied, the setting title will be wrapped
 *                             in a `<label>` element, its `for` attribute populated
 *                             with this value.
 *     @type string $class     CSS Class to be added to the `<tr>` element when the
 *                             field is output.
 * }

Or these array key descriptions could be moved into the main DocBlock description:

 * When the label_for argument is supplied, the setting title will be wrapped...
 * The class argument is the CSS class to be added to the `<tr>` element...
 * ...
 * @param array{label_for: string, class: string} $args

That keeps all of the descriptions in one place, and avoids key and key-type duplications, and allows the actual param tag to follow exactly what tooling expects.

#6 @GaryJ
17 months ago

While the draft PSR-5 does allow the use of the string[] format rather than array<string>, I've found the former to be illogical - having the type of items on the outside of the thing representing the array (think of a $my_array[] variable as the array) doesn't make sense to me. Plus, using the latter format works for both indexed and associative arrays, so there's a consistency there that the string[] format doesn't give.

Note: See TracTickets for help on using tickets.