Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Code Review
This pull request integrates PHPStan for static analysis, adding a configuration file and comprehensive PHPDoc type hints throughout the project. It also refactors path handling and logic in Option_Value. Review feedback highlights critical issues: changing $_when to a string in File.php breaks check discovery, removing the defined property check in Constant_Definition incorrectly alters validation logic, and casting potentially non-scalar values to strings in human_value may trigger PHP warnings.
| * File checks are run as their own group. | ||
| */ | ||
| protected $_when = false; | ||
| protected $_when = 'manual'; // Run manually via group |
There was a problem hiding this comment.
Changing $_when from false to 'manual' breaks the discovery logic for file-based checks. In src/Command.php, the code identifies File checks by checking if get_when() returns a falsy value (see lines 142 and 156-161). By setting this to a string, these checks will be incorrectly treated as regular hooked checks and will not be added to the $file_checks array, causing the filesystem scan to skip them entirely. If this change was made to satisfy PHPStan's type requirements, consider updating the base class type hint or using a @phpstan-ignore instead.
protected $_when = false;| } | ||
|
|
||
| if ( $this->defined && ! isset( $this->value ) ) { | ||
| if ( ! isset( $this->value ) ) { |
There was a problem hiding this comment.
Removing $this->defined from this condition changes the behavior of the check. Previously, this block only executed if the configuration explicitly requested a definition check (e.g., defined: true in YAML). Now, it will execute for any constant check where a value is not specified, potentially leading to incorrect success reports if the constant is not actually defined. If PHPStan is flagging $this->defined as an undefined property, it should be explicitly declared in the class rather than removed from the logic.
if ( $this->defined && ! isset( $this->value ) ) {
src/Check/Constant_Definition.php
Outdated
| return 'null'; | ||
| } | ||
| return $value; | ||
| return (string) $value; |
There was a problem hiding this comment.
Casting $value directly to a string can cause a PHP notice or warning if the constant's value is an array (e.g., Array to string conversion). Since WordPress constants can occasionally be arrays or other non-scalar types, it's safer to handle these cases explicitly.
return is_scalar( $value ) ? (string) $value : json_encode( $value );
No description provided.