-
-
Notifications
You must be signed in to change notification settings - Fork 13
feat: add support for flavor selection #27
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
Conversation
2147c05 to
ec516ca
Compare
3df8d38 to
9c3a414
Compare
9c3a414 to
2d61ff5
Compare
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.
Pull Request Overview
This PR adds support for selecting specific PHP “flavors” (cli, cgi, fpm, frankenphp) when discovering and choosing versions. Key changes include:
- Introduction of
Flavor*constants,SupportsFlavorandForceFlavorIfSupportedmethods inversion.go - Parsing of optional
-flavorsuffix inbestVersionand filtering by flavor instore.go - New tests in
version_test.goand extendedstore_test.goto cover flavor-based selection
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| version.go | Added flavor constants, override field, SupportsFlavor and ForceFlavorIfSupported, updated serverType |
| version_test.go | Added tests for SupportsFlavor covering all flavor cases |
| store.go | Added newEmpty, updated bestVersion to parse flavor suffix and filter versions |
| store_test.go | Switched to newEmpty, added sorting, extended TestBestVersion to validate “-fpm” requirement |
Comments suppressed due to low confidence (3)
version.go:61
- [nitpick] The field name
typeOverrideis not immediately clear; consider renaming tooverrideServerTypeorforcedServerTypefor better readability.
typeOverride serverType
version.go:144
- Public methods should have doc comments. Add a comment explaining what ForceFlavorIfSupported does, its side-effects on the Version, and when it should be used.
func (v *Version) ForceFlavorIfSupported(flavor string) bool {
version.go:144
- There are no tests covering
ForceFlavorIfSupported. Add unit tests to verify that it correctly applies overrides only when the flavor is supported and doesn’t apply when unsupported.
func (v *Version) ForceFlavorIfSupported(flavor string) bool {
store.go
Outdated
| // look for an exact match, the order does not matter here | ||
| for _, v := range s.versions { | ||
| if v.Version == versionPrefix { | ||
| if v.Version == versionPrefix && v.ForceFlavorIfSupported(flavor) { |
Copilot
AI
Jul 4, 2025
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.
Calling ForceFlavorIfSupported here mutates the Version (setting typeOverride) during lookup, which can leak state across calls. Consider using SupportsFlavor for filtering and only applying ForceFlavorIfSupported when actually selecting a version to avoid unintended side-effects.
| if v.Version == versionPrefix && v.ForceFlavorIfSupported(flavor) { | |
| if v.Version == versionPrefix && v.SupportsFlavor(flavor) { | |
| v.ForceFlavorIfSupported(flavor) |
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.
@fabpot, what do you think about this one? My goal was to avoid unnecessary double checks because selecting the best PHP version is somewhat on the hot path for the CLI.
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 don't think the overhead is significant.
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.
updated
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.
@fabpot did you get a chance to review the updated version?
store.go
Outdated
| for i := len(s.versions) - 1; i >= 0; i-- { | ||
| v := s.versions[i] | ||
| if strings.HasPrefix(v.Version, versionPrefix) { | ||
| if strings.HasPrefix(v.Version, versionPrefix) && v.ForceFlavorIfSupported(flavor) { |
Copilot
AI
Jul 4, 2025
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.
Same issue as above: using ForceFlavorIfSupported here changes the Version state during iteration. Swap this for a pure SupportsFlavor check, and defer state changes until after the best match is chosen.
| if strings.HasPrefix(v.Version, versionPrefix) && v.ForceFlavorIfSupported(flavor) { | |
| if strings.HasPrefix(v.Version, versionPrefix) && v.SupportsFlavor(flavor) { | |
| v.ForceFlavorIfSupported(flavor) // Apply state change after match is chosen |
As a maintainer I need on a regular basis to pick specific server type to reproduce issues. But patching source code to do so is: a. cumbersome b. error prone And sometimes this can be useful for users as well (imagine you have a 8.4.5 installed with CLI only and 8.4.4 with FPM and CLI and your project requires FPM for instance). This also lays the ground to pick more flavors (debug or zts for example)
2d61ff5 to
30ac661
Compare
Co-authored-by: Copilot <[email protected]>
As a maintainer, I need on a regular basis to pick specific server type to reproduce issues.
But patching source code to do so is:
a. cumbersome
b. error prone
And sometimes this can be useful for users as well (imagine you have a 8.4.5 installed with CLI only and 8.4.4 with FPM and CLI and your project requires FPM for instance).
This also lays the ground to pick more variants (debug or zts for example)