-
Notifications
You must be signed in to change notification settings - Fork 26
Improve compatibility with other operating systems #274
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
base: main
Are you sure you want to change the base?
Conversation
|
Looks like there's basically two categories of remaining testing errors on Windows:
|
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request focuses on improving cross-platform compatibility, particularly for Windows, which is a valuable effort. The changes largely involve replacing shell-specific commands with PHP-native implementations and using DIRECTORY_SEPARATOR for path construction. These are solid improvements for portability. I've identified a few issues, including a critical bug in path concatenation, a high-severity regression that affects file permissions on non-Windows systems, and several medium-severity items related to robust error handling and consistent path construction. Addressing these will significantly strengthen the cross-platform support.
| $phar_path = $bin_dir . '/wp'; | ||
| $bin = Utils\is_windows() ? 'wp.bat' : 'wp'; | ||
| if ( false !== $bin_dir && file_exists( $bin_dir . DIRECTORY_SEPARATOR . $bin ) && (string) file_get_contents( $bin_dir . DIRECTORY_SEPARATOR . $bin, false, null, 0, $phar_begin_len ) === $phar_begin ) { | ||
| $phar_path = $bin_dir . $bin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'TEST_RUN_DIR' => self::$behat_run_dir, | ||
| ]; | ||
|
|
||
| $env = array_merge( $env, $_ENV ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of array_merge() appears to be incorrect. By using array_merge( $env, $_ENV ), any environment variables from $_ENV with the same key (like PATH) will overwrite the ones you've carefully constructed in $env. This could lead to an incorrect environment for the sub-process. To ensure your specific settings take precedence unless overridden by $_ENV, you should reverse the order of the arguments.
$env = array_merge( $_ENV, $env );| $response = Utils\http_request( 'GET', $download_url, null, [], [ 'filename' => $this->variables['PHAR_PATH'] ] ); | ||
|
|
||
| if ( 200 !== $response->status_code ) { | ||
| throw new RuntimeException( "Could not download WP-CLI PHAR (HTTP code {$response->status_code})" ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation using curl also made the downloaded phar executable with chmod +x. This functionality was removed in the refactoring, which is a regression for non-Windows systems where the phar needs to be executable to be run directly. Please add logic to set the executable permission on non-Windows platforms.
$response = Utils\http_request( 'GET', $download_url, null, [], [ 'filename' => $this->variables['PHAR_PATH'] ] );
if ( 200 !== $response->status_code ) {
throw new RuntimeException( "Could not download WP-CLI PHAR (HTTP code {$response->status_code})" );
}
if ( ! Utils\is_windows() ) {
chmod( $this->variables['PHAR_PATH'], 0755 );
}| } | ||
|
|
||
| $composer = json_decode( file_get_contents( $project_composer ) ); | ||
| $composer = json_decode( (string) file_get_contents( $project_composer ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting the result of file_get_contents() to a string with (string) can hide potential file read errors. If file_get_contents() fails, it returns false, which is then cast to an empty string. json_decode('') on this empty string results in null, allowing the script to proceed without a clear indication of failure. It would be more robust to explicitly check the return value of file_get_contents().
$composer_contents = file_get_contents( $project_composer );
if ( false === $composer_contents ) {
return;
}
$composer = json_decode( $composer_contents );| if ( false !== $bin_dir && file_exists( $bin_dir . '/wp' ) && file_get_contents( $bin_dir . '/wp', false, null, 0, $phar_begin_len ) === $phar_begin ) { | ||
| $phar_path = $bin_dir . '/wp'; | ||
| $bin = Utils\is_windows() ? 'wp.bat' : 'wp'; | ||
| if ( false !== $bin_dir && file_exists( $bin_dir . DIRECTORY_SEPARATOR . $bin ) && (string) file_get_contents( $bin_dir . DIRECTORY_SEPARATOR . $bin, false, null, 0, $phar_begin_len ) === $phar_begin ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting the result of file_get_contents() to (string) can mask file read errors. If file_get_contents() fails, it returns false, which becomes an empty string. This could lead to the if condition failing silently for the wrong reason. It's safer to check the return value of file_get_contents() explicitly before using it.
| ); | ||
|
|
||
| foreach ( $iterator as $item ) { | ||
| $dest_path = $dest_dir . '/' . $iterator->getSubPathname(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| echo "WordPress {$result->stdout}\n"; | ||
| } | ||
|
|
||
| $dest_dir = $this->variables['RUN_DIR'] . "/$subdir"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ( $file->isDir() ) { | ||
| rmdir( $file->getPathname() ); | ||
| } else { | ||
| unlink( $file->getPathname() ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the implementation in FeatureContext.php and for better robustness (e.g., to correctly handle symlinks), it's recommended to use getRealPath() instead of getPathname() when removing files and directories.
if ( $file->isDir() ) {
rmdir( $file->getRealPath() );
} else {
unlink( $file->getRealPath() );
}
I'm trying to improve Windows support a little bit so that tests could run on CI. Doing things in PHP vs. CLI commands is one step.
Lots of AI code changes in here, might need some cleanup
Main change is blocked by wp-cli/wp-cli#6124To-do:
See #155