Skip to content

fix: include FrankenPHP version in startup logs#2551

Open
ousamabenyounes wants to merge 1 commit into
php:mainfrom
ousamabenyounes:fix/issue-2483
Open

fix: include FrankenPHP version in startup logs#2551
ousamabenyounes wants to merge 1 commit into
php:mainfrom
ousamabenyounes:fix/issue-2483

Conversation

@ousamabenyounes

@ousamabenyounes ousamabenyounes commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds the configured FrankenPHP build version to startup log attributes.
  • Keeps the version internal to the FrankenPHP package and reads the existing FRANKENPHP_VERSION C build flag through a package-local cgo helper.
  • Keeps the Caddy module code unchanged outside the startup log call path.
Q A
Issues Fix #2483
Tests yes

Test verification (RED -> GREEN)

RED on origin/main with the new startup-log test copied in:

# github.com/dunglas/frankenphp [github.com/dunglas/frankenphp.test]
./startup_log_test.go:20:31: undefined: frankenPHPVersion
./startup_log_test.go:23:11: undefined: startupLogAttrs
./startup_log_test.go:27:21: undefined: startupLogAttrVersion
./startup_log_test.go:28:62: undefined: startupLogAttrVersion
FAIL	github.com/dunglas/frankenphp [build failed]
FAIL

GREEN after the review cleanup:

$ docker run --rm --label com.ora.cleanup.scope=pr-2551 -v "$PWD":/src -w /src dunglas/frankenphp:1.12.6-builder-php8 /bin/sh -lc 'export PATH=/usr/local/go/bin:$PATH; gofmt -w frankenphp.go startup_log.go startup_log_test.go && ./go.sh test -run TestStartupLogAttrsIncludeFrankenPHPVersion -count=1 .'
ok  	github.com/dunglas/frankenphp	0.017s

Additional validation:

git diff --check

@dunglas

dunglas commented Jul 21, 2026

Copy link
Copy Markdown
Member

To me, this shouldn't be configurable with a Go API. This is already doable using build flags. I don't want to introduce the API surface for that.

ousamabenyounes added a commit to ousamabenyounes/frankenphp that referenced this pull request Jul 21, 2026
@ousamabenyounes

Copy link
Copy Markdown
Contributor Author

Thanks, addressed in 73e6a61.

I removed the public Go option and the Caddy parser. The startup log version is now kept internal to the frankenphp package and set from Docker build flags with:

-X github.com/dunglas/frankenphp.frankenPHPVersion=$FRANKENPHP_VERSION

Local validation:

./go.sh test -run TestStartupLogAttrsIncludeBuildFlagFrankenPHPVersion -count=1 .
FRANKENPHP_EXPECT_LDFLAG_VERSION=1.12.4-test ./go.sh test -ldflags "-X github.com/dunglas/frankenphp.frankenPHPVersion=1.12.4-test" -run TestStartupLogAttrsIncludeBuildFlagFrankenPHPVersion -count=1 .
./go.sh test -count=1 .

@henderkes

Copy link
Copy Markdown
Contributor

I don't understand why this is necessary. We already have the caddy version string which includes the frankenphp version?

ousamabenyounes added a commit to ousamabenyounes/frankenphp that referenced this pull request Jul 23, 2026
@ousamabenyounes

ousamabenyounes commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Good point. The existing Caddy custom version is still the source used by frankenphp version, but it is not emitted in the current runtime startup log. Before this PR the startup log was only:

{"php_version":"...","num_threads":...,"max_threads":...,"max_requests":...}

I updated the patch so it does not add another Go ldflag or parse Caddy’s version string. FrankenPHP already has the FRANKENPHP_VERSION build-time macro for the PHP extension version, so the startup log now reads that internal value directly and emits it as a structured version attribute. That keeps this out of the public Go API and should cover the existing Docker, Windows, and static build paths that already set FRANKENPHP_VERSION.

Validated locally in the builder image:

./go.sh test -run TestStartupLogAttrsIncludeFrankenPHPVersion -count=1 .
FRANKENPHP_EXPECT_VERSION=1.12.4-test CGO_CFLAGS="-DFRANKENPHP_VERSION=1.12.4-test" ./go.sh test -run TestStartupLogAttrsIncludeFrankenPHPVersion -count=1 .
./go.sh test -count=1 .

ousamabenyounes added a commit to ousamabenyounes/frankenphp that referenced this pull request Jul 23, 2026
Comment thread caddy/app.go Outdated
Comment thread frankenphp.go
@ousamabenyounes

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review follow-up: caddy/app.go is no longer part of the PR diff, and I replied on the version-source question explaining why the patch reuses the existing FRANKENPHP_VERSION C build flag instead of adding another Go ldflag path.

Validation: the pushed SHA c47ae88 has the full GitHub test/check matrix green; local check here: git diff --check.

@dunglas

dunglas commented Jul 24, 2026

Copy link
Copy Markdown
Member

I still don't understand the use case for having the version number in logs.

@ousamabenyounes

Copy link
Copy Markdown
Contributor Author

@dunglas the use case is environments where operators only have access to centralized logs and not necessarily interactive shell access to the running instance. The startup log currently includes php_version, num_threads, max_threads and max_requests, but not the FrankenPHP version. Recording it there provides a simple deployment trace and helps identify which FrankenPHP version was started when reviewing startup logs. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include frankenphp version in logs

3 participants