-
Notifications
You must be signed in to change notification settings - Fork 701
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
Multiple/Parallel PHP Version Support for Valet #1192
Conversation
I'll fix the test cases/issues and maybe re-submit this pull request again. |
Fixed StyleCI and updated a few tests. Please feel free to suggest any other changes 👍 |
Wow.. That is really a very useful feature (isolating php version for different projects). I would really love to have it in valet as I use different project with different php version and every-time I need to switch php version while switching between projects which is painful :( |
Thank you so much @NasirNobin you have saved my valuable time |
I was looking for this kind of solution it's not only saving time, saving lots of work too. Very excited to use it on the next release. |
This feature will be really helpful in lots of cases. Often I need to switch php version of my separate projects, but it was really cumbersome before. With this addition, life will be much better for us developers. |
Hey all, we realize a lot of you want this. Please give us some time to review is and use the thumbs up feature if you want this and not comments as it spams our notifications. Thanks. |
Locking this until we've had time to review this. |
I'm reading through the code now--thanks for your patience!--but one note, I think it's unwise for us to merge this until we at least have the "remove isolation" command you suggested in your future todo's.
This is just an idea, but in my mind, what if we can basically say "use the default PHP version for this site"? I can imagine the syntax being something like this: valet use default --site . It's introducing a new syntax, "default", but it means we're keeping it in the same way of basically Any thoughts, from OP or anyone else? |
@mattstauffer thanks for the review and positive feedback, appreciate it!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even remotely done reading this but I have to go for today, so here is my first round of notes :) Great work here so far!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and feedback. I tried to update the PR accordingly.
I'll be available to work on other suggestions as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another round of review. Once these changes are implemented I think I want to do one more high-level view over everything, then I'll test it out some.
Thanks so much for your hard work on this!
Co-authored-by: Matt Stauffer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just re-read it after your latest round of changes, and it looks great with just one small change here! Please @ message me once you've added some tests, or if you need any help adding them.
THanks!
Co-authored-by: Matt Stauffer <[email protected]>
Finishing the tests here. Mostly done. I'll do some self-review before pushing into this PR. |
@mattstauffer I have tried to add tests around the new methods introduced in this PR. Please feel free to suggest changes as needed. Also, if you think there's some certian scenario missing in the tests, please let me know. I'll be available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! Just a few typos here and there.
Co-authored-by: Matt Stauffer <[email protected]>
@mattstauffer Just checking if there's anything else needed from my side on this PR. Got an additional day off from work tomorrow, which I can totally spend on this. |
@NasirNobin Thanks for checking in! Let me read it again, but I think what I want after this point is review from someone who's not me :D let me ask around at Tighten to see if anyone has time and energy. |
Thanks for your PR, We need this future. |
I'll be doing hopefully a final review tomorrow afternoon, and if it goes well, merging and releasing. 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this with a team of wonderful folks from Tighten and they had some ideas for refactors; don't worry, I'll take these on, but just note these potential refactors need to be addressed before release.
- @fuelingtheweb
- @bakerkretzmar
- @rissajackson
- @marcusmoore
- @annayoungyeun
- @krievley
return $this->fpmSockName($this->normalizePhpVersion($version)); | ||
})->unique(); | ||
|
||
return collect($this->files->scandir(VALET_HOME_PATH.'/Nginx')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential future refactor here to:
- Make it clearer that this
map
and then the laterfilter
is really rejecting any without a custom valet sock file - Stop looking for future instances of any given sock file (e.g. valet74.sock) once you find it in one site config
Thank you so much for your hard work on this, @NasirNobin! I'm going to tag your work (and mine combined in) as a 3.0.0 beta shortly. |
Thanks for the hard work from your side @mattstauffer. This was my very first open-source contribution, learned a lot. Excited to see this feature being used by other Valet users! |
@NasirNobin For your first open source contribution, this was really great code and you were a wonderful collaborator on the changes I requested. Thank you so much, and really fantastic work! |
Update on Valet 3.0 (March 31, 2022)
This PR got merged and released as Valet 3.0 with some changes. The original
valet use [email protected] --site=site.test
syntax was replaced with a new command calledvalet isolate
.Multiple/Parallel PHP Version Support for Valet
This pull request introduces an optional
--site
flag on thevalet use
command, that allows users to run multiple versions of PHP in parallel on different sites.Btw, I build this feature to solve my very own problem. I need to constantly switch between PHP versions, as I need to switch between old and new projects. This is being very handy for me.
To isolate a site's PHP version, run:
or pass the site directory's name
or
cd
into the site's directory then use.
to use current directory:Remove isolation for a site:
valet use default --site .
Here's a video showing this feature up and running
CleanShot.2022-02-04.at.19.06.52.mp4
Behind the scene:
/etc/php/8.0/php-fpm.d/valet-fpm.conf
and updates the listen parameter to something like:valet80.sock
~/.config/valet/Nginx
location for that site.fastcgi_pass
param withvalet80.sock
Few other notes:
valet.sock
as expected.valet use [email protected]
would still work as expected. Alongside, it will perform some operations on the isolated site's Nginx config to make sure those isolated sites still work as expected.valet install
orvalet restart
would do a scan on isolated sites to determine what versions of PHP are being used. So it can restart each version of PHP correctly.valet unsecure
on an isolated site will still keep the isolation and will remove the SSL.Future todos:
Ideally, we would need to have a way to remove isolation for a site (valet use default --site .)This modification doesn't seem to break any existing test cases or any existing flows.
I tried to add tests for the new methods that were introduced in this PR.