Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Preview

Copilot AI Jul 4, 2025

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.”

Suggested change
// 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.

// 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) {
Copy link
Preview

Copilot AI Jul 4, 2025

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.

Suggested change
if v.Version == versionPrefix && v.ForceFlavorIfSupported(flavor) {
if v.Version == versionPrefix && v.SupportsFlavor(flavor) {
v.ForceFlavorIfSupported(flavor)

Copilot uses AI. Check for mistakes.

Copy link
Member Author

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.

Copy link
Contributor

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.

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) {
Copy link
Preview

Copilot AI Jul 4, 2025

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.

Suggested change
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.

return v, source, warning, nil
}
}
Expand Down
45 changes: 36 additions & 9 deletions store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,42 @@ package phpstore

import (
"path/filepath"
"sort"
"testing"
)

func TestBestVersion(t *testing.T) {
store := New("/dev/null", false, nil)
store := newEmpty("/dev/null", nil)
for _, v := range []string{"7.4.33", "8.0.27", "8.1.2", "8.1.14", "8.2.1"} {
store.addVersion(&Version{
Version: v,
PHPPath: filepath.Join("/foo", v, "bin", "php"),
})
ver := NewVersion(v)
ver.PHPPath = filepath.Join("/foo", v, "bin", "php")
store.addVersion(ver)

if !store.IsVersionAvailable(v) {
t.Errorf("Version %s should be shown as available", v)
}
}

{
v := "8.0.26"
ver := NewVersion(v)
ver.PHPPath = filepath.Join("/foo", v, "bin", "php")
ver.FPMPath = filepath.Join("/foo", v, "bin", "php-fpm")
store.addVersion(ver)

if !store.IsVersionAvailable(v) {
t.Errorf("Version %s should be shown as available", v)
}
}

sort.Sort(store.versions)

{
bestVersion, _, _, _ := store.bestVersion("8", "testing")
if bestVersion == nil {
t.Error("8 requirement should find a best version")
} else if bestVersion.Version != "8.2.1" {
t.Error("8 requirement should find 8.2.1 as best version")
t.Errorf("8 requirement should find 8.2.1 as best version, got %s", bestVersion.Version)
}
}

Expand All @@ -32,7 +46,7 @@ func TestBestVersion(t *testing.T) {
if bestVersion == nil {
t.Error("8.1 requirement should find a best version")
} else if bestVersion.Version != "8.1.14" {
t.Error("8.1 requirement should find 8.1.14 as best version")
t.Errorf("8.1 requirement should find 8.1.14 as best version, got %s", bestVersion.Version)
}
}

Expand All @@ -41,7 +55,7 @@ func TestBestVersion(t *testing.T) {
if bestVersion == nil {
t.Error("8.0.10 requirement should find a best version")
} else if bestVersion.Version != "8.0.27" {
t.Error("8.0.10 requirement should find 8.0.27 as best version")
t.Errorf("8.0.10 requirement should find 8.0.27 as best version, got %s", bestVersion.Version)
} else if warning == "" {
t.Error("8.0.10 requirement should trigger a warning")
}
Expand All @@ -52,9 +66,22 @@ func TestBestVersion(t *testing.T) {
if bestVersion == nil {
t.Error("8.0.99 requirement should find a best version")
} else if bestVersion.Version != "8.0.27" {
t.Error("8.0.99 requirement should find 8.0.27 as best version")
t.Errorf("8.0.99 requirement should find 8.0.27 as best version, got %s", bestVersion.Version)
} else if warning != "" {
t.Error("8.0.99 requirement should not trigger a warning")
}
}

{
bestVersion, _, warning, _ := store.bestVersion("8.0-fpm", "testing")
if bestVersion == nil {
t.Error("8.0-fpm requirement should find a best version")
} else if bestVersion.Version != "8.0.26" {
t.Errorf("8.0-fpm requirement should find 8.0.26 as best version, got %s", bestVersion.Version)
} else if bestVersion.serverType() != fpmServer {
t.Error("8.0-fpm requirement should find an FPM expectedFlavors")
} else if warning != "" {
t.Error("8.0-fpm requirement should not trigger a warning")
}
}
}
79 changes: 77 additions & 2 deletions version.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:"-"`
Expand All @@ -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
Expand Down Expand Up @@ -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
Copy link
Preview

Copilot AI Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently serverType() checks v.FrankenPHP before typeOverride, so forced flavors are ignored when FrankenPHP is true. Consider moving the typeOverride check above the v.FrankenPHP branch to respect overrides.

Suggested change
if v.FrankenPHP {
return frankenphpServer
}
if v.typeOverride != noServerType {
return v.typeOverride
}
if v.typeOverride != noServerType {
return v.typeOverride
}
if v.FrankenPHP {
return frankenphpServer
}

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Added a comment in this sense

if v.FPMPath != "" {
return fpmServer
}
Expand All @@ -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)
Expand Down
89 changes: 89 additions & 0 deletions version_test.go
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)
}
}
}
}