-
-
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -44,18 +44,24 @@ type PHPStore struct { | |||||||
|
||||||||
// New creates a new PHP store | ||||||||
func New(configDir string, reload bool, logger func(msg string, a ...interface{})) *PHPStore { | ||||||||
s := &PHPStore{ | ||||||||
configDir: configDir, | ||||||||
seen: make(map[string]int), | ||||||||
discoveryLogFunc: logger, | ||||||||
} | ||||||||
s := newEmpty(configDir, logger) | ||||||||
|
||||||||
if reload { | ||||||||
_ = os.Remove(filepath.Join(configDir, "php_versions.json")) | ||||||||
} | ||||||||
s.loadVersions() | ||||||||
return s | ||||||||
} | ||||||||
|
||||||||
// newEmpty creates a new "empty" (without loading versions) PHP store | ||||||||
func newEmpty(configDir string, logger func(msg string, a ...interface{})) *PHPStore { | ||||||||
return &PHPStore{ | ||||||||
configDir: configDir, | ||||||||
seen: make(map[string]int), | ||||||||
discoveryLogFunc: logger, | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
// Versions returns all available PHP versions | ||||||||
func (s *PHPStore) Versions() []*Version { | ||||||||
return s.versions | ||||||||
|
@@ -136,47 +142,49 @@ func (s *PHPStore) BestVersionForDir(dir string) (*Version, string, string, erro | |||||||
return s.fallbackVersion("") | ||||||||
} | ||||||||
|
||||||||
// bestVersion returns the latest patch version for the given major (X), minor (X.Y), or patch (X.Y.Z) | ||||||||
// version can be 7 or 7.1 or 7.1.2 | ||||||||
// non-symlinked versions have priority | ||||||||
// bestVersion returns the latest patch version for the given major (X), | ||||||||
// minor (X.Y), or patch (X.Y.Z). | ||||||||
// Version can be 7 or 7.1 or 7.1.2 and optionally suffixed with a flavor. | ||||||||
// Non-symlinked versions have priority. | ||||||||
// If the asked version contains a flavor (e.g. "7.4-fpm"), it will only accept | ||||||||
// versions supporting this flavor. | ||||||||
// If the asked version is a patch one (X.Y.Z) and is not available, the lookup | ||||||||
// will fallback to the last path version for the minor version (X.Y). | ||||||||
// will fallback to the last patch version for the minor version (X.Y). | ||||||||
// There's no fallback to the major version because PHP is known to occasionally | ||||||||
// break BC in minor versions, so we can't safely fall back. | ||||||||
func (s *PHPStore) bestVersion(versionPrefix, source string) (*Version, string, string, error) { | ||||||||
warning := "" | ||||||||
flavor := "" | ||||||||
|
||||||||
isPatchVersion := false | ||||||||
pos := strings.LastIndexByte(versionPrefix, '.') | ||||||||
if pos != strings.IndexByte(versionPrefix, '.') { | ||||||||
if versionPrefix[pos+1:] == "99" { | ||||||||
versionPrefix = versionPrefix[:pos] | ||||||||
pos = strings.LastIndexByte(versionPrefix, '.') | ||||||||
} else { | ||||||||
isPatchVersion = true | ||||||||
} | ||||||||
// Check if versionPrefix has a expectedFlavors constraint, if so first do an | ||||||||
// exact match lookup and fallback to a minor version check | ||||||||
if pos := strings.LastIndexByte(versionPrefix, '-'); pos != -1 { | ||||||||
flavor = versionPrefix[pos+1:] | ||||||||
versionPrefix = versionPrefix[:pos] | ||||||||
} | ||||||||
|
||||||||
// Check if versionPrefix is actually a patch version, if so first do an | ||||||||
// exact match lookup and fallback to a minor version check | ||||||||
if isPatchVersion { | ||||||||
if pos := strings.LastIndexByte(versionPrefix, '.'); pos != strings.IndexByte(versionPrefix, '.') { | ||||||||
// 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. Choose a reason for hiding this commentThe 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the overhead is significant. |
||||||||
return v, source, "", nil | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
// exact match not found, fallback to minor version check | ||||||||
newVersionPrefix := versionPrefix[:pos] | ||||||||
warning = fmt.Sprintf(`the current dir requires PHP %s (%s), but this version is not available: fallback to %s`, versionPrefix, source, newVersionPrefix) | ||||||||
if versionPrefix[pos+1:] != "99" { | ||||||||
warning = fmt.Sprintf(`the current dir requires PHP %s (%s), but this version is not available: fallback to %s`, versionPrefix, source, newVersionPrefix) | ||||||||
} | ||||||||
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. Choose a reason for hiding this commentThe 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||
return v, source, warning, nil | ||||||||
} | ||||||||
} | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,12 +30,20 @@ import ( | |||||||||||||||||||||||||
type serverType int | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const ( | ||||||||||||||||||||||||||
fpmServer serverType = iota | ||||||||||||||||||||||||||
cgiServer | ||||||||||||||||||||||||||
noServerType serverType = iota | ||||||||||||||||||||||||||
cliServer | ||||||||||||||||||||||||||
cgiServer | ||||||||||||||||||||||||||
fpmServer | ||||||||||||||||||||||||||
frankenphpServer | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const ( | ||||||||||||||||||||||||||
FlavorCLI string = "cli" | ||||||||||||||||||||||||||
FlavorCGI string = "cgi" | ||||||||||||||||||||||||||
FlavorFPM string = "fpm" | ||||||||||||||||||||||||||
FlavorFrankenPHP string = "frankenphp" | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Version stores information about an installed PHP version | ||||||||||||||||||||||||||
type Version struct { | ||||||||||||||||||||||||||
FullVersion *version.Version `json:"-"` | ||||||||||||||||||||||||||
|
@@ -49,6 +57,15 @@ type Version struct { | |||||||||||||||||||||||||
PHPdbgPath string `json:"phpdbg_path"` | ||||||||||||||||||||||||||
IsSystem bool `json:"is_system"` | ||||||||||||||||||||||||||
FrankenPHP bool `json:"frankenphp"` | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
typeOverride serverType | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func NewVersion(v string) *Version { | ||||||||||||||||||||||||||
return &Version{ | ||||||||||||||||||||||||||
Version: v, | ||||||||||||||||||||||||||
FullVersion: version.Must(version.NewVersion(v)), | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
type versions []*Version | ||||||||||||||||||||||||||
|
@@ -106,9 +123,14 @@ func (v *Version) IsFrankenPHPServer() bool { | |||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func (v *Version) serverType() serverType { | ||||||||||||||||||||||||||
// FrankenPHP is a special case as it will not support several server types | ||||||||||||||||||||||||||
// for a single installation. | ||||||||||||||||||||||||||
if v.FrankenPHP { | ||||||||||||||||||||||||||
return frankenphpServer | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if v.typeOverride != noServerType { | ||||||||||||||||||||||||||
return v.typeOverride | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Comment on lines
128
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FrankenPHP installations don't support multiple flavors (because they are standalone and won't have multiple SAPI binaries). As such, we should not support an override for them. |
||||||||||||||||||||||||||
if v.FPMPath != "" { | ||||||||||||||||||||||||||
return fpmServer | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
@@ -119,6 +141,59 @@ func (v *Version) serverType() serverType { | |||||||||||||||||||||||||
return cliServer | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func (v *Version) ForceFlavorIfSupported(flavor string) bool { | ||||||||||||||||||||||||||
if flavor == "" { | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if !v.SupportsFlavor(flavor) { | ||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
switch flavor { | ||||||||||||||||||||||||||
case FlavorCLI: | ||||||||||||||||||||||||||
v.typeOverride = cliServer | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
case FlavorCGI: | ||||||||||||||||||||||||||
v.typeOverride = cgiServer | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
case FlavorFPM: | ||||||||||||||||||||||||||
v.typeOverride = fpmServer | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
case FlavorFrankenPHP: | ||||||||||||||||||||||||||
// FrankenPHP installations does not support multiple flavors so there's | ||||||||||||||||||||||||||
// no need to set the typeOverride. | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func (v *Version) SupportsFlavor(flavor string) bool { | ||||||||||||||||||||||||||
if flavor == "" { | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
serverFlavor := v.serverType() | ||||||||||||||||||||||||||
if serverFlavor == frankenphpServer { | ||||||||||||||||||||||||||
return flavor == FlavorFrankenPHP | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// CLI flavor is always supported | ||||||||||||||||||||||||||
if flavor == FlavorCLI { | ||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
switch serverFlavor { | ||||||||||||||||||||||||||
case cgiServer: | ||||||||||||||||||||||||||
return flavor == FlavorCGI | ||||||||||||||||||||||||||
case fpmServer: | ||||||||||||||||||||||||||
return flavor == FlavorFPM | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func (v *Version) setServer(fpm, cgi, phpconfig, phpize, phpdbg string) string { | ||||||||||||||||||||||||||
msg := fmt.Sprintf(" Found PHP: %s", v.PHPPath) | ||||||||||||||||||||||||||
fpm = filepath.Clean(fpm) | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* Copyright (c) 2021-present Fabien Potencier <fabien@symfony.com> | ||
* | ||
* This file is part of Symfony CLI project | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License as | ||
* published by the Free Software Foundation, either version 3 of the | ||
* License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
package phpstore | ||
|
||
import ( | ||
"testing" | ||
) | ||
|
||
func TestVersion_SupportsFlavor(t *testing.T) { | ||
testCases := []struct { | ||
version *Version | ||
expectedFlavors []string | ||
}{ | ||
{ | ||
version: func() *Version { | ||
v := NewVersion("8.1") | ||
v.FPMPath = "/usr/bin/php-fpm8.1" | ||
v.PHPPath = "/usr/bin/php-8.1" | ||
return v | ||
}(), | ||
expectedFlavors: []string{FlavorFPM, FlavorCLI}, | ||
}, | ||
{ | ||
version: func() *Version { | ||
v := NewVersion("8.2") | ||
v.CGIPath = "/usr/bin/php-cgi8.1" | ||
v.PHPPath = "/usr/bin/php-8.1" | ||
return v | ||
}(), | ||
expectedFlavors: []string{FlavorCGI, FlavorCLI}, | ||
}, | ||
{ | ||
version: func() *Version { | ||
v := NewVersion("8.3") | ||
v.PHPPath = "/usr/bin/php-8.3" | ||
return v | ||
}(), | ||
expectedFlavors: []string{FlavorCLI}, | ||
}, | ||
{ | ||
version: func() *Version { | ||
v := NewVersion("8.4") | ||
v.PHPPath = "/usr/bin/frankenphp" | ||
v.FrankenPHP = true | ||
return v | ||
}(), | ||
expectedFlavors: []string{FlavorFrankenPHP}, | ||
}, | ||
} | ||
for _, testCase := range testCases { | ||
if !testCase.version.SupportsFlavor("") { | ||
t.Error("version.SupportsFlavor('') should return true, got false") | ||
} | ||
for _, flavor := range testCase.expectedFlavors { | ||
if !testCase.version.SupportsFlavor(flavor) { | ||
t.Errorf("version.SupportsFlavor(%v) should return true, got false", flavor) | ||
} | ||
} | ||
flavorLoop: | ||
for _, possibleFlavor := range []string{FlavorCLI, FlavorCGI, FlavorFPM, FlavorFrankenPHP} { | ||
for _, flavor := range testCase.expectedFlavors { | ||
if flavor == possibleFlavor { | ||
continue flavorLoop | ||
} | ||
} | ||
|
||
if testCase.version.SupportsFlavor(possibleFlavor) { | ||
t.Errorf("version.SupportsFlavor(%v) should return false, got true", possibleFlavor) | ||
} | ||
} | ||
} | ||
} |
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.
[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.”
Copilot uses AI. Check for mistakes.