refactor(tests): Add comprehensive tests for getServerPort() method in Request class to ensure correct handling of forwarded ports and server parameters.
#84
Conversation
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
… in `Request` class to ensure correct handling of forwarded ports and server parameters.
WalkthroughAdds PSR-7–aware server port retrieval and PSR-7 uploaded-file conversion to Request; introduces data-driven server-port tests and a provider enumerating port-resolution scenarios and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Request
participant Adapter
participant PSR7 as "PSR-7 Request"
Client->>Request: getServerPort()
alt Adapter present
Request->>Adapter: getHeaders()
Adapter-->>Request: header values
Note right of Request: Check configured portHeaders\nfor numeric forwarded values and trusted host
alt found numeric forwarded port
Request-->>Client: return int port
else
Request->>Adapter: getServerParams()
Adapter-->>Request: SERVER_PORT
alt numeric SERVER_PORT
Request-->>Client: return int port
else
Request-->>Client: return null
end
end
else No adapter
Request-->>Client: return parent::getServerPort()
end
sequenceDiagram
participant Client
participant Request
participant Adapter
participant PSR7 as "PSR-7 UploadedFiles"
Client->>Request: getUploadedFiles()
alt Adapter present
Request->>Adapter: getUploadedFiles()
Adapter-->>Request: PSR-7 UploadedFileInterface[]
Request->>Request: convert recursively to Yii UploadedFile[]
Request-->>Client: return Yii UploadedFile[]
else No adapter
Request-->>Client: return []
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/http/Request.php (2)
650-651: Prefer getHeaders() over magic property to access HeaderCollectionUsing $this->getHeaders() is clearer and avoids relying on magic property resolution.
Apply this minimal change:
- if ($this->headers->has($portHeader)) { - $port = $this->headers->get($portHeader); + $headers = $this->getHeaders(); + if ($headers->has($portHeader)) { + $port = $headers->get($portHeader);
648-662: Consolidate header access and validation (combined improvement)If you prefer a single diff, this folds both the header access nit and input validation into one change.
- if ($this->adapter !== null) { - foreach ($this->portHeaders as $portHeader) { - if ($this->headers->has($portHeader)) { - $port = $this->headers->get($portHeader); - - if (is_numeric($port)) { - return (int) $port; - } - } - } - - $port = $this->getServerParam('SERVER_PORT'); - - return is_numeric($port) ? (int) $port : null; - } + if ($this->adapter !== null) { + $headers = $this->getHeaders(); + foreach ($this->portHeaders as $portHeader) { + if ($headers->has($portHeader)) { + $raw = $headers->get($portHeader); + $first = is_array($raw) ? reset($raw) : (string) $raw; + $candidate = trim(strtok($first, ',')); + $validated = filter_var( + $candidate, + FILTER_VALIDATE_INT, + ['options' => ['min_range' => 1, 'max_range' => 65535]], + ); + if ($validated !== false) { + return $validated; + } + } + } + + $rawServerPort = $this->getServerParam('SERVER_PORT'); + $first = is_array($rawServerPort) ? reset($rawServerPort) : $rawServerPort; + if (is_string($first) || is_int($first)) { + $validated = filter_var( + trim((string) $first), + FILTER_VALIDATE_INT, + ['options' => ['min_range' => 1, 'max_range' => 65535]], + ); + return $validated !== false ? $validated : null; + } + return null; + }tests/adapter/ServerParamsPsr7Test.php (4)
20-51: Good untrusted-host case; consider restoring $_SERVER after testSolid assertion of header filtering. To avoid cross-test leakage, snapshot and restore $_SERVER in a finally block or use tearDown to reset.
$prev = $_SERVER; try { $_SERVER = ['REMOTE_ADDR' => '192.168.1.100']; // ...test body... } finally { $_SERVER = $prev; }
183-212: Trusted proxy coverage looks correctForwarded port wins when host is trusted. Same minor note about restoring $_SERVER applies here.
549-567: Covers numeric-string to int coercionAs a micro-nit, store the value once to avoid calling getServerPort() twice, but it’s minor in tests.
569-615: Multiple configured headers precedence coveredNice. Consider adding a case for comma-separated header values (e.g. 'X-Forwarded-Port: 9443, 7443') to ensure only the first numeric value is used.
I can add a test like:
public function testReturnServerPortFromCommaSeparatedForwardedHeader(): void { $_SERVER = ['REMOTE_ADDR' => '127.0.0.1']; $request = new Request([ 'portHeaders' => ['X-Forwarded-Port'], 'secureHeaders' => ['X-Forwarded-Port'], 'trustedHosts' => ['127.0.0.1'], ]); $request->setPsr7Request( FactoryHelper::createRequest( 'GET', '/test', ['X-Forwarded-Port' => '9443, 7443'], serverParams: ['SERVER_PORT' => '80', 'REMOTE_ADDR' => '127.0.0.1'], ), ); self::assertSame(9443, $request->getServerPort()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/http/Request.php(2 hunks)tests/adapter/ServerParamsPsr7Test.php(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#78
File: src/http/Request.php:0-0
Timestamp: 2025-08-10T13:17:04.252Z
Learning: In the yii2-extensions/psr-bridge project, PHPStan type annotations should be defensive when dealing with external input from PSR-7 sources. For methods like `getUploadedFiles()` and `convertPsr7ToUploadedFiles()`, use `mixed` in the return type annotation to account for untrusted input values, even if the implementation filters them.
🧬 Code Graph Analysis (1)
tests/adapter/ServerParamsPsr7Test.php (2)
src/http/Request.php (4)
Request(55-884)setPsr7Request(802-805)getServerPort(646-665)reset(737-740)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createRequest(68-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (10)
src/http/Request.php (1)
20-20: Import of is_numeric looks goodUseful for numeric gate checks. No issues.
tests/adapter/ServerParamsPsr7Test.php (9)
215-229: Empty array -> null case is correctCovers malformed server param shape well.
231-245: Absent SERVER_PORT -> null case is correct
247-261: Non-string SERVER_PORT -> null case is correct
263-277: Explicit null SERVER_PORT -> null case is correct
616-646: Forwarded header preferred over server param (trusted) — correct
648-664: Falls back to SERVER_PORT when header absent — correct
665-690: Fallback to SERVER_PORT when all headers empty — correct
769-808: Reset path validatedGood coverage ensuring independence across adapter resets.
809-847: Independence across instances validatedClear and correct assertions.
…aders variable for better readability and performance.
…erverPort()` method of `Request` class.
Member
Author
|
@coderabbitai Server port tests could be summarized using a data provider such as testReturnServerParamFromPsr7RequestCases() |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3263 🏁 Script executed: Length of output: 2189 🏁 Script executed: Length of output: 37799 @terabytesoftw Great suggestion! Using data providers would indeed make the server port tests more maintainable and reduce code duplication. Here's a refactored version: Add to
|
…ider for server port cases in `ServerParamsPsr7Provider` class.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
========================================
Coverage ? 100.00%
Complexity ? 321
========================================
Files ? 12
Lines ? 805
Branches ? 0
========================================
Hits ? 805
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/provider/ServerParamsPsr7Provider.php (5)
10-21: Tighten type shape and document tuple positions for clarity and toolingNarrow header value types to match PSR-7 (strings or list of strings) and label tuple positions. This improves IDE/static analysis and reduces misuse.
- /** - * @phpstan-return array< - * string, - * array{ - * array<string, mixed>, - * array<string, mixed>, - * array<string, array<int, string>|int|string>, - * array<string, mixed>, - * int|null, - * string, - * } - * > - */ + /** + * @phpstan-return array< + * string, + * array{ + * 0: array{portHeaders?: list<string>, secureHeaders?: list<string>, trustedHosts?: list<string>}, + * 1: array<string, mixed>, // SAPI/env server params (e.g., REMOTE_ADDR) + * 2: array<string, string|list<string>>, // HTTP headers + * 3: array<string, mixed>, // PSR-7 serverParams (e.g., SERVER_PORT) + * 4: int|null, // expected resolved port + * 5: string // assertion message + * } + * > + */
25-33: Be explicit about proxy trust gating for forwarded headersThis positive case uses forwarded port but omits secureHeaders. To make intent unambiguous (and symmetric with the “untrusted host” case), include X-Forwarded-Port in secureHeaders so forwarded data is only honored when the client is trusted.
'Forwarded port when request from trusted proxy' => [ [ 'portHeaders' => ['X-Forwarded-Port'], + 'secureHeaders' => ['X-Forwarded-Port'], 'trustedHosts' => ['10.0.0.0/24'], // trust this subnet ],If the Request implementation already gates by trustedHosts regardless of secureHeaders, keeping secureHeaders here still clarifies the contract.
81-89: Consider adding an explicit integer SERVER_PORT caseSome PSR-7 implementations may surface SERVER_PORT as an int. Add a case to assert intended behavior (accept and cast or reject as non-string).
'Null when PSR-7 request server port is not string' => [ [], [], [], ['SERVER_PORT' => ['invalid' => 'array']], null, "'SERVER_PORT' should return 'null' from PSR-7 'serverParams' when adapter is set but 'SERVER_PORT' " . 'is not a string.', ], + 'Handle integer when PSR-7 request server port is int' => [ + [], + [], + [], + ['SERVER_PORT' => 443], + null, // or 443 if you intend to accept ints + "'SERVER_PORT' should " . (/* choose one */ true ? "return 'null' when it's an integer." : "accept integer and return 443.") , + ],Tell me which behavior you prefer and I can update the provider and tests accordingly.
144-158: Forwarded-port case: consider including secureHeaders for parityLike the trusted/untrusted scenarios, include X-Forwarded-Port in secureHeaders to make the trust requirement explicit.
'Server port from forwarded header when adapter is set' => [ [ 'portHeaders' => ['X-Forwarded-Port'], + 'secureHeaders' => ['X-Forwarded-Port'], 'trustedHosts' => ['127.0.0.1'], ],
159-171: Message nit: reflect “empty string” explicitlyThe case uses an empty string header value, so tweak the message to match the input.
- "'getServerPort()' should fallback to 'SERVER_PORT' when all forwarded headers are 'null' or missing.", + "'getServerPort()' should fallback to 'SERVER_PORT' when all forwarded headers are null/empty or missing.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/adapter/ServerParamsPsr7Test.php(3 hunks)tests/provider/ServerParamsPsr7Provider.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/adapter/ServerParamsPsr7Test.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-20T16:35:15.341Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1536-1552
Timestamp: 2025-07-20T16:35:15.341Z
Learning: In the yii2-extensions/psr-bridge project, the base TestCase class already handles $_SERVER cleanup in setUp() and tearDown() methods, so individual test methods that extend TestCase don't need manual $_SERVER restoration.
Applied to files:
tests/provider/ServerParamsPsr7Provider.php
📚 Learning: 2025-07-20T16:35:15.341Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1536-1552
Timestamp: 2025-07-20T16:35:15.341Z
Learning: In the yii2-extensions/psr-bridge project, the base TestCase class already handles $_SERVER cleanup in setUp() and tearDown() methods (lines 28 and 32), so individual test methods that extend TestCase don't need manual $_SERVER restoration.
Applied to files:
tests/provider/ServerParamsPsr7Provider.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (3)
tests/provider/ServerParamsPsr7Provider.php (3)
54-89: Good coverage of null/missing/invalid PSR-7 SERVER_PORT casesThese cases exercise empty array, null, missing, and non-string. Solid negative-path coverage.
98-112: Comma-separated header case is on pointCovers trimming and first-value selection for X-Forwarded-Port. Nice.
113-143: First valid header among multiple configured is well coveredThis ensures precedence is deterministic across configured portHeaders.
…msPsr7Test` class.
…Port()' when 'SERVER_PORT' its integer.
This was referenced Aug 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Tests