Conversation
|
Hello! 👋 Thanks for opening this pull request! Please check out our contributing guidelines. We appreciate you taking the initiative to contribute to this project. Contributing isn't limited to just code. We encourage you to contribute in the way that best fits your abilities, by writing tutorials, giving a demo at your local meetup, helping other users with their support questions, or revising our documentation. Here are some useful Composer commands to get you started:
To run a single Behat test, you can use the following command: # Run all tests in a single file
composer behat features/some-feature.feature
# Run only a specific scenario (where 123 is the line number of the "Scenario:" title)
composer behat features/some-feature.feature:123You can find a list of all available Behat steps in our handbook. |
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 ) ) {| 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.