[Process] Add Laravel Herd php detection path #58258
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
I would treat that as a new feature for 7.2 though, not as a bugfix (and if treated as a bugfix, it would not have 7.1 as its oldest impacted version) |
|
Right, this is my first contribution to Symfony, so sorry about any formal issues. |
|
This looks like a new feature to me and should thus target the |
|
Can we add a test that covers the change? |
|
@derrabus Well, I don't know a good way to test this - and as I said, there are currently no tests for the xampp detection either, otherwise I would've used that as a starting point. This is what the current coverage looks like: 
(This coverage reported was generated by only running the Process component tests using |
|
Can we try installing Laravel Herd in the integration test job? |
|
Given Herd install PHP globally in the system (as far as I understand), an integration tests will be quite costly (a whole new CI job just for one test). I think we should at most add a small unit test to make sure we don't break the behavior of checking the |
|
@wouterj I don't know how this should be tested. As |
|
Aha, indeed. Let's go without tests then 👍 |
3af06f1 to
93ca4e9
Compare
|
Thank you @mpociot. |
As of right now, using the
PhpExecutableFinderin combination with Laravel Herd, results in an empty string, which is what this PR fixes.As the PhpExecutableFinder is already aware of XAMPP, it might be a good idea to also add support for Laravel Herd, which is widely used - not only in the Laravel ecosystem.
Laravel Herd uses statically compiled PHP binaries, which is why the
PHP_BINDIRfolder always points to/bininstead of a user-specific folder.As Laravel Herd adds a
HERD_HOMEenvironment variable, we can check for the existence of this variable and then add the bin subdirectory to the list of directories to search.I couldn't find any tests for XAMPP, which is why I haven't included any for this addition.