fix(php-fpm): correct socket creation #1515
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While my PR laravel#1514 fixed recreating the FPM configs, it introduced a different issue: Due to the use of `utilizedPhpVersions()`, the code now also configured the FPM config for the alias `php` version. This caused it to invoke the configure function with an empty version string and thus overwriting the FPM config for (in my case) the [email protected] config templated with `valet.sock` instead of the correct `valet84.sock`. The nginx sites that were configured to proxy their requests to the `valet84.sock` then failed because it did not exist anymore. We fixed this by always including the actual linked PHP version via the `linkedPhp` function. This returns `php8.4` instead of `php`. `php` is an alias anyway and this also removes another unnecessary service restart call. Previously, this would also try to restart the `php` service via brew which was already restarted through the restart of `[email protected]`, which is an alias in brew. This also fixes an issue with the previous PR, to correctly symlink `valet.sock` again to the linked PHP version, which we oversaw.
saibotk
force-pushed
the
fix-php-fpm-config
branch
from
fc1b067 to
0525673
Compare
|
👍 /cc @mattstauffer |
|
Brilliant, thanks so much for catching this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While my PR #1514 fixed recreating the FPM configs, it introduced a different issue:
Due to the use of
utilizedPhpVersions(), the code now also configured the FPM config for the aliasphpversion.So e.g. on my system it returned these versions:
This caused it to invoke the configure function 4 times, once for each version.
For
phpit was invoked with an empty version string argument and thus caused overwriting the FPM config for (in my case) [email protected], templated withvalet.sock, instead of the correctvalet84.sock.The nginx sites that were configured to proxy their requests to the
valet84.sockthen failed because it did not exist anymore.We fixed this by always including the actual linked PHP version via the
linkedPhpfunction. This returnsphp8.4instead ofphp, so we only get the 3 actual PHP versions.phpis an alias anyway and this also removes another unnecessary service restart call.Previously, this would also try to restart the
phpservice via brew which was already restarted through the restart of[email protected], which is an alias in brew.This also fixes an issue with the previous PR, to correctly symlink
valet.sockagain to the linked PHP version, which we oversaw. We also fixed a small comment typo.