fix: add E2E tests and CHANGELOG entry for SERVER_PROTOCOL (#60)#110
fix: add E2E tests and CHANGELOG entry for SERVER_PROTOCOL (#60)#110
Conversation
s2x
left a comment
There was a problem hiding this comment.
Code Review
Thanks for this PR. The test itself is well-structured, but there are several issues with the CHANGELOG entry that need to be addressed before merge.
Important
1. CHANGELOG entry is under the wrong section
The entry is placed under ### Added, but it describes a fix ("Fixed SERVER_PROTOCOL format..."). The project has a ### Fixed section that should be used here. Compare with the existing StreamedResponse entry (line 16) which correctly uses ### Added with the framing "Added E2E tests for..." — that's the appropriate pattern when a PR only adds test coverage.
2. CHANGELOG wording is misleading
"Fixed SERVER_PROTOCOL format to include HTTP/ prefix (#60)" reads as if this PR introduced the actual code fix. However, the fix ('HTTP/' . $rawRequest->protocolVersion()) was already merged in PR #101 (commit cac2de8). This PR only adds test coverage and documentation. The entry should reflect that — e.g., "Added E2E test verifying SERVER_PROTOCOL includes HTTP/ prefix (#60)".
3. Duplicate CHANGELOG entry
The fix is already documented on line 96 of CHANGELOG.md:
SERVER_PROTOCOL now has correct HTTP/1.1 format instead of 1.1
Adding a second entry under [Unreleased] creates duplication. Either:
- Remove the new entry entirely (the fix is already documented), or
- Keep it but reword to clarify this PR adds test coverage for the fix, not the fix itself
Minor
4. Test doesn't verify HTTP/2.0 path
The raw request is hardcoded to HTTP/1.1. If the implementation uses 'HTTP/' . $rawRequest->protocolVersion(), it would be valuable to also assert against HTTP/2.0 to confirm the prefix logic works for other protocol versions. Not critical since HTTP/1.1 is the common case and the implementation is trivial string concatenation, but worth considering for completeness.
Summary
The E2E test is solid — it follows existing patterns, uses dual assertions (server->get('SERVER_PROTOCOL') and getProtocolVersion()), and correctly verifies the behavior described in issue #60. The CHANGELOG entry needs rewording and/or deduplication before this is ready to merge.
Summary
testServerProtocolHasHttpPrefixE2E()verifying SERVER_PROTOCOL includes HTTP/ prefixChanges
tests/SymfonyControllerTest.php- new E2E test methodCHANGELOG.md- entry referencing issue RequestConverter — SERVER_PROTOCOL has wrong format (missing HTTP/ prefix) #60Verification