fix(php-fpm): correctly reconfigure all versions #1514
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
This previously only reapplied the configuration for the linked version. Because it is recommended to run valet install again after a brew upgrade, it is necessary to update all utilized PHP versions. On brew upgrades, some versions received the default www.conf again. This would leave the user with a broken setup until they either call `valet use php@X.X` for all the versions or isolate a site again with the version. This commit fixes the behavior by rerunning the configuration step on all php versions. This is more consistent, because in the next line all utilized PHP versions are also restarted.
Contributor
|
Agreed. 👍 /cc @mattstauffer |
Collaborator
|
OOO great find--thanks so much! |
saibotk
added a commit
to saibotk/valet
that referenced
this pull request
Apr 25, 2025
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 php@8.4 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 `php@8.4`, 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
added a commit
to saibotk/valet
that referenced
this pull request
Apr 25, 2025
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 php@8.4 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 `php@8.4`, 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This previously only reapplied the configuration for the linked version. Because it is recommended to run valet install again after a brew upgrade, it is necessary to update all utilized PHP versions.
On brew upgrades, some versions received the default
www.confagain. This would leave the user with a broken setup until they either callvalet use php@X.Xfor all the versions or isolate a site again with the version.This commit fixes the behavior by rerunning the configuration step on all php versions. This is more consistent, because in the next line all utilized PHP versions are also restarted.
Seems like this also solves issues like #1410 or at least lets them only rerun a single
valet install, which is something users would do in a regular troubleshooting session.