-
Notifications
You must be signed in to change notification settings - Fork 488
feature: register phpinfo entries #2578
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
henderkes
wants to merge
38
commits into
main
Choose a base branch
from
feat/register_phpinfo_entries
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
922de2d
feat: register phpinfo info entries under frankenphp extension
henderkes ce102e5
report e-dant/watcher, dunglas/caddy-cbrotli, libbrotli and dunglas/m…
henderkes 032a742
keep go array, only convert to c array in init function
henderkes 98aadc4
clang-format
henderkes 87a0874
why is this missing in CI? @dunglas
henderkes f06d146
rename method
henderkes fdbf473
test for phpinfo as plaintext
henderkes ef483ee
suggestion by @dunglas - also include all go modules and go version
henderkes b95c966
don't capitalise Go version in PHPInfo entry, nothing else is capital…
henderkes bec8ae3
Merge remote-tracking branch 'origin/main' into feat/register_phpinfo…
henderkes df91cab
amend cli test
henderkes 1bffc57
hook frankenphp_module into cli execution too
henderkes c3950da
make sure tests set display_errors=1 when they rely on it
henderkes 72c3a25
Merge branch 'main' into feat/register_phpinfo_entries
henderkes 0296a89
fix cli metadata without server runtime hooks
henderkes 8019e98
restore extension registration hooks after cli execution
henderkes 083ba46
respect module replacements in component version entries
henderkes f8eabf6
include main module in phpinfo module inventory
henderkes 5ea3d90
test cli behavior across startup and shutdown
henderkes c604428
test phpinfo module metadata and escaped rendering
henderkes acaad7e
reword comment to shut copilot up
henderkes 0df6262
Merge remote-tracking branch 'origin/main' into feat/register_phpinfo…
henderkes 78136e7
fix: zero phpinfo arrays before Go pointer writes
henderkes 2a4d1d3
fix windows stream closing issue
henderkes 44cfc28
fix linux amd64 race tests
henderkes 1b7a973
satisfy go fmt
henderkes 6d5a4fd
suggestions
henderkes c64dd5b
safety fix for userland closing non-duplicate streams
henderkes 873ca4a
pin instead of manual c memory management
henderkes d055b38
switch to caddy's simpleVersion
henderkes 90a1fb0
move phpinfo helpers into phpinfo.go
henderkes f991a3a
centralize phpinfo module version lookup
henderkes 2b6244b
lazily collect phpinfo metadata through minfo callback
henderkes 57cb92b
add novulcain tag
henderkes 9b824f2
move phpinfo registration into phpinfo.go
henderkes 7fd5b6b
up timeout
henderkes 7a8df3f
fix: move phpinfo printing out of the Go callback
henderkes 0ce2e8e
refactor: inline phpinfo printing into module info callback
henderkes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| //go:build !nomercure | ||
|
|
||
| package main | ||
|
|
||
| import _ "github.com/dunglas/mercure/caddy" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| //go:build !novulcain | ||
|
|
||
| package main | ||
|
|
||
| import _ "github.com/dunglas/vulcain/caddy" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package caddy_test | ||
|
|
||
| import ( | ||
| "html" | ||
| "io" | ||
| "net/http" | ||
| "regexp" | ||
| "testing" | ||
|
|
||
| "github.com/caddyserver/caddy/v2" | ||
| "github.com/caddyserver/caddy/v2/caddytest" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPHPInfoCaddyVersion(t *testing.T) { | ||
| tester := caddytest.NewTester(t) | ||
| initServer(t, tester, ` | ||
| { | ||
| skip_install_trust | ||
| admin localhost:2999 | ||
| } | ||
|
|
||
| http://localhost:`+testPort+` { | ||
| php_server { | ||
| root ../testdata | ||
| } | ||
| } | ||
| `, "caddyfile") | ||
|
|
||
| resp, err := tester.Client.Get("http://localhost:" + testPort + "/phpinfo.php") | ||
| require.NoError(t, err) | ||
| defer func() { require.NoError(t, resp.Body.Close()) }() | ||
| require.Equal(t, http.StatusOK, resp.StatusCode) | ||
| body, err := io.ReadAll(resp.Body) | ||
| require.NoError(t, err) | ||
|
|
||
| row := regexp.MustCompile(`<tr><td class="e">caddy </td><td class="v">(.*?) </td></tr>`).FindSubmatch(body) | ||
| simpleVersion, _ := caddy.Version() | ||
| require.Len(t, row, 2, "phpinfo must include the Caddy version row") | ||
| require.Equal(t, html.EscapeString(simpleVersion), string(row[1])) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| //go:build linux | ||
|
|
||
| package frankenphp_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| func TestExecuteScriptCLIDetachedChild(t *testing.T) { | ||
| const helperEnv = "FRANKENPHP_TEST_DETACHED_CHILD" | ||
| dir := os.Getenv(helperEnv) | ||
| if dir == "" { | ||
| if _, err := os.Stat("internal/testcli/testcli"); err != nil { | ||
| t.Skip("internal/testcli/testcli has not been compiled, run `cd internal/testcli/ && go build`") | ||
| } | ||
| self, err := os.Executable() | ||
| require.NoError(t, err) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) | ||
| defer cancel() | ||
| cmd := exec.CommandContext(ctx, self, "-test.run=^TestExecuteScriptCLIDetachedChild$", "-test.v") | ||
| cmd.Env = append(os.Environ(), helperEnv+"="+t.TempDir()) | ||
| cmd.WaitDelay = time.Second | ||
| output, err := cmd.CombinedOutput() | ||
| var exitError *exec.ExitError | ||
| if errors.As(err, &exitError) && exitError.ExitCode() == 77 { | ||
| t.Skipf("pcntl/posix unavailable: %s", output) | ||
| } | ||
| require.NoError(t, err, "%s", output) | ||
| return | ||
| } | ||
|
|
||
| // PDEATHSIG and subreapers are Linux-specific. Isolate adoption from other tests. | ||
| require.NoError(t, unix.Prctl(unix.PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0)) | ||
| input, release, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| defer func() { _ = input.Close() }() | ||
| pid := 0 | ||
| t.Cleanup(func() { | ||
| // EOF also releases a child whose PID was not reported before a parent failure. | ||
| _ = release.Close() | ||
| if pid > 0 { | ||
| _ = unix.Kill(pid, unix.SIGKILL) | ||
| } | ||
| deadline := time.Now().Add(2 * time.Second) | ||
| for time.Now().Before(deadline) { | ||
| var status unix.WaitStatus | ||
| _, err := unix.Wait4(-1, &status, unix.WNOHANG, nil) | ||
| if errors.Is(err, unix.ECHILD) { | ||
| return | ||
| } | ||
| if err != nil && !errors.Is(err, unix.EINTR) { | ||
| t.Errorf("reaping detached child: %v", err) | ||
| return | ||
| } | ||
| time.Sleep(10 * time.Millisecond) | ||
| } | ||
| t.Error("detached child cleanup timed out") | ||
| }) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 8*time.Second) | ||
| defer cancel() | ||
| ready := filepath.Join(dir, "ready") | ||
| _, err = os.Lstat(ready) | ||
| require.ErrorIs(t, err, os.ErrNotExist, "readiness path must not already exist") | ||
| cmd := exec.CommandContext(ctx, "internal/testcli/testcli", "testdata/command-detached.php") | ||
| // PHP's emulated and native CLIs expose different script argv layouts. | ||
| cmd.Env = append(os.Environ(), "FRANKENPHP_TEST_DETACHED_READY="+ready) | ||
| cmd.Stdin = input | ||
| cmd.WaitDelay = time.Second | ||
| output, err := cmd.CombinedOutput() | ||
| var exitError *exec.ExitError | ||
| if errors.As(err, &exitError) && exitError.ExitCode() == 2 { | ||
| // The fixture checks extensions before forking, so nothing needs reaping. | ||
| t.Logf("%s", output) | ||
| os.Exit(77) | ||
| } | ||
| for _, line := range strings.Split(string(output), "\n") { | ||
| if strings.HasPrefix(line, "CHILD=") { | ||
| pid, _ = strconv.Atoi(strings.TrimPrefix(line, "CHILD=")) | ||
| } | ||
| } | ||
| require.NoError(t, err, "CLI parent: %s", output) | ||
| require.Greater(t, pid, 0, "no child PID: %s", output) | ||
|
|
||
| // CombinedOutput has waited for the actual CLI parent exit, not just readiness. | ||
| // The CLI joins its PHP thread before exiting, so this also covers Linux's | ||
| // PDEATHSIG on the forking thread's exit rather than the whole process's exit. | ||
| _, writeErr := release.WriteString("survived\n") | ||
| deadline := time.Now().Add(5 * time.Second) | ||
| for time.Now().Before(deadline) { | ||
| var status unix.WaitStatus | ||
| got, err := unix.Wait4(pid, &status, unix.WNOHANG, nil) | ||
| if errors.Is(err, unix.EINTR) { | ||
| continue | ||
| } | ||
| require.NoError(t, err) | ||
| if got == pid { | ||
| pid = 0 // Reaped: cleanup must not signal a potentially reused PID. | ||
| require.True(t, status.Exited(), "detached child terminated by signal %d (%s)", status.Signal(), status.Signal()) | ||
| require.Equal(t, 0, status.ExitStatus(), "detached child failed") | ||
| require.NoError(t, writeErr) | ||
| return | ||
| } | ||
| time.Sleep(10 * time.Millisecond) | ||
| } | ||
| t.Fatal("detached child did not finish after CLI parent exited") | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
After trying this out, shouldn't this just be
AddPHPInfoModule?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.
Would it maybe even make sense to just add these aliases directly to phpinfo.go?
And then remove
AddPHPInfoModuleand just keepAddPHPInfoEntryfor extensions.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.
No, because they would be shown unconditionally. I want the modules only shown if they're actually registered. (-tags nomercure)
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.
Version and module import don't have to be the same, e.g. if overwritten to a dir at build.
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.
We can only show them if a version is present in the debug info, no?
What is the use case for that? If you are building from a local fork of Caddy?
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.
Hmm you're right, it seems mercure is still present on -nomercure, only the watcher module is gone on -nowatcher.
Still would prefer to just have 1 function as they do the exact same thing anyways.
Uh oh!
There was an error while loading. Please reload this page.
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.
brotli is also not present on
-nobrotli. Realistically, mercure should not be present on-nomercure, not sure what else is pulling it in.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.
Oh It's still getting pulled in here. So kind of correct that it was in the binary. Probably makes sense to actually remove if on -nomercure (separate PR).
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.
Oh, really? I'll have to investigate that. I tested with mercure and it was always in.
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 think we commented on the exactly same time, removing this line also removes mercure from the debug info
frankenphp/caddy/frankenphp/main.go
Line 9 in 51e6246