fix: add SERVER_PORT to server bag in RequestConverter (#65)#111
Conversation
- Enables $request->getPort() for non-standard ports (8080, 8443) - Falls back to port 80 when connection is not available - Add CHANGELOG entry requirement to CONTRIBUTING.md
s2x
left a comment
s2x
left a comment
There was a problem hiding this comment.
Code Review
Great work on this implementation — the one-line change is clean and follows the existing patterns for REMOTE_ADDR/REMOTE_PORT. Below are a few points to address before merge.
Important
1. Missing test for the exact issue #65 scenario
File: tests/RequestConverterTest.php
The current test testGetPortReturnsServerPortWhenNoHostHeader uses a request with no Host header at all, which is technically invalid for HTTP/1.1. The actual production scenario from issue #65 is when the Host header exists but has no port (e.g., Host: example.com), and Symfony should fall back to SERVER_PORT.
Why it matters: This is the exact case described in the issue — behind a reverse proxy that strips the port from the Host header, URL generation produces wrong results because getSchemeAndHttpHost() doesn't include the port.
Suggested test to add:
public function testGetSchemeAndHttpHostIncludesPortWhenHostHeaderHasNoPort(): void
{
// Exact scenario from issue #65: Host header without port, non-standard server port
$buffer = "GET /test HTTP/1.1\r\nHost: example.com\r\n\r\n";
$rawRequest = new Request($buffer);
$mockConnection = new class extends \Workerman\Connection\TcpConnection {
public function __construct()
{
$this->remoteAddress = '192.168.1.1:12345';
}
public function getLocalPort(): int
{
return 8080;
}
};
$rawRequest->connection = $mockConnection;
$symfonyRequest = RequestConverter::toSymfonyRequest($rawRequest);
// This is the broken behavior from issue #65 — should include :8080
$this->assertSame('http://example.com:8080', $symfonyRequest->getSchemeAndHttpHost());
$this->assertSame(8080, $symfonyRequest->getPort());
}Minor
2. Duplicated mock connection class (DRY violation)
File: tests/RequestConverterTest.php:264-342
The anonymous mock connection class is copy-pasted 4 times across testServerPortFromConnection, testGetPortReturnsServerPortWhenNoHostHeader, and testGetPortReturnsPortFromHostHeaderWhenPresent. Only the getLocalPort() return value differs.
Why it matters: If the mock needs updating (e.g., new Workerman interface changes, additional required methods), it must be changed in 4 places.
Suggested fix: Extract a reusable helper method:
private function createMockConnection(int $localPort): \Workerman\Connection\TcpConnection
{
return new class($localPort) extends \Workerman\Connection\TcpConnection {
private int $port;
public function __construct(int $port) { $this->port = $port; $this->remoteAddress = '192.168.1.1:12345'; }
public function getLocalPort(): int { return $this->port; }
};
}3. Hardcoded fallback 80 assumes HTTP
File: src/DTO/RequestConverter.php:38
'SERVER_PORT' => $rawRequest->connection?->getLocalPort() ?? 80,The fallback to 80 assumes HTTP traffic. For HTTPS connections (e.g., TLS termination at Workerman level or X-Forwarded-Proto: https), the conventional default should be 443.
Why it matters: If the bundle handles HTTPS traffic and the connection is unavailable, getSchemeAndHttpHost() could produce http://example.com:80 instead of https://example.com for HTTPS requests. Symfony's Request::getPort() returns null for standard ports when it can infer the scheme, but a hardcoded 80 could interfere with this.
Suggested fix: Consider detecting the scheme (from X-Forwarded-Proto header, connection type, or REQUEST_SCHEME) and using 443 for HTTPS. Alternatively, document this as a known limitation if TLS is not supported by the bundle.
Summary
| # | Severity | Status |
|---|---|---|
| 1 | Important | Add test for Host header without port |
| 2 | Minor | Extract mock connection helper |
| 3 | Minor | Consider HTTPS-aware fallback |
The core implementation is correct and production-ready. Point #1 is the most valuable addition — it proves the fix solves the actual issue #65 scenario.
- Add SERVER_NAME to server bag to fix getPort() and getSchemeAndHttpHost() - Extract createMockConnection() helper method to avoid DRY violation - Add test for Host header without port scenario
- Add SERVER_NAME from getLocalIp() to fix getPort() when Host header has no port - Add getLocalIp() to mock connection in tests - Update CHANGELOG entry
Addressed Review Comments1. Added test for issue #65 scenarioI added the test you suggested, but discovered an important limitation: Symfony's The fix requires trusted proxies to be configured with 2. Extracted createMockConnection() helperCreated a reusable 3. Added SERVER_NAMEDiscovered that 'SERVER_NAME' => $rawRequest->connection?->getLocalIp() ?? 'localhost',Summary
|
Follow-up: Detailed thoughts on points #1 and #3Thanks for addressing the review feedback so quickly. I want to share more detailed reasoning on two points that remain open, so we can decide together whether they need action. Point #1 — Test for
|
| Point | Action | Priority |
|---|---|---|
| #1 | Add getSchemeAndHttpHost() test for Host: example.com + port 8080 |
Low — proves the fix works |
| #3 | Keep as-is (Option A), document TLS as future work | Low — connection=null is test-only |
Neither is a blocker. Both are about documentation and confidence in the fix.
Recommendation: Option B — HTTPS-aware fallbackAfter thinking about this more, I'd recommend Option B for this PR rather than deferring it. Here's why: The problem with the current approachRight now the fallback is hardcoded to port 80:
This means if Proposed implementation
Why do this now, not later?
Tests to add
SummaryThis isn't a big feature — it's 5 lines of code + 3 tests. And it prevents a subtle but serious bug: HTTPS requests behind a reverse proxy being treated as HTTP. I'd rather fix it now than have someone debug why |
1 similar comment
Recommendation: Option B — HTTPS-aware fallbackAfter thinking about this more, I'd recommend Option B for this PR rather than deferring it. Here's why: The problem with the current approachRight now the fallback is hardcoded to port 80:
This means if Proposed implementation
Why do this now, not later?
Tests to add
SummaryThis isn't a big feature — it's 5 lines of code + 3 tests. And it prevents a subtle but serious bug: HTTPS requests behind a reverse proxy being treated as HTTP. I'd rather fix it now than have someone debug why |
- Detect HTTPS from connection port 443 - Detect HTTPS from X-Forwarded-Proto header - Set HTTPS=on for HTTPS requests - Add tests for HTTPS detection and URL generation This fixes getScheme() returning 'http' for HTTPS requests.
Implemented Option B — HTTPS-aware fallbackDone! I've implemented Option B as recommended: Changes Made
$isHttps = $localPort === 443 || $forwardedProto === 'https';
$forwardedProto = strtolower((string) $rawRequest->header('x-forwarded-proto', ''));
if ($isHttps) {
$server['HTTPS'] = 'on';
}Tests Added
CHANGELOG UpdatedAdded entries for HTTPS detection support. All tests pass, lint passes. |
Missing test:
|
1 similar comment
Missing test:
|
Update on remaining test requestI investigated the requested test for FindingsAfter extensive debugging, I found that Symfony's The test
This is consistent with PHP-FPM behavior - when the Host header has no port, Symfony returns the default port regardless of SERVER_PORT. What the fix DOES solve
Why this is acceptableThe fix solves the core issue #65: non-standard ports are now available in the server bag. What Symfony does with them (inferring default ports when Host header is present) is consistent with PHP-FPM and is Symfony's design decision, not a bug in our implementation. All tests pass. The implementation is correct. |
1 participant
Summary
SERVER_PORTto server bag inRequestConverter$request->getPort()for non-standard ports (8080, 8443, etc.)Changes
src/DTO/RequestConverter.php- AddedSERVER_PORTfrom$connection->getLocalPort()tests/RequestConverterTest.php- Added 4 unit testsAdditional