Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe pull request updates Httpful to require PHP 8.0+ and significantly enhances the request/response API by introducing bearer token authentication, configurable retry logic with exponential backoff, advanced TLS/certificate options, cookie persistence, HTTP/2 and HTTP/3 support, Alt-Svc/HSTS caching, proxy routing, and response transfer-info extraction. Type annotations are strengthened throughout. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Request
participant Curl
participant Network
participant Response
Client->>Request: withBearerAuth() / withRetry() / withTls*()
Request->>Request: Store retry policy & auth config
Client->>Request: send()
Request->>Request: _curlPrep()
Request->>Curl: configureAuthAndOptions()
Curl->>Curl: setOpt(CURLOPT_HTTPAUTH, etc.)
Curl->>Curl: _configureRetryBehavior()
Curl->>Network: exec() with retry loop
Network-->>Curl: Response / timeout / connection error
alt Retryable Error & Retries Remain
Curl->>Curl: Apply exponential backoff
Curl->>Network: Retry exec()
else Max Retries or Non-Retryable
Curl-->>Request: Final response or error
end
Request->>Response: Parse & populate transfer info
Response->>Response: getEffectiveUrl() / getTotalTime() / getTransferHttpVersion()
Response-->>Client: Completed Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 46 minutes and 46 seconds.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the library to PHP 8.0+, bumps PHPStan to version 2.x, and introduces a comprehensive suite of cURL-inspired request and response helpers. Key additions include support for Bearer token authentication, configurable retry logic with exponential backoff, advanced TLS/SSL settings, cookie persistence, and explicit HTTP/2 and HTTP/3 selection. The changes also include bug fixes for NTLM authentication and retry timing, alongside extensive type-hinting improvements across the codebase. I have no feedback to provide as there are no review comments to assess.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Httpful/Client.php (1)
20-20:⚠️ Potential issue | 🟠 MajorReplace implicit nullable
arrayparams with explicit?array.Lines 20, 32, 63, 74, 85, 96, 108, and 119 use the deprecated pattern
array $param = null. PHP 8.4 requires explicit nullable types; switch to?array $param = null.Example fix pattern
- public static function get_form(string $uri, array $param = null): array + public static function get_form(string $uri, ?array $param = null): arrayApply the same pattern across:
delete,delete_request,get,get_dom,get_form,get_json,get_request, andget_xml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Httpful/Client.php` at line 20, Update the method signatures that currently use the deprecated pattern "array $param = null" to explicit nullable arrays by changing each parameter type to "?array $param = null"; specifically update signatures for delete, delete_request, get, get_dom, get_form, get_json, get_request, and get_xml so any parameter declared as "array $params = null" (or similar) becomes "?array $params = null" to satisfy PHP 8.4 nullable typing requirements and avoid deprecation warnings.
🧹 Nitpick comments (1)
phpstan.neon (1)
9-9: Use explicit empty list forignoreErrors.Replace the bare
ignoreErrors:key (currently on line 8) withignoreErrors: []for explicit clarity. This prevents config interpretation ambiguity and aligns with PHPStan 2.x best practices.Proposed change
- ignoreErrors: + ignoreErrors: []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpstan.neon` at line 9, The phpstan.neon config currently contains a bare ignoreErrors: key; replace that key with an explicit empty list by changing ignoreErrors: to ignoreErrors: [] so the configuration explicitly specifies no ignore rules and avoids PHPStan 2.x ambiguity—update the phpstan.neon file's ignoreErrors entry accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 22: Fix the inconsistent list nesting under the "Features" section by
normalizing indentation for all bullets to a consistent number of spaces per
nesting level (e.g., two spaces per indent); locate the top-level "Features"
heading and adjust the nested list items including "Client Side Certificate Auth
(SSL)" and the other multi-line feature bullets so every nested bullet uses the
same indentation style and alignment to resolve MD005.
In `@src/Httpful/Handlers/XmlMimeHandler.php`:
- Line 121: Array keys and reflected property names are being passed directly
into DOMDocument::createElement via _future_createElement which can throw on
invalid XML names; add a private sanitizer (e.g. _future_normalizeElementName)
that replaces characters not allowed in XML names with underscores and ensures
the resulting name begins with a valid start character (letter, underscore or
colon), prefixing with "node_" if necessary, and call this sanitizer whenever
you pass a user-derived name into _future_createElement (both at the array-key
path where _future_createElement is invoked and the reflection property-name
path) so createElement always receives a valid XML element name.
In `@src/Httpful/Request.php`:
- Around line 3150-3166: The retry decider is currently attached for any request
with $this->retry > 0, which can replay non-idempotent methods; update
_configureRetryBehavior() to only enable automatic retries for idempotent HTTP
methods (e.g. GET, HEAD, PUT, DELETE, OPTIONS, TRACE) by checking $this->method
(normalize to uppercase) before calling
$curl->setRetry($this->_createRetryDecider()), and call $curl->setRetry(0) for
non-idempotent methods unless an explicit opt-in is present (e.g. a new
$this->unsafeRetry flag or an idempotency token header) so POST/PATCH/etc. are
not retried by default.
- Around line 320-324: initialize() can throw a LogicException from the Curl
handle guard so _curlPrep() should catch that and rethrow a
NetworkErrorException instead; wrap the call to initialize() in _curlPrep() (or
the relevant method in Request.php) in a try/catch that catches LogicException
(and optionally Throwable/Exception) and throws a NetworkErrorException with the
original exception as the previous/inner exception so callers see a consistent
NetworkErrorException on cURL init failures (refer to initialize(), _curlPrep(),
and the handle guard in Curl::... in src/Httpful/Curl/Curl.php).
In `@src/Httpful/Response.php`:
- Around line 960-977: The code currently sets $this->content_type from
$content_type_parts[0] but only reads a charset when count($content_type_parts)
=== 2; change this to scan all parameter parts after the media type for a
"charset=" parameter: after assigning $this->content_type =
trim($content_type_parts[0]) iterate the remaining $content_type_parts (starting
at index 1), for each string part trim it, split it into key/value with
explode('=', $part, 2), and if strtolower(trim($key)) === 'charset' set
$this->charset to the trimmed value (stripping surrounding quotes) and stop;
keep using the existing symbols $content_type_parts, $this->content_type, and
$this->charset.
---
Outside diff comments:
In `@src/Httpful/Client.php`:
- Line 20: Update the method signatures that currently use the deprecated
pattern "array $param = null" to explicit nullable arrays by changing each
parameter type to "?array $param = null"; specifically update signatures for
delete, delete_request, get, get_dom, get_form, get_json, get_request, and
get_xml so any parameter declared as "array $params = null" (or similar) becomes
"?array $params = null" to satisfy PHP 8.4 nullable typing requirements and
avoid deprecation warnings.
---
Nitpick comments:
In `@phpstan.neon`:
- Line 9: The phpstan.neon config currently contains a bare ignoreErrors: key;
replace that key with an explicit empty list by changing ignoreErrors: to
ignoreErrors: [] so the configuration explicitly specifies no ignore rules and
avoids PHPStan 2.x ambiguity—update the phpstan.neon file's ignoreErrors entry
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1ebce4e-3815-42d1-8fbc-f3cac56b5784
📒 Files selected for processing (25)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mdcomposer.jsonphpstan.neonsrc/Httpful/Client.phpsrc/Httpful/ClientMulti.phpsrc/Httpful/Curl/Curl.phpsrc/Httpful/Curl/MultiCurl.phpsrc/Httpful/Factory.phpsrc/Httpful/Handlers/CsvMimeHandler.phpsrc/Httpful/Handlers/DefaultMimeHandler.phpsrc/Httpful/Handlers/FormMimeHandler.phpsrc/Httpful/Handlers/JsonMimeHandler.phpsrc/Httpful/Handlers/XmlMimeHandler.phpsrc/Httpful/Headers.phpsrc/Httpful/Http.phpsrc/Httpful/Request.phpsrc/Httpful/Response.phpsrc/Httpful/ServerRequest.phpsrc/Httpful/Stream.phpsrc/Httpful/UploadedFile.phpsrc/Httpful/Uri.phptests/Httpful/CurlFeatureApiTest.phptests/Httpful/ExtraCoverageTest.php
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
| } | ||
|
|
||
| $el = $dom->createElement($n); | ||
| $el = $this->_future_createElement($dom, (string) $n); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP DOMDocument::createElement valid element-name rules; what happens with invalid characters and namespaced class strings like "App\Model\User"?
💡 Result:
PHP DOMDocument::createElement requires the element name (localName) to follow XML Name production rules from the XML specification (w3.org/TR/xml/#NT-Name). Valid rules: - Starts with NameStartChar: letters (a-zA-Z), underscore (_), colon (:), or specific Unicode characters. - Subsequent characters: NameChar, which includes NameStartChar plus digits (0-9), hyphen (-), period (.), middle dot (·), combining characters, etc. - Cannot start with "xml" in certain cases (reserved), but DOM allows it. - No spaces, control characters, or invalid Unicode. Invalid characters trigger DOM_INVALID_CHARACTER_ERR: throws DOMException (e.g., spaces cause "Invalid Character Error"). Special cases: - NUL byte (\0): No exception, but truncates the name at NUL. - Backslash (): Valid NameChar per XML spec (Unicode category Pc or similar), so "App\Model\User" is valid. Creates element with tagName "App\Model\User" (double backslashes preserved as literal characters). Not a namespace (use createElementNS for xmlns:prefix). Results confirmed via PHP manual user notes and StackOverflow tests matching XML Name rules. For namespaced elements like "App:Model:User", use createElementNS(null, "App:Model:User") or define proper namespace/prefix.
Citations:
- 1: https://www.php.net/manual/en/domdocument.createelement.php
- 2: https://typeerror.org/docs/php/domdocument.createelement
- 3: https://stackoverflow.com/questions/3957218/php-dom-createelement-allow-spaces
- 4: https://stackoverflow.com/questions/2519845/how-to-check-if-string-is-a-valid-xml-element-name
- 5: https://stackoverflow.com/questions/27257283/php-create-element-attribute-with-an-associated-namespace
- 6: https://www.php.net/manual/en/domdocument.createelementns.php
🏁 Script executed:
# Examine the file and look at the specific lines mentioned
wc -l src/Httpful/Handlers/XmlMimeHandler.phpRepository: voku/httpful
Length of output: 100
🏁 Script executed:
# Read the relevant sections around lines 121, 155, 197
sed -n '115,130p' src/Httpful/Handlers/XmlMimeHandler.phpRepository: voku/httpful
Length of output: 463
🏁 Script executed:
# Check line 155
sed -n '150,165p' src/Httpful/Handlers/XmlMimeHandler.phpRepository: voku/httpful
Length of output: 817
🏁 Script executed:
# Check line 197 and surrounding context
sed -n '190,205p' src/Httpful/Handlers/XmlMimeHandler.phpRepository: voku/httpful
Length of output: 655
🏁 Script executed:
# Also check the helper method _future_createElement
sed -n '171,185p' src/Httpful/Handlers/XmlMimeHandler.phpRepository: voku/httpful
Length of output: 407
Validate array keys and property names before using as XML element names.
Array keys (Line 121) and reflection property names (Line 197) can contain characters invalid for XML element names (spaces, control characters), causing DOMDocument::createElement() to throw exceptions. While class names with namespace separators (Line 155) are technically valid per the XML specification, arbitrary user-derived names require validation.
Consider normalizing keys and property names using a sanitization function that replaces invalid characters and ensures the name starts with a valid XML name character. For example:
Suggested implementation
private function _future_normalizeElementName(string $name): string
{
// Replace invalid characters with underscores
$normalized = \preg_replace('/[^A-Za-z0-9_:.-]+/', '_', $name) ?? '';
// Ensure name starts with valid character (letter, underscore, or colon)
if ($normalized === '' || \preg_match('/^[^A-Za-z_:]/', $normalized) === 1) {
$normalized = 'node_' . $normalized;
}
return $normalized;
}Apply this at Lines 121 and 197 where arbitrary user data is passed as element names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Httpful/Handlers/XmlMimeHandler.php` at line 121, Array keys and
reflected property names are being passed directly into
DOMDocument::createElement via _future_createElement which can throw on invalid
XML names; add a private sanitizer (e.g. _future_normalizeElementName) that
replaces characters not allowed in XML names with underscores and ensures the
resulting name begins with a valid start character (letter, underscore or
colon), prefixing with "node_" if necessary, and call this sanitizer whenever
you pass a user-derived name into _future_createElement (both at the array-key
path where _future_createElement is invoked and the reflection property-name
path) so createElement always receives a valid XML element name.
| $this->initialize(); | ||
| \assert($this->curl instanceof Curl); | ||
| $curl = $this->curl; | ||
| if ($curl === null) { | ||
| throw new NetworkErrorException('Unable to initialize cURL.'); | ||
| } |
There was a problem hiding this comment.
Wrap cURL init failures here; the null check never catches them.
initialize() either assigns a Curl instance or throws first. With the new handle guard in src/Httpful/Curl/Curl.php, a failed curl_init() now bubbles out as a LogicException, so _curlPrep() no longer consistently throws NetworkErrorException on initialization failure.
Suggested fix
- $this->initialize();
- $curl = $this->curl;
- if ($curl === null) {
- throw new NetworkErrorException('Unable to initialize cURL.');
- }
+ try {
+ $this->initialize();
+ } catch (\LogicException $e) {
+ throw new NetworkErrorException('Unable to initialize cURL.', 0, $e);
+ }
+
+ $curl = $this->curl;
+ if ($curl === null) {
+ throw new NetworkErrorException('Unable to initialize cURL.');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Httpful/Request.php` around lines 320 - 324, initialize() can throw a
LogicException from the Curl handle guard so _curlPrep() should catch that and
rethrow a NetworkErrorException instead; wrap the call to initialize() in
_curlPrep() (or the relevant method in Request.php) in a try/catch that catches
LogicException (and optionally Throwable/Exception) and throws a
NetworkErrorException with the original exception as the previous/inner
exception so callers see a consistent NetworkErrorException on cURL init
failures (refer to initialize(), _curlPrep(), and the handle guard in Curl::...
in src/Httpful/Curl/Curl.php).
| private function _configureRetryBehavior(): void | ||
| { | ||
| $curl = $this->curl; | ||
| if ($curl === null) { | ||
| throw new \LogicException('cURL is not initialized.'); | ||
| } | ||
|
|
||
| $curl->attempts = 0; | ||
| $curl->retries = 0; | ||
|
|
||
| if ($this->retry <= 0) { | ||
| $curl->setRetry(0); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $curl->setRetry($this->_createRetryDecider()); |
There was a problem hiding this comment.
Don't enable automatic retries for non-idempotent methods by default.
This attaches the retry decider for any request with retry > 0, including POST and PATCH. A timeout or 5xx does not mean the first attempt was not applied, so this can replay writes and duplicate side effects. Please gate automatic retries to idempotent methods by default, or require an explicit unsafe-retry opt-in/idempotency mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Httpful/Request.php` around lines 3150 - 3166, The retry decider is
currently attached for any request with $this->retry > 0, which can replay
non-idempotent methods; update _configureRetryBehavior() to only enable
automatic retries for idempotent HTTP methods (e.g. GET, HEAD, PUT, DELETE,
OPTIONS, TRACE) by checking $this->method (normalize to uppercase) before
calling $curl->setRetry($this->_createRetryDecider()), and call
$curl->setRetry(0) for non-idempotent methods unless an explicit opt-in is
present (e.g. a new $this->unsafeRetry flag or an idempotency token header) so
POST/PATCH/etc. are not retried by default.
Agent-Logs-Url: https://github.com/voku/httpful/sessions/63d2ba89-588c-4084-b0b7-0c8f5e983f05 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Completing task
|
coverage: 90.483% (+0.6%) from 89.902% — php8 into master |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the library for PHP 8.0+ while expanding the cURL-driven feature set (HTTP/2+HTTP/3 selection, retries, TLS/cookie/proxy helpers) and tightening typing/static analysis.
Changes:
- Raise minimum PHP version to 8.0, update CI matrix and dev tooling (PHPStan 2.x, PHPUnit 9.6).
- Add new Request/Response features around auth, retries/backoff, HTTP version selection, and transfer metadata helpers.
- Add/adjust tests and type annotations to validate new behavior and keep static analysis clean.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Httpful/ExtraCoverageTest.php | Updates NTLM regression test to assert CURLOPT_HTTPAUTH is preserved. |
| tests/Httpful/CurlFeatureApiTest.php | Adds API-level tests for new cURL feature helpers (retry, TLS, cookies, HTTP versions, transfer info). |
| src/Httpful/Uri.php | Tightens typing and phpdoc generics; constructor now requires string. |
| src/Httpful/UploadedFile.php | Updates phpdoc types for PHP 8+ and iterable value types. |
| src/Httpful/Stream.php | Adds phpstan annotations and improves metadata typing. |
| src/Httpful/Setup.php | Tightens setupGlobalMimeType typing and null-handling. |
| src/Httpful/ServerRequest.php | Improves phpdoc generics and constructor param typing. |
| src/Httpful/Response.php | Adds transfer-info helper APIs; hardens header parsing and callback invocation. |
| src/Httpful/Request.php | Adds many new request helpers (bearer, retry, TLS, cookies, proxy, HTTP versions) and refactors cURL prep. |
| src/Httpful/Http.php | Adds HTTP/3 constant and improves return type annotations. |
| src/Httpful/Headers.php | Improves typing, iterator key handling, and value normalization. |
| src/Httpful/Handlers/XmlMimeHandler.php | Hardens DOM element creation and tightens return types. |
| src/Httpful/Handlers/JsonMimeHandler.php | Tightens configuration array typing. |
| src/Httpful/Handlers/FormMimeHandler.php | Tightens parse return type annotation. |
| src/Httpful/Handlers/DefaultMimeHandler.php | Tightens constructor/config typing. |
| src/Httpful/Handlers/CsvMimeHandler.php | Tightens parse return type annotation. |
| src/Httpful/Factory.php | Tightens nullable param typing for PSR-17 factory methods. |
| src/Httpful/Exception/RequestException.php | Adds nullable previous exception typing. |
| src/Httpful/Exception/NetworkErrorException.php | Adds nullable argument typing for PHP 8+. |
| src/Httpful/Exception/ClientErrorException.php | Adds nullable previous exception typing. |
| src/Httpful/Curl/MultiCurlPromise.php | Makes callbacks nullable in then() signature. |
| src/Httpful/Curl/MultiCurl.php | Switches to typed \CurlMultiHandle, refactors lifecycle/handle access. |
| src/Httpful/Curl/Curl.php | Switches to typed \CurlHandle, refactors handle access and retry state. |
| src/Httpful/ClientMulti.php | Updates param typing annotations for PHP 8+ collections. |
| src/Httpful/Client.php | Updates param typing annotations and return typing for parsed helpers. |
| phpstan.neon | Removes ignore rules and tightens analysis configuration. |
| composer.json | Bumps minimum PHP to ^8.0; updates phpstan/phpunit dev requirements. |
| README.md | Documents new features and adds example usage for new helpers. |
| CHANGELOG.md | Adds unreleased notes for PHP 8+ rework and new features/fixes. |
| .github/workflows/ci.yml | Drops PHP 7.4 from CI and makes PHPStan required on latest matrix entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return isset($this->parse_callback) | ||
| && | ||
| \is_callable($this->parse_callback); | ||
| return $this->parse_callback !== null; |
There was a problem hiding this comment.
hasParseCallback() previously ensured the callback was callable; now it returns true for any non-null value, even if it’s a non-callable string. This can lead to a fatal error when invoking the callback (e.g., in Response parsing). Fix by restoring a callability check (or ensuring getParseCallback() always returns a callable when non-null), e.g. require is_callable($this->parse_callback) in addition to non-null.
| return $this->parse_callback !== null; | |
| return $this->parse_callback !== null && \is_callable($this->parse_callback); |
| public function withAddedHeader($name, $value): MessageInterface | ||
| { | ||
| if (!\is_string($name) || $name === '') { | ||
| if ($name === '') { |
There was a problem hiding this comment.
The validation no longer rejects non-string header names (e.g., int/array). PSR-7 requires a string header name, and downstream normalization typically assumes a string. Restore the is_string($name) check (and keep the empty-string check) so invalid types consistently throw InvalidArgumentException instead of causing type errors later.
| if ($name === '') { | |
| if (!\is_string($name) || $name === '') { |
| // fallback | ||
| if (!isset($this->template)) { | ||
| $this->template = new static(Http::GET, null, $this); | ||
| $this->template = new self(Http::GET, null, $this); |
There was a problem hiding this comment.
Using new self(...) here breaks late-static-binding for subclasses: a subclassed Request will now end up with a base Request template instead of a template of the subclass, which can change fluent behavior/overrides. Prefer new static(...) (or otherwise preserve subclass type) while keeping PHPStan happy, e.g. via appropriate phpdoc assertions.
| $this->template = new self(Http::GET, null, $this); | |
| $this->template = new static(Http::GET, null, $this); |
| $this->content_type = $content_type_parts[0] ?? ''; | ||
| if ( | ||
| \count($content_type) === 2 | ||
| \count($content_type_parts) === 2 | ||
| && | ||
| \strpos($content_type[1], '=') !== false | ||
| \strpos($content_type_parts[1], '=') !== false | ||
| ) { | ||
| /** @noinspection PhpUnusedLocalVariableInspection */ | ||
| list($nill, $this->charset) = \explode('=', $content_type[1]); | ||
| list($nill, $this->charset) = \explode('=', $content_type_parts[1]); | ||
| } |
There was a problem hiding this comment.
Charset extraction is now limited to exactly two ;-separated parts. For common headers like text/html; charset=UTF-8; boundary=..., this will no longer populate $this->charset. Instead of checking count(...) === 2, scan all parts after index 0 for a charset= parameter and parse it when present.
| - Client Side Certificate Auth (SSL) | ||
| - Retry Configuration (count, delay, max time, all-errors, connection-refused) | ||
| - Advanced TLS Configuration (CA bundle / path, pinned public key, TLS version) | ||
| - Cookie Persistence (cookie file / cookie jar) | ||
| - Modern HTTP Version Helpers (HTTP/2 prior knowledge, HTTP/3, HTTP/3 only) | ||
| - Alt-Svc / HSTS Cache Helpers | ||
| - Proxy / Routing Helpers (no-proxy, proxy tunnel, resolve, connect-to) | ||
| - Request "Download" | ||
| - Request "Templates" | ||
| - Parallel Request (via curl_multi) | ||
| - Transfer Metadata Helpers | ||
| - PSR-3: Logger Interface | ||
| - PSR-7: HTTP Message Interface | ||
| - PSR-17: HTTP Factory Interface | ||
| - PSR-18: HTTP Client Interface |
There was a problem hiding this comment.
Several list items are misindented (extra leading space before -), which renders them as nested bullets and makes the feature list harder to read. Align indentation consistently so each feature is at the intended list level.
| - Client Side Certificate Auth (SSL) | |
| - Retry Configuration (count, delay, max time, all-errors, connection-refused) | |
| - Advanced TLS Configuration (CA bundle / path, pinned public key, TLS version) | |
| - Cookie Persistence (cookie file / cookie jar) | |
| - Modern HTTP Version Helpers (HTTP/2 prior knowledge, HTTP/3, HTTP/3 only) | |
| - Alt-Svc / HSTS Cache Helpers | |
| - Proxy / Routing Helpers (no-proxy, proxy tunnel, resolve, connect-to) | |
| - Request "Download" | |
| - Request "Templates" | |
| - Parallel Request (via curl_multi) | |
| - Transfer Metadata Helpers | |
| - PSR-3: Logger Interface | |
| - PSR-7: HTTP Message Interface | |
| - PSR-17: HTTP Factory Interface | |
| - PSR-18: HTTP Client Interface | |
| - Client Side Certificate Auth (SSL) | |
| - Retry Configuration (count, delay, max time, all-errors, connection-refused) | |
| - Advanced TLS Configuration (CA bundle / path, pinned public key, TLS version) | |
| - Cookie Persistence (cookie file / cookie jar) | |
| - Modern HTTP Version Helpers (HTTP/2 prior knowledge, HTTP/3, HTTP/3 only) | |
| - Alt-Svc / HSTS Cache Helpers | |
| - Proxy / Routing Helpers (no-proxy, proxy tunnel, resolve, connect-to) | |
| - Request "Download" | |
| - Request "Templates" | |
| - Parallel Request (via curl_multi) | |
| - Transfer Metadata Helpers | |
| - PSR-3: Logger Interface | |
| - PSR-7: HTTP Message Interface | |
| - PSR-17: HTTP Factory Interface | |
| - PSR-18: HTTP Client Interface |
|
@copilot apply changes based on the comments in this thread |
…ack, charset, README) Agent-Logs-Url: https://github.com/voku/httpful/sessions/9399ebc4-8b6c-4d3f-81bc-2183620fff6e Co-authored-by: voku <264695+voku@users.noreply.github.com>
Applied all five review suggestions in commit
|
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/Httpful/Headers.php (1)
12-15: ⚡ Quick winAlign internal phpdocs with the new generic contract.
The class-level generics are now precise, but internal docs still use broad types (e.g.,
$dataasmixed[]), which weakens static analysis and makes the unguarded inner iteration intoArray()harder to verify.Proposed docblock tightening
- /** - * `@var` mixed[] data storage with lowercase keys + /** + * `@var` array<string, array<int, string>> data storage with lowercase keys * * `@see` offsetSet() * `@see` offsetExists() * `@see` offsetUnset() * `@see` offsetGet() * `@see` count() * `@see` current() * `@see` next() * `@see` key() */ private $data = []; - /** - * `@var` string[] case-sensitive keys + /** + * `@var` array<string, string> case-sensitive keys * * `@see` offsetSet() * `@see` offsetUnset() * `@see` key() */ private $keys = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Httpful/Headers.php` around lines 12 - 15, The class-level phpdoc generics were tightened but internal docs (e.g., the $data property and related internal types) still use broad types like mixed[], making static analysis and the unguarded loop inside toArray() unreliable; update the docblocks for the Headers class to reflect the precise generic types (ArrayAccess<string, array<int, string>> / Iterator<string, array<int, string>>), tighten the $data property docblock to array<string, array<int, string>> (or the equivalent precise type used elsewhere), and adjust the toArray() method/docblock to indicate it iterates over array<string, array<int, string>> so static analyzers can verify the inner iteration and prevent unguarded mixed usage (refer to the Headers class, $data property, and toArray() method).src/Httpful/Curl/MultiCurlPromise.php (1)
40-54: 💤 Low valuePHPDoc doesn't reflect nullable callable parameters.
The signature now uses
?callablebut PHPDoc at Lines 46-47 still shows@param callable. Consider updating:- * `@param` callable $onComplete Called when a response will be available - * `@param` callable $onRejected Called when an error happens. + * `@param` callable|null $onComplete Called when a response will be available + * `@param` callable|null $onRejected Called when an error happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Httpful/Curl/MultiCurlPromise.php` around lines 40 - 54, Update the PHPDoc for the MultiCurlPromise::then method to reflect the nullable callable types: change the two `@param` annotations for $onComplete and $onRejected from "@param callable" to "@param callable|null" (and you can also update the textual description to mention null is allowed) so the docblock matches the ?callable signature in then(?callable $onComplete = null, ?callable $onRejected = null).src/Httpful/Setup.php (1)
147-158: 💤 Low valuePHPDoc doesn't reflect the nullable parameter.
The method signature now accepts
?string $mimeType, but the PHPDoc at Line 148 still declares@param string $mimeType. Update for consistency:/** - * `@param` string $mimeType + * `@param` string|null $mimeType * * `@return` MimeHandlerInterface */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Httpful/Setup.php` around lines 147 - 158, Update the PHPDoc for Setup::setupGlobalMimeType to reflect the nullable parameter: change the `@param` annotation from "@param string $mimeType" to a nullable type such as "@param string|null $mimeType" (or "@param ?string $mimeType") so it matches the method signature; verify the docblock near the call to self::initMimeHandlers() and references to self::$mime_registrar remain unchanged.src/Httpful/Request.php (1)
1656-1659: ⚡ Quick winRedundant
is_callablecheck.Static analysis correctly identifies that if
$this->parse_callback !== null, thenis_callable($this->parse_callback)is always true since the property is typedcallable|null. The second condition can be removed:public function hasParseCallback(): bool { - return $this->parse_callback !== null && \is_callable($this->parse_callback); + return $this->parse_callback !== null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Httpful/Request.php` around lines 1656 - 1659, The hasParseCallback() method includes a redundant is_callable() check because the parse_callback property is typed callable|null; replace the body to simply return whether parse_callback is non-null (e.g., return $this->parse_callback !== null;) so static analyzers stop flagging it; keep the method name hasParseCallback and the property parse_callback unchanged.src/Httpful/Client.php (1)
59-60: Add?stringtype hint to$mimeparameter inClient::get()andClient::get_request().The PHPDoc comments document
string|nullfor the$mimeparameter at lines 60 and 104-105, but the function signatures at lines 63 and 108 leave it untyped. Adding?stringtype hints would make the API consistent with its documentation and theRequest::get()implementation, which already uses?string $mime = null. The default valueMime::PLAINis a string constant, making it fully compatible with the type hint.Proposed diff
- public static function get(string $uri, ?array $params = null, $mime = Mime::PLAIN): Response + public static function get(string $uri, ?array $params = null, ?string $mime = Mime::PLAIN): Response @@ - public static function get_request(string $uri, ?array $param = null, $mime = Mime::PLAIN): Request + public static function get_request(string $uri, ?array $param = null, ?string $mime = Mime::PLAIN): Request🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Httpful/Client.php` around lines 59 - 60, The Client::get() and Client::get_request() signatures lack the nullable string type hint for the $mime parameter despite PHPDoc and Request::get() documenting ?string $mime = null; update both method declarations (Client::get and Client::get_request) to declare ?string $mime = Mime::PLAIN (or ?string $mime = null if preferred) so the runtime signature matches the docs and Request::get(); ensure no other callers break and run tests/type checks after changing the signatures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Httpful/Client.php`:
- Around line 59-60: The Client::get() and Client::get_request() signatures lack
the nullable string type hint for the $mime parameter despite PHPDoc and
Request::get() documenting ?string $mime = null; update both method declarations
(Client::get and Client::get_request) to declare ?string $mime = Mime::PLAIN (or
?string $mime = null if preferred) so the runtime signature matches the docs and
Request::get(); ensure no other callers break and run tests/type checks after
changing the signatures.
In `@src/Httpful/Curl/MultiCurlPromise.php`:
- Around line 40-54: Update the PHPDoc for the MultiCurlPromise::then method to
reflect the nullable callable types: change the two `@param` annotations for
$onComplete and $onRejected from "@param callable" to "@param callable|null"
(and you can also update the textual description to mention null is allowed) so
the docblock matches the ?callable signature in then(?callable $onComplete =
null, ?callable $onRejected = null).
In `@src/Httpful/Headers.php`:
- Around line 12-15: The class-level phpdoc generics were tightened but internal
docs (e.g., the $data property and related internal types) still use broad types
like mixed[], making static analysis and the unguarded loop inside toArray()
unreliable; update the docblocks for the Headers class to reflect the precise
generic types (ArrayAccess<string, array<int, string>> / Iterator<string,
array<int, string>>), tighten the $data property docblock to array<string,
array<int, string>> (or the equivalent precise type used elsewhere), and adjust
the toArray() method/docblock to indicate it iterates over array<string,
array<int, string>> so static analyzers can verify the inner iteration and
prevent unguarded mixed usage (refer to the Headers class, $data property, and
toArray() method).
In `@src/Httpful/Request.php`:
- Around line 1656-1659: The hasParseCallback() method includes a redundant
is_callable() check because the parse_callback property is typed callable|null;
replace the body to simply return whether parse_callback is non-null (e.g.,
return $this->parse_callback !== null;) so static analyzers stop flagging it;
keep the method name hasParseCallback and the property parse_callback unchanged.
In `@src/Httpful/Setup.php`:
- Around line 147-158: Update the PHPDoc for Setup::setupGlobalMimeType to
reflect the nullable parameter: change the `@param` annotation from "@param string
$mimeType" to a nullable type such as "@param string|null $mimeType" (or "@param
?string $mimeType") so it matches the method signature; verify the docblock near
the call to self::initMimeHandlers() and references to self::$mime_registrar
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92678ccd-da95-488f-98ef-8e633042f51c
📒 Files selected for processing (16)
README.mdsrc/Httpful/Client.phpsrc/Httpful/ClientMulti.phpsrc/Httpful/Curl/MultiCurl.phpsrc/Httpful/Curl/MultiCurlPromise.phpsrc/Httpful/Exception/ClientErrorException.phpsrc/Httpful/Exception/NetworkErrorException.phpsrc/Httpful/Exception/RequestException.phpsrc/Httpful/Factory.phpsrc/Httpful/Handlers/XmlMimeHandler.phpsrc/Httpful/Headers.phpsrc/Httpful/Request.phpsrc/Httpful/Response.phpsrc/Httpful/ServerRequest.phpsrc/Httpful/Setup.phpsrc/Httpful/Uri.php
✅ Files skipped from review due to trivial changes (1)
- src/Httpful/Exception/ClientErrorException.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Httpful/Factory.php
- src/Httpful/Response.php
- src/Httpful/ClientMulti.php
- src/Httpful/ServerRequest.php
Agent-Logs-Url: https://github.com/voku/httpful/sessions/1a96e2f8-8c99-482b-833a-e02e774b09e5 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/httpful/sessions/e1af6f48-c4a7-4c8e-baa9-fe8e4fa18e3c Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/httpful/sessions/e1af6f48-c4a7-4c8e-baa9-fe8e4fa18e3c Co-authored-by: voku <264695+voku@users.noreply.github.com>
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes