Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the codebase's type safety and static analysis compatibility by adding comprehensive PHPDoc annotations, implementing variadic parameters for output functions, and introducing a PHPStan configuration. Key refactorings include updating the lexer logic in Arguments.php, ensuring consistent return types across various classes, and adding explicit type casting for environment variables and calculations. The review feedback highlights a few remaining inconsistencies, specifically regarding indentation and method usage in Arguments.php, and a missing variadic parameter in the line() function to match the updated out() and err() signatures.
| $this->_lexer->next(); | ||
|
|
||
| // Loop until we find a flag in peak-ahead | ||
| foreach ($this->_lexer as $value) { | ||
| array_push($values, $value->raw); | ||
| while ( $this->_lexer->valid() ) { | ||
| $value = $this->_lexer->current(); | ||
| array_push( $values, $value->raw ); | ||
|
|
||
| if (!$this->_lexer->end() && !$this->_lexer->peek->isValue) { | ||
| if ( ! $this->_lexer->end() && ! $this->_lexer->peek->isValue ) { | ||
| break; | ||
| } | ||
| $this->_lexer->next(); | ||
| } |
There was a problem hiding this comment.
The newly added block uses spaces for indentation, which is inconsistent with the rest of the file that uses tabs. Additionally, for consistency with the change made on line 449, it is recommended to use the raw() method instead of the magic property raw. The spacing inside parentheses is also inconsistent with the existing codebase.
$this->_lexer->next();
// Loop until we find a flag in peak-ahead
while ($this->_lexer->valid()) {
$value = $this->_lexer->current();
array_push($values, $value->raw());
if (!$this->_lexer->end() && !$this->_lexer->peek->isValue) {
break;
}
$this->_lexer->next();
}| * Prints a message to `STDOUT` with a newline appended. See `\cli\out` for | ||
| * more documentation. | ||
| * | ||
| * @param string $msg Message to print. | ||
| * @return void | ||
| * @see cli\out() | ||
| */ | ||
| function line( $msg = '' ) { |
There was a problem hiding this comment.
The line() function is missing the variadic ...$args parameter in its signature and docblock. This is inconsistent with the updates made to out(), out_padded(), and err(), and may cause issues with static analysis (PHPStan) when the function is used with formatting arguments.
/**
* Prints a message to `STDOUT` with a newline appended. See `\cli\out` for
* more documentation.
*
* @param string $msg Message to print.
* @param mixed ...$args Either scalar arguments or a single array argument.
* @return void
* @see cli\out()
*/
function line( $msg = '', ...$args ) {
There was a problem hiding this comment.
Pull request overview
This PR introduces an initial PHPStan setup for the project and updates a broad set of library files with more precise PHPDoc/typing and small refactors to make the codebase pass stricter static analysis.
Changes:
- Add
phpstan.neon.distwith level 9 configuration and WordPress/WP-CLI stubs scanning. - Tighten PHPDoc types across CLI components (tables, trees, streams, arguments, notify/progress).
- Refactor a few implementations for safer scalar casting and iterator handling (notably arguments parsing and stream rendering).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| phpstan.neon.dist | Adds baseline PHPStan configuration (level, paths, stubs). |
| lib/cli/tree/Renderer.php | Narrows $tree PHPDoc type for static analysis. |
| lib/cli/tree/Markdown.php | Narrows $tree PHPDoc type for static analysis. |
| lib/cli/tree/Ascii.php | Narrows $tree PHPDoc type for static analysis. |
| lib/cli/Tree.php | Adds property/param/return PHPDoc types for PHPStan. |
| lib/cli/table/Tabular.php | Adds stricter row typing and safer string conversions. |
| lib/cli/table/Renderer.php | Adds property/param typing and return annotations. |
| lib/cli/table/Ascii.php | Adds property typing and multiple safety casts/guards. |
| lib/cli/Table.php | Adds property typing; constructor refactor to sanitize inputs. |
| lib/cli/Streams.php | Adds typing/docs; refactors render/out/err to variadics and safer casting. |
| lib/cli/Shell.php | Minor robustness tweak and adds return annotation. |
| lib/cli/progress/Bar.php | Adds property typing; tightens numeric-to-string conversions. |
| lib/cli/Progress.php | Adds typing/docs; small return type normalization. |
| lib/cli/notify/Spinner.php | Adds property typing/docs. |
| lib/cli/notify/Dots.php | Adds property typing; initializes iteration counter. |
| lib/cli/Notify.php | Adds property typing/docs; adjusts speed/time formatting. |
| lib/cli/Memoize.php | Adds typing/docs; refactors memoized getter invocation. |
| lib/cli/Colors.php | Adds typing/docs; adds guards/asserts for split/substr results. |
| lib/cli/cli.php | Updates function signatures/docs; adds type guards and stronger checks. |
| lib/cli/arguments/Lexer.php | Adds typing/docs; makes shifting/exploding safer for null cases. |
| lib/cli/arguments/InvalidArguments.php | Adds typing/docs for stored invalid arguments. |
| lib/cli/arguments/HelpScreen.php | Adds typing/docs; adds scalar-safe formatting of descriptions/defaults. |
| lib/cli/arguments/Argument.php | Adds property-read PHPDoc and stronger typing in exploded(). |
| lib/cli/Arguments.php | Adds typing/docs; improves input normalization and option parsing iteration. |
Comments suppressed due to low confidence (2)
lib/cli/Arguments.php:589
- The "no value given" check intends to allow a default value of
0, but the condition currently compares the entire$optionSettingsarray to0($optionSettings !== 0), which is always true. As a result, options with default0will incorrectly warn and be set tonull. Compare against$optionSettings['default']instead (or explicitly handlearray_key_exists('default', ...)).
if ($this->_lexer->end() || !$this->_lexer->peek->isValue) {
$optionSettings = $this->getOption($option->key);
if (empty($optionSettings['default']) && $optionSettings !== 0) {
// Oops! Got no value and no default , throw a warning and continue.
$this->_warn('no value given for ' . $option->raw);
$this[$option->key] = null;
} else {
// No value and we have a default, so we set to the default
$this[$option->key] = $optionSettings['default'];
}
lib/cli/cli.php:349
grapheme_strlen()returnsint|falseon failure (notnull). The current checknull !== ( $width = grapheme_strlen(...) )will treatfalseas a valid width and return early (coercingfalseto0), skipping the safer fallbacks. Use anis_int(...)/false !== ...check consistent withsafe_strlen()to avoid mis-measuring non‑UTF8 strings.
// Assume UTF-8 if no encoding given - `grapheme_strlen()` will return null if given non-UTF-8 string.
if ( ( ! $encoding || 'UTF-8' === $encoding ) && can_use_icu() && null !== ( $width = grapheme_strlen( $string ) ) ) {
if ( ! $test_strwidth || ( $test_strwidth & 1 ) ) {
return $width + preg_match_all( $eaw_regex, $string, $dummy /*needed for PHP 5.3*/ );
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( ! empty( $headers ) ) { | ||
| // If all the rows is given in $headers we use the keys from the | ||
| // first row for the header values | ||
| if ($rows === array()) { | ||
| $rows = $headers; | ||
| $keys = array_keys(array_shift($headers)); | ||
| $headers = array(); | ||
| if ( $rows === array() ) { | ||
| $rows = $headers; | ||
| $first_row = array_shift( $headers ); | ||
| $keys = is_array( $first_row ) ? array_keys( $first_row ) : array(); | ||
| $headers = array_map( $safe_strval, $keys ); | ||
| } else { | ||
| $headers = array_map( $safe_strval, $headers ); | ||
| } | ||
|
|
||
| foreach ($keys as $header) { | ||
| $headers[$header] = $header; | ||
| $this->setHeaders( $headers ); | ||
|
|
||
| $safe_rows = array(); | ||
| foreach ( $rows as $row ) { | ||
| if ( is_array( $row ) ) { | ||
| $safe_rows[] = array_map( $safe_strval, $row ); | ||
| } | ||
| } | ||
|
|
||
| $this->setHeaders($headers); | ||
| $this->setRows($rows); | ||
| $this->setRows( $safe_rows ); |
There was a problem hiding this comment.
In the constructor path where $rows === array() (hash-table rows passed via $headers), $headers is converted to a numeric list of column names (array_map on $keys), while $safe_rows preserves the original associative keys from each row. This can leave header row widths keyed by integers and data row widths keyed by strings, causing misaligned output / undefined width indexes in renderers. Preserve the original keys (e.g., build $headers as an associative array mapping each key to itself, and ensure rows are normalized to the same key set/order).
| public function render() { | ||
| $help = array(); | ||
|
|
||
| array_push($help, $this->_renderFlags()); | ||
| array_push($help, $this->_renderOptions()); | ||
| array_push( $help, $this->_renderFlags() ); | ||
| array_push( $help, $this->_renderOptions() ); | ||
|
|
||
| return join("\n\n", $help); | ||
| return join( "\n\n", $help ); | ||
| } |
There was a problem hiding this comment.
render() always pushes the results of _renderFlags() / _renderOptions() into $help, but those methods can return null. join("\n\n", $help) will then introduce leading/trailing blank sections when only flags or only options exist. Filter out null/empty sections before joining (e.g., array_filter with static fn($v) => $v !== null && $v !== '').
|
/gemini review |
No description provided.