Skip to content

Improve compatibility with other operating systems#274

Open
swissspidy wants to merge 119 commits intomainfrom
try/os
Open

Improve compatibility with other operating systems#274
swissspidy wants to merge 119 commits intomainfrom
try/os

Conversation

@swissspidy
Copy link
Member

@swissspidy swissspidy commented Oct 2, 2025

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#6124

To-do:

  • Add Behat tags to skip tests for certain OS?

See #155

@swissspidy
Copy link
Member Author

Looks like there's basically two categories of remaining testing errors on Windows:

  • When doing things like echo "value1", it will actually add the quotes to stdout.
  • Line ending issues in Test CSV containing with headers scenario

@swissspidy swissspidy modified the milestones: 5.0.4, 5.0.5 Dec 9, 2025
@swissspidy swissspidy modified the milestones: 5.0.6, 5.0.7 Dec 17, 2025
@swissspidy swissspidy modified the milestones: 5.0.7, 5.1.0 Jan 13, 2026
@swissspidy

This comment was marked as resolved.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There's a missing DIRECTORY_SEPARATOR when constructing the $phar_path. This will lead to an incorrect path (e.g., /path/to/bindirwp.bat on Windows) and will cause failures when trying to use the phar.

                $phar_path = $bin_dir . DIRECTORY_SEPARATOR . $bin;

'TEST_RUN_DIR' => self::$behat_run_dir,
];

$env = array_merge( $env, $_ENV );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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 );

Comment on lines +1180 to +1184
$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})" );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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 ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For better cross-platform compatibility, DIRECTORY_SEPARATOR should be used here instead of a hardcoded forward slash (/) when constructing the destination path.

            $dest_path = $dest_dir . DIRECTORY_SEPARATOR . $iterator->getSubPathname();

@@ -1375,13 +1454,13 @@ public function download_wp( $subdir = '', $version = '' ): void {
$dest_dir = $this->variables['RUN_DIR'] . "/$subdir";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure cross-platform compatibility, it's best to use the DIRECTORY_SEPARATOR constant instead of a hardcoded forward slash (/) for constructing paths.

        $dest_dir = $this->variables['RUN_DIR'] . DIRECTORY_SEPARATOR . $subdir;

Comment on lines +49 to +53
if ( $file->isDir() ) {
rmdir( $file->getPathname() );
} else {
unlink( $file->getPathname() );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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() );
            }

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 12 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

public function then_a_specific_file_folder_should_exist( $path, $type, $strictly, $action, $expected = null ): void {
$path = $this->replace_variables( $path );

$is_absolute = preg_match( '#^[a-zA-Z]:\\\\#', $path ) || ( '/' === $path[0] || '\\' === $path[0] );
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The absolute path detection logic has a potential bug. When checking if a path starts with '/' or '', it will throw a warning if the path is an empty string or doesn't have a character at index 0. The check should verify the string length before accessing the first character, or use a function like str_starts_with() in PHP 8+.

Consider using: $is_absolute = preg_match( '#^[a-zA-Z]:\\\\#', $path ) || ( strlen( $path ) > 0 && ( '/' === $path[0] || '\\' === $path[0] ) );

Suggested change
$is_absolute = preg_match( '#^[a-zA-Z]:\\\\#', $path ) || ( '/' === $path[0] || '\\' === $path[0] );
$is_absolute = preg_match( '#^[a-zA-Z]:\\\\#', $path ) || ( strlen( $path ) > 0 && ( '/' === $path[0] || '\\' === $path[0] ) );

Copilot uses AI. Check for mistakes.
Comment on lines +1392 to +1394
rmdir( $file->getRealPath() );
} else {
unlink( $file->getRealPath() );
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of getRealPath() can fail for symbolic links or certain file system configurations, returning false instead of a path. This could cause rmdir() or unlink() to fail silently or with a warning. Consider using getPathname() instead, which is more reliable across platforms, or add error checking for when getRealPath() returns false.

Suggested change
rmdir( $file->getRealPath() );
} else {
unlink( $file->getRealPath() );
rmdir( $file->getPathname() );
} else {
unlink( $file->getPathname() );

Copilot uses AI. Check for mistakes.
$this->variables['COMPOSER_PATH'] = exec( 'which composer' );
$command = Utils\is_windows() ? 'where composer' : 'which composer';
$path = exec( $command );
if ( false === $path ) {
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error checking for exec() is incomplete. While checking for false catches the case when exec() completely fails, it doesn't handle the case where exec() returns an empty string (when the command succeeds but produces no output, or when composer is not found). Consider also checking if the returned path is empty: if ( false === $path || empty( $path ) ).

Suggested change
if ( false === $path ) {
if ( false === $path || empty( $path ) ) {

Copilot uses AI. Check for mistakes.
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 ) {
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is checking if wp.bat starts with #!/usr/bin/env php, which is incorrect for Windows batch files. A .bat file will never start with a shebang. This logic should only check for phar when not on Windows, or the check should be updated to handle the Windows case differently.

Suggested change
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 ) {
if ( false !== $bin_dir && ! Utils\is_windows() && file_exists( $bin_dir . DIRECTORY_SEPARATOR . $bin ) && (string) file_get_contents( $bin_dir . DIRECTORY_SEPARATOR . $bin, false, null, 0, $phar_begin_len ) === $phar_begin ) {

Copilot uses AI. Check for mistakes.
$db_copy_contents = file_get_contents( $db_copy );

if ( false === $db_copy_contents ) {
return;
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silently returning on file read failure could hide issues. If the db.copy file doesn't exist or can't be read, the SQLite configuration will be incomplete, potentially causing cryptic errors later. Consider throwing an exception or logging a warning instead of silently returning: throw new RuntimeException( "Could not read db.copy file at: {$db_copy}" );

Suggested change
return;
throw new RuntimeException( "Could not read db.copy file at: {$db_copy}" );

Copilot uses AI. Check for mistakes.
$mysqldump_binary = Utils\force_env_on_nix_systems( $mysqldump_binary );
$support_column_statistics = exec( "{$mysqldump_binary} --help | grep 'column-statistics'" );
$help_output = shell_exec( "{$mysqldump_binary} --help" );
$support_column_statistics = false !== strpos( $help_output, 'column-statistics' );
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of shell_exec() can return null on failure, not just an empty string. The code should check for null before using strpos(). Consider: $help_output = shell_exec( "{$mysqldump_binary} --help" ); $support_column_statistics = ( null !== $help_output && false !== strpos( $help_output, 'column-statistics' ) );

Suggested change
$support_column_statistics = false !== strpos( $help_output, 'column-statistics' );
$support_column_statistics = ( null !== $help_output && false !== strpos( $help_output, 'column-statistics' ) );

Copilot uses AI. Check for mistakes.
Comment on lines +781 to +782
if ( Utils\is_windows() ) {
shell_exec( "taskkill /F /T /PID $master_pid > NUL 2>&1" );
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Windows taskkill command does not escape the PID value, which could potentially be exploited if the PID value comes from untrusted sources. While PIDs are typically integers from proc_get_status, it's safer to validate or escape the value. Consider adding validation: $master_pid = (int) $master_pid; shell_exec( "taskkill /F /T /PID {$master_pid} > NUL 2>&1" );

Suggested change
if ( Utils\is_windows() ) {
shell_exec( "taskkill /F /T /PID $master_pid > NUL 2>&1" );
$master_pid = (int) $master_pid;
if ( Utils\is_windows() ) {
shell_exec( "taskkill /F /T /PID {$master_pid} > NUL 2>&1" );

Copilot uses AI. Check for mistakes.
'TEST_RUN_DIR' => self::$behat_run_dir,
];

$env = array_merge( $env, $_ENV );
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging $_ENV after setting explicit environment variables can overwrite intentionally set values like PATH, HOME, or COMPOSER_HOME. If any of these variables exist in $_ENV, they would replace the carefully constructed values. Consider either merging $_ENV before setting the specific variables, or selectively merging only the needed variables from $_ENV.

Suggested change
$env = array_merge( $env, $_ENV );
$env = array_merge( $_ENV, $env );

Copilot uses AI. Check for mistakes.
$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;
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an inconsistency in path construction. Line 995 concatenates $bin_dir . $bin without a directory separator, while lines 998-999 use forward slashes. This could cause the path to be incorrect. It should be $bin_dir . DIRECTORY_SEPARATOR . $bin to match the check on line 994.

Suggested change
$phar_path = $bin_dir . $bin;
$phar_path = $bin_dir . DIRECTORY_SEPARATOR . $bin;

Copilot uses AI. Check for mistakes.

This comment was marked as resolved.

Copilot AI and others added 4 commits February 5, 2026 19:23
* Initial plan

* Add cleanup for temporary files on Windows in background_proc

Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>

* Use error suppression for unlink calls to handle edge cases

Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>

* Use explicit file_exists checks before cleanup

Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>

* Track and cleanup temporary files for all background processes

Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>

* Refactor: Extract cleanup logic into helper method

Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants