Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement short inline syntax for array parameters and return values #15

Closed
johnbillion opened this issue Jan 3, 2021 · 10 comments · Fixed by #21
Closed

Implement short inline syntax for array parameters and return values #15

johnbillion opened this issue Jan 3, 2021 · 10 comments · Fixed by #21

Comments

@johnbillion
Copy link
Contributor

Neither PHPStan nor Psalm support the hash-style notation used by WordPress for its array parameters and array return values.

@param array $args {
    Array of arguments used to display the help tab.

    @type string   $title    Title for the tab. Default false.
    @type string   $id       Tab ID. Must be HTML-safe and should be unique for this menu.
                             It is NOT allowed to contain any empty spaces. Default false.
    @type string   $content  Optional. Help tab content in plain text or HTML. Default empty string.
    @type callable $callback Optional. A callback to generate the tab content. Default false.
    @type int      $priority Optional. The priority of the tab, used for ordering. Default 10.
}

This means there's currently no way to inform static analysis tools of the types of the array elements passed to or returned by functions such as wp_remote_post() and classes such as WP_Query.

It would be great to:

  1. Post-process the stubs after they're generated and convert hash-style array notation in the docblocks into short inline syntax. Example:
    - * @param array $args {
    - *     Array of arguments used to display the help tab.
    - *
    - *     @type string   $title    Title for the tab. Default false.
    - *     @type string   $id       Tab ID. Must be HTML-safe and should be unique for this menu.
    - *                              It is NOT allowed to contain any empty spaces. Default false.
    - *     @type string   $content  Optional. Help tab content in plain text or HTML. Default empty string.
    - *     @type callable $callback Optional. A callback to generate the tab content. Default false.
    - *     @type int      $priority Optional. The priority of the tab, used for ordering. Default 10.
    - * }
    + * @param array{
    + *     title?: string,
    + *     id?: string,
    + *     content?: string,
    + *     callback?: callable,
    + *     priority?: int,
    + * } $args
  2. Create a mapping from function parameters such as wp_remote_post() $args to their canonical function parameter such as WP_Http::request() $args and then apply the same docblock from the canonical function in its wrapper functions. This allows for parameter type checking in wrapper functions which are more commonly used. This likely needs to happen as a separate second step as it will be a manual process to create the mapping. WordPress core isn't well documented or consistently documented in this regard.

Approach

It's possible that johnbillion/wp-parser-lib could be used to aid the transformation as it's able to tokenise the hash style array notation.

Considerations

Unfortunately the short inline syntax supported by PHPStan and Psalm don't allow for human-readable documentation of the array elements. This means the stubs will be better for PHPStan and Psalm but worse for humans. Maybe two sets of stubs needs to be created, one with the human-readable docblock from core and one with the short inline syntax for use in static analysis.

Refs

@szepeviktor
Copy link
Member

szepeviktor commented Jan 3, 2021

There is room for improvement in core's source code.
That "hash-style" thing is from JavaScript. Only WordPress uses it in PHP.

This package does not alter the source code in any way, simply removes function contents.
I am open to add (rock solid) transformations!

@szepeviktor szepeviktor pinned this issue Jan 3, 2021
@johnbillion
Copy link
Contributor Author

A few observations:

  • Core uses type string|array where the parameter technically also accepts a URL query string because the parameter gets passed through wp_parse_args(). I think in the context of using a tool such as PHPStan or Psalm this should be treated as array only because nobody in their right mind actually uses a query string.
  • The error message from PHPStan when an incorrect type is used is not very useful if there is a large number of array elements:

    Parameter #2 $args of method WP_Http::request() expects array(?'method' => string, ?'timeout' => float, ?'redirection' => int, ?'httpversion' => string, ?'user-agent' => string, ?'reject_unsafe_urls' => bool, ?'blocking' => bool, ?'headers' => array|string, ...), array('blocking' => WP_User) given.

@johnbillion
Copy link
Contributor Author

  • Additional array elements that are not in the documented list are allowed by PHPStan:
( new WP_Http )->request( 'http://example.com', [
	'banana' => 'yellow',
] );

[OK] No errors

@szepeviktor
Copy link
Member

  • Developers above 14 years should be punished for storing various data in an array
  • I'm just a single person, not capable of processing a huge codebase like WordPress

@szepeviktor
Copy link
Member

Developers above 14 years should be punished for storing various data in an array

PHP has object properties for more than a decade. Code could be written in a human-friendly way.

https://github.com/szepeviktor/small-project/blob/de32f4b232967f43d0d9532febc97f81ba8ec6ad/plugin-name.php#L92-L99

@johnbillion
Copy link
Contributor Author

PHP has object properties for more than a decade. Code could be written in a human-friendly way.

Oh yes I completely agree, these array-style arguments are a nightmare. I regret using them in some of my libraries such as Extended CPTs. This is why I started playing around with johnbillion/args. Unfortunately we're stuck with them in WordPress core unless we decided to implement such classes.

@szepeviktor
Copy link
Member

szepeviktor commented Jan 3, 2021

Unfortunately we're stuck with them in WordPress core

Smart people started building dirt-hiding-layers between WordPress core and project-specific code - meaning what you write and read.

dirt=techdebt

@johnbillion
Copy link
Contributor Author

Alternative: Convince PHPStan and Psalm to support this hash style notation.

@szepeviktor
Copy link
Member

szepeviktor commented Jan 3, 2021

Alternative: Convince PHPStan and Psalm to support this hash style notation.

WordPress marches into a dead-end. Let's convince others to join :)

@johnbillion
Copy link
Contributor Author

Potential solution for the machine-readable versus human-readable: Use @phpstan-param and @phpstan-return tags in addition to the existing @param and @return tags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants