Skip to content
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

[11.x] Fix PHP and Artisan binary #52744

Merged
merged 11 commits into from
Sep 12, 2024

Conversation

hafezdivandari
Copy link
Contributor

This PR fixes usages of PHP and Artisan binary on different places of the framework.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@hafezdivandari hafezdivandari marked this pull request as ready for review September 11, 2024 21:25
@hafezdivandari hafezdivandari marked this pull request as draft September 11, 2024 21:39
@hafezdivandari hafezdivandari marked this pull request as ready for review September 11, 2024 21:45
@hafezdivandari hafezdivandari marked this pull request as draft September 11, 2024 21:50
@hafezdivandari hafezdivandari deleted the 11.x-fix-php-artisan-binary branch September 11, 2024 22:20
@devajmeireles
Copy link
Contributor

@hafezdivandari Are you going ahead with this? This fixes issues caused by the binary path via Herd: /Users/<user>/Library/Application Support/Herd/bin//php. The whitespace in Application Support causes an exception. I'm just wondering if Application can be used here since Concurrency doesn't depend on it in composer.json?

@hafezdivandari hafezdivandari restored the 11.x-fix-php-artisan-binary branch September 11, 2024 22:36
@hafezdivandari
Copy link
Contributor Author

@devajmeireles yeah I'm gonna fix this if I can control my OCD!

@hafezdivandari hafezdivandari marked this pull request as ready for review September 11, 2024 23:10
@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Sep 11, 2024

I'm just wondering if Application can be used here since Concurrency doesn't depend on it in composer.json

It uses Console\Application and I think we already require illuminate/console, because Cocurrency works by calling this command that depends on illuminate/console?

@devajmeireles
Copy link
Contributor

I'm just wondering if Application can be used here since Concurrency doesn't depend on it in composer.json

It uses Console\Application and I think we already require illuminate/console, because Cocurrency works by calling this command that depends on illuminate/console?

This is outdated. Previously you had entered Illuminate\Foundation\Application, but now it looks correct using Illuminate\Console\Application.

@crynobone
Copy link
Member

crynobone commented Sep 12, 2024

symfony/symfony#52409

There are some internal differences between passing a string and an array to Symfony Process where using an array can be 2x to 4x faster.

CleanShot 2024-09-12 at 16 34 32

@crynobone crynobone linked an issue Sep 12, 2024 that may be closed by this pull request
@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Sep 12, 2024

@devajmeireles you're right, that was a typo.

@crynobone good to know, I changed it to pass as an array.

PS: we could use Console\Application::phpBinary() and artisanBinary() everywhere on the framework if they weren't unnecessarily escaped by default!

@taylorotwell taylorotwell merged commit 6a547e4 into laravel:11.x Sep 12, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array offset error when using the Concurrency facade
4 participants