feat: add support for flavor selection #27
Conversation
tucksaun
force-pushed
the
feat/variant-support
branch
2 times, most recently
from
2147c05 to
ec516ca
Compare
tucksaun
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
tucksaun
force-pushed
the
feat/variant-support
branch
from
3df8d38 to
9c3a414
Compare
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)
tucksaun
force-pushed
the
feat/variant-support
branch
from
9c3a414 to
2d61ff5
Compare
There was a problem hiding this comment.
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 {
| // 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) { |
There was a problem hiding this comment.
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) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@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.
I don't think the overhead is significant.
| versionPrefix = newVersionPrefix | ||
| } | ||
|
|
||
| // start from the end as versions are always sorted | ||
| 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) { |
There was a problem hiding this comment.
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 |
Copilot uses AI. Check for mistakes.
| } else { | ||
| isPatchVersion = true | ||
| } | ||
| // Check if versionPrefix has a expectedFlavors constraint, if so first do an |
There was a problem hiding this comment.
[nitpick] Minor typo/grammar: “a expectedFlavors constraint” is awkward and refers to a non-existent identifier. Consider rephrasing to “an expected-flavor constraint” or simply “a flavor constraint.”
| // Check if versionPrefix has a expectedFlavors constraint, if so first do an | |
| // Check if versionPrefix has an expected-flavor constraint, if so first do an |
Copilot uses AI. Check for mistakes.
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)