[11.x] Make tests pass on Herd#54171
Conversation
| public static function artisanBinary() | ||
| { | ||
| return ProcessUtils::escapeArgument(defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan'); | ||
| return ProcessUtils::escapeArgument(artisan_binary()); |
There was a problem hiding this comment.
I don't feel like we need to introduce artisan_binary() helper since calling Illuminate\Console\Application::artisanBinary() would always yield the correct value.
There was a problem hiding this comment.
| return ProcessUtils::escapeArgument(artisan_binary()); | |
| return ProcessUtils::escapeArgument(defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan'); |
There was a problem hiding this comment.
Illuminate\Console\Application::phpBinary() also uses the php_binary() helper, and this way defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan' is now only used inside that helper, nowhere else. I would be fine with accepting your commit suggestion (or you can push it yourself), but it feels like a step bakc
There was a problem hiding this comment.
I feel like this method should return without quote in Laravel 12 and let method that needs it to explicit espace the argument.
Escaping argument is only needed when we providing string instead of array to Symfony Process.
There was a problem hiding this comment.
artisan_binary() also returns without quotes, the change on this line doesn't change the output from defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan'
I agree with you that there's further cleanup possible for v12
| Process::run(array_filter([ | ||
| php_binary(), | ||
| defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan', | ||
| artisan_binary(), |
There was a problem hiding this comment.
| artisan_binary(), | |
| \Illuminate\Console\Application::artisanBinary(), |
| Process::run([ | ||
| php_binary(), | ||
| defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan', | ||
| artisan_binary(), |
There was a problem hiding this comment.
| artisan_binary(), | |
| \Illuminate\Console\Application::artisanBinary(), |
| Process::run([ | ||
| php_binary(), | ||
| defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan', | ||
| artisan_binary(), |
There was a problem hiding this comment.
| artisan_binary(), | |
| \Illuminate\Console\Application::artisanBinary(), |
| protected function artisanBinary() | ||
| { | ||
| return defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan'; | ||
| return artisan_binary(); |
There was a problem hiding this comment.
| return artisan_binary(); | |
| return \Illuminate\Console\Application::artisanBinary(); |
There was a problem hiding this comment.
This is functionally different since \Illuminate\Console\Application::artisanBinary() escapes it using ProcessUtils::escapeArgument()
|
Marking as draft since few tests on Windows are failing. |
|
@crynobone thank you for reviewing, please let me know if you are looking for any additional changes at this point |
3 participants
Fix Test Compatibility with Herd by Standardizing Binary Handling
This PR fixes tests that use
PHP_BINARYand therefore fail when running via Herd.It standardizes the handling of
PHPandArtisanbinaries via the existingphp_binaryfunction (orApplication::phpBinary()for console operations) and the newly addedartisan_binaryfunction.Key Changes:
artisan_binaryhelper inIlluminate\Support\functions.phpto standardize access to the Artisan binary path.artisan_binaryhelper, replacing inline checks likedefined('ARTISAN_BINARY').Benefits:
Tests have been updated and verified to maintain existing behavior while ensuring compatibility with Herd. This change focuses on improving developer experience in environments using Herd.