refactor(Request): Remove getScriptUrl() method for ServerRequestAdapter class and related tests; introduce getScriptName() for improved script URL handling in PSR-7 and Yii2 environments.
#78
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
…dapter` class and related tests; introduce `getScriptName()` for improved script URL handling in PSR-7 and Yii2 environments.
WalkthroughThe changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Request
participant Adapter
Client->>Request: getScriptUrl()
alt Adapter is set and workerMode
Request-->>Client: ""
else Adapter is set and traditional mode
Request->>Request: getScriptName()
Request-->>Client: SCRIPT_NAME or ""
else No adapter
Request->>Request: parent::getScriptUrl()
Request-->>Client: parent result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 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). (4)
✨ 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 (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
===========================================
Coverage 100.00% 100.00%
+ Complexity 313 311 -2
===========================================
Files 12 12
Lines 790 789 -1
===========================================
- Hits 790 789 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/http/Request.php (1)
515-539: getScriptUrl(): Logic LGTM; simplify control flow and fix misleading @throwsBehavior matches tests: empty string in worker mode when adapter is set; otherwise return SCRIPT_NAME; fallback to parent. Use a simple if for readability and remove
@throwssince nothing in this method throws.- /** - * Retrieves the script URL for the current request, supporting PSR-7 and Yii2 fallback. - * - * Returns the script URL as an empty string for PSR-7 worker environments (such as RoadRunner, FrankenPHP, etc.) - * where no script file exists, preventing URL duplication as routing is handled internally. If not in worker mode, - * return the script name. Falls back to the parent implementation if no PSR-7 adapter is set. - * - * This method enables seamless interoperability with both PSR-7 and Yii2 environments, ensuring the correct script - * URL resolution for modern HTTP stacks and legacy workflows. - * - * @throws InvalidConfigException if the configuration is invalid or incomplete. - * - * @return string Script URL for the current request, or an empty string in worker mode. - * - * Usage example: - * ```php - * $scriptUrl = $request->getScriptUrl(); - * ``` - */ + /** + * Retrieves the script URL for the current request, supporting PSR-7 and Yii2 fallback. + * + * In worker environments (RoadRunner, FrankenPHP, etc.) with a PSR-7 adapter set, returns an empty string, since no + * script file exists and routing is handled by the worker. In traditional mode, returns 'SCRIPT_NAME'. Falls back + * to the parent implementation if no PSR-7 adapter is set. + * + * @return string Script URL for the current request, or an empty string in worker mode. + * + * Usage example: + * ```php + * $scriptUrl = $request->getScriptUrl(); + * ``` + */ public function getScriptUrl(): string { - return match ($this->adapter !== null) { - true => $this->workerMode === true ? '' : $this->getScriptName(), - default => parent::getScriptUrl(), - }; + if ($this->adapter !== null) { + return $this->workerMode ? '' : $this->getScriptName(); + } + return parent::getScriptUrl(); }
🧹 Nitpick comments (1)
src/http/Request.php (1)
590-593: Minor: avoid duplicate getServerParams() callsReadability and micro-efficiency: call
getServerParams()once.- public function getServerParam(string $name, mixed $default = null): mixed - { - return array_key_exists($name, $this->getServerParams()) ? $this->getServerParams()[$name] : $default; - } + public function getServerParam(string $name, mixed $default = null): mixed + { + $params = $this->getServerParams(); + return array_key_exists($name, $params) ? $params[$name] : $default; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/adapter/ServerRequestAdapter.php(0 hunks)src/http/Request.php(8 hunks)tests/adapter/ServerParamsPsr7Test.php(2 hunks)tests/adapter/ServerRequestAdapterTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- src/adapter/ServerRequestAdapter.php
- tests/adapter/ServerRequestAdapterTest.php
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
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.
📚 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/adapter/ServerParamsPsr7Test.php
📚 Learning: 2025-08-06T22:52:05.608Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#64
File: tests/http/StatelessApplicationTest.php:1939-1967
Timestamp: 2025-08-06T22:52:05.608Z
Learning: In yii2-extensions/psr-bridge tests, when testing specific component methods like Request::resolve(), it's necessary to call $app->handle($request) first to initialize all application components before testing the method in isolation. This ensures proper component lifecycle initialization.
Applied to files:
tests/adapter/ServerParamsPsr7Test.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/adapter/ServerParamsPsr7Test.php
🪛 GitHub Actions: ecs
tests/adapter/ServerParamsPsr7Test.php
[error] 440-500: PHP CS Fixer detected an error in code style/order in this test file. The error is fixable by running the tool with the '--fix' option.
⏰ 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). (3)
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (5)
tests/adapter/ServerParamsPsr7Test.php (1)
446-498: Drop unnecessary @throws annotations in tests
The@throws InvalidConfigExceptiondocblocks in these PHPUnit methods aren’t needed and can trigger ECS/CS ordering/style issues. Removing them keeps the tests focused and should resolve any style violations.Please remove the docblocks in
tests/adapter/ServerParamsPsr7Test.phparound these methods and then verify compliance by running your project’s ECS or PHP CS Fixer:--- a/tests/adapter/ServerParamsPsr7Test.php +++ b/tests/adapter/ServerParamsPsr7Test.php @@ -446,9 +446,6 @@ final class ServerParamsPsr7Test extends TestCase - /** - * @throws InvalidConfigException if the configuration is invalid or incomplete. - */ public function testReturnEmptyScriptUrlWhenAdapterIsSetInTraditionalModeWithoutScriptName(): void { $request = new Request(['workerMode' => false]); @@ -464,9 +461,6 @@ final class ServerParamsPsr7Test extends TestCase - /** - * @throws InvalidConfigException if the configuration is invalid or incomplete. - */ public function testReturnEmptyScriptUrlWhenAdapterIsSetInWorkerMode(): void { $request = new Request(); @@ -482,9 +476,6 @@ final class ServerParamsPsr7Test extends TestCase - /** - * @throws InvalidConfigException if the configuration is invalid or incomplete. - */ public function testReturnScriptNameWhenAdapterIsSetInTraditionalMode(): void { $expectedScriptName = '/app/public/index.php';Run your style checker locally to confirm no remaining violations.
src/http/Request.php (4)
823-839: New helper getScriptName(): clear and correctStraightforward, type-safe retrieval of SCRIPT_NAME. No issues.
158-159: Doc type refinement for getBodyParams()Type expansion to
array|objectis appropriate given PSR-7 parsed body possibilities.
320-321: Doc type refinement for getParsedBody()Allowing
object|nullmatches PSR-7'sgetParsedBody()contract. LGTM.
533-539: AllServerRequestAdapter::getScriptUrl()references have been removedRipgrep searches confirm there’s no
getScriptUrl()declaration inServerRequestAdapterand no calls toadapter->getScriptUrl(). No further action required.
…notation for clarity; add tests for script URL handling in traditional and worker modes in `ServerParamsPsr7Test`.
…oadedFiles()` and `convertPsr7ToUploadedFiles()` methods for improved type clarity.
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
Documentation
Bug Fixes
Tests