Issue/563 http client request factory#564
Conversation
📝 WalkthroughWalkthroughHTTP client adapter ChangesHTTP client lifecycle and creation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant HttpClientFactory
participant HttpClient
Caller->>HttpClientFactory: createRequest or createMultiRequest
HttpClientFactory->>HttpClient: create fresh configured client
HttpClientFactory-->>Caller: return HttpClient
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/HttpClient/Helpers/http_client.php (1)
12-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap global helper functions with
function_exists.It is highly recommended to wrap global helper function declarations in
if (!function_exists(...))checks. This defensive programming practice prevents fatal redeclaration errors if the file is ever loaded multiple times by the autoloader or manual includes.♻️ Proposed refactor
-/** - * Creates single HTTP request client - */ -function httpRequest(string $url): HttpClient -{ - return HttpClientFactory::createRequest($url); -} - -/** - * Creates multi HTTP request client - */ -function httpMultiRequest(): HttpClient -{ - return HttpClientFactory::createMultiRequest(); -} - -/** - * Creates async multi HTTP request client - */ -function httpAsyncMultiRequest(callable $success, callable $error): HttpClient -{ - return HttpClientFactory::createAsyncMultiRequest($success, $error); -} +if (!function_exists('httpRequest')) { + /** + * Creates single HTTP request client + */ + function httpRequest(string $url): HttpClient + { + return HttpClientFactory::createRequest($url); + } +} + +if (!function_exists('httpMultiRequest')) { + /** + * Creates multi HTTP request client + */ + function httpMultiRequest(): HttpClient + { + return HttpClientFactory::createMultiRequest(); + } +} + +if (!function_exists('httpAsyncMultiRequest')) { + /** + * Creates async multi HTTP request client + */ + function httpAsyncMultiRequest(callable $success, callable $error): HttpClient + { + return HttpClientFactory::createAsyncMultiRequest($success, $error); + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/HttpClient/Helpers/http_client.php` around lines 12 - 34, Wrap each global helper declaration—httpRequest, httpMultiRequest, and httpAsyncMultiRequest—in function_exists guards so they are defined only when not already declared, while preserving their existing signatures and delegation to HttpClientFactory.tests/Unit/HttpClient/Factories/HttpClientFactoryTest.php (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that the URL is correctly wired into the client.
While the tests successfully verify that a fresh
HttpClientinstance backed by aCurlAdapteris returned, they do not assert that the$urlargument was properly passed down to the client configuration. Adding these assertions will improve confidence in the parameter wiring.
tests/Unit/HttpClient/Factories/HttpClientFactoryTest.php#L13-L21: Add$this->assertSame('https://example.com', $httpClient1->url());(and similarly for$httpClient2) to verify the URL was set.tests/Unit/HttpClient/Helpers/HttpClientHelperFunctionsTest.php#L12-L20: Add the same assertions here to ensure the helper perfectly forwards the URL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/HttpClient/Factories/HttpClientFactoryTest.php` around lines 13 - 21, Update testHttpClientFactoryCreatesSingleRequest in tests/Unit/HttpClient/Factories/HttpClientFactoryTest.php at lines 13-21 to assert each returned client's url() matches its input URL; add equivalent URL assertions in tests/Unit/HttpClient/Helpers/HttpClientHelperFunctionsTest.php at lines 12-20 to verify helper forwarding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/HttpClient/Helpers/http_client.php`:
- Around line 12-34: Wrap each global helper declaration—httpRequest,
httpMultiRequest, and httpAsyncMultiRequest—in function_exists guards so they
are defined only when not already declared, while preserving their existing
signatures and delegation to HttpClientFactory.
In `@tests/Unit/HttpClient/Factories/HttpClientFactoryTest.php`:
- Around line 13-21: Update testHttpClientFactoryCreatesSingleRequest in
tests/Unit/HttpClient/Factories/HttpClientFactoryTest.php at lines 13-21 to
assert each returned client's url() matches its input URL; add equivalent URL
assertions in tests/Unit/HttpClient/Helpers/HttpClientHelperFunctionsTest.php at
lines 12-20 to verify helper forwarding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 466a060b-5de7-465b-acb7-af8fc48123d2
📒 Files selected for processing (10)
src/HttpClient/Adapters/CurlAdapter.phpsrc/HttpClient/Adapters/MultiCurlAdapter.phpsrc/HttpClient/Contracts/HttpClientAdapterInterface.phpsrc/HttpClient/Enums/HttpClientType.phpsrc/HttpClient/Factories/HttpClientFactory.phpsrc/HttpClient/Helpers/http_client.phptests/Unit/HttpClient/Adapters/CurlAdapterTest.phptests/Unit/HttpClient/Adapters/MultiCurlAdapterTest.phptests/Unit/HttpClient/Factories/HttpClientFactoryTest.phptests/Unit/HttpClient/Helpers/HttpClientHelperFunctionsTest.php
💤 Files with no reviewable changes (1)
- src/HttpClient/Enums/HttpClientType.php
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34b554c60c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3 participants
Closes #563
Summary by CodeRabbit
New Features
Improvements
Removed