RFC 7234 HTTP Caching Support #68
Merged
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
✅ Deploy Preview for fetch-php canceled.
|
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds the github-protocols configuration to composer.json, which restricts Composer to only use HTTPS protocols when interacting with GitHub repositories. While the PR title suggests this is about RFC 7234 HTTP caching support, the current changes only include a composer.json configuration update.
Key Changes
- Added
"github-protocols": ["https"]configuration to the composer config section
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
…ed config Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add RFC 7234 compliant HTTP caching support
RFC 7234 HTTP Caching Support
Nov 28, 2025
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
74
to
81
| // Handle negative TTL (already expired) | ||
| if ($ttl < 0) { | ||
| $expiresAt = time() + $ttl; // Will be in the past | ||
| } elseif ($ttl > 0) { | ||
| $expiresAt = time() + $ttl; | ||
| } else { | ||
| $expiresAt = null; // TTL of 0 means no expiration | ||
| } |
There was a problem hiding this comment.
The comment states "TTL of 0 means no expiration" but this behavior is inconsistent with the CachedResponse's own expiration logic. When a TTL of 0 is passed and expiresAt is set to null, the CachedResponse will never expire. However, in FileCache at line 116-121, when expiresAt is null AND defaultTtl > 0, it sets a new expiration. This creates inconsistent behavior between MemoryCache and FileCache when TTL is 0. Consider either:
- Always setting expiration based on defaultTtl when ttl=0, or
- Documenting that ttl=0 means "use no expiration" and ttl=null means "use default"
Contributor
Author
There was a problem hiding this comment.
Fixed in commit da5c143. Made TTL behavior consistent between MemoryCache and FileCache: null uses default TTL, 0 means no expiration, and negative values mean already expired. Also documented this behavior in the CacheInterface.
src/Fetch/Cache/FileCache.php
Outdated
Comment on lines
110
to
127
| if ($ttl !== null) { | ||
| $response = CachedResponse::fromArray( | ||
| array_merge($response->toArray(), [ | ||
| 'expires_at' => time() + $ttl, | ||
| ]) | ||
| ); | ||
| } elseif ($response->getExpiresAt() === null && $this->defaultTtl > 0) { | ||
| $response = CachedResponse::fromArray( | ||
| array_merge($response->toArray(), [ | ||
| 'expires_at' => time() + $this->defaultTtl, | ||
| ]) | ||
| ); | ||
| } | ||
|
|
||
| // PHPStan cannot infer that $response is non-null here after the conditionals above | ||
| // but we know $response is always a valid CachedResponse at this point | ||
| // @phpstan-ignore-next-line | ||
| $serialized = serialize($response->toArray()); |
There was a problem hiding this comment.
Bug: The fromArray() method can return null, but this is not handled properly. If fromArray() returns null, the code attempts to call toArray() on a null value at line 127, which will cause a fatal error. The condition at lines 110-122 reassigns $response but doesn't check if the result is null before using it.
src/Fetch/Cache/CacheControl.php
Outdated
Comment on lines
93
to
94
| $status = $response->getStatusCode(); | ||
| $cacheableStatuses = [200, 203, 204, 206, 300, 301, 404, 405, 410, 414, 501]; |
There was a problem hiding this comment.
The hardcoded list of cacheable status codes in shouldCache() doesn't match the configurable list in ManagesCache::$cacheOptions['cache_status_codes'] (line 41 of ManagesCache.php). The CacheControl default list includes 405, 414, and 501 which are not in the ManagesCache default. This creates inconsistent behavior - a response might pass the CacheControl::shouldCache() check but fail the isCacheableStatusCode() check in ManagesCache, or vice versa. Consider either making this list configurable or ensuring both lists match.
Contributor
Author
There was a problem hiding this comment.
Fixed in commit acb60ba. Aligned the status codes between CacheControl and ManagesCache - both now use [200, 203, 204, 206, 300, 301, 404, 410].
Comment on lines
85
to
99
| public function test_cache_miss_adds_x_cache_status_header(): void | ||
| { | ||
| $responses = [ | ||
| new GuzzleResponse(200, ['Content-Type' => 'application/json'], '{"data":"test"}'), | ||
| ]; | ||
|
|
||
| $handler = $this->create_handler_with_mock_responses($responses); | ||
| $handler->baseUri('https://api.example.com'); | ||
| $handler->withCache(); | ||
|
|
||
| $response = $handler->get('/users'); | ||
|
|
||
| // After request is cached, the response won't have the header until fetched from cache | ||
| $this->assertEquals(200, $response->getStatusCode()); | ||
| } |
There was a problem hiding this comment.
This test doesn't actually verify anything meaningful. The test name suggests it should check for an "X-Cache-Status" header on a cache miss, but the test only checks that the status code is 200. The comment at line 97 acknowledges this limitation. Either update the test to actually verify the expected behavior (e.g., that there's no X-Cache-Status header, or that it's "MISS" if your implementation adds it), or rename the test to reflect what it actually tests.
Contributor
Author
There was a problem hiding this comment.
Fixed in commit acb60ba. Renamed the test to better reflect what it actually tests and added a new test for no-cache directive behavior.
Owner
|
@copilot please fix code style and static analysis issues to get CI to pass. |
Copilot stopped work on behalf of
Thavarshan due to an error
November 28, 2025 17:55
Owner
|
@copilot please fix code style and static analysis issues to get CI to pass. |
…PHPDoc tags Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Contributor
Author
Fixed code style issues in commit 914eeb6:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return null; | ||
| } | ||
|
|
||
| $data = json_decode($contents, true); |
There was a problem hiding this comment.
The unserialize() function is being used without the allowed_classes option, which could pose a security risk. When unserializing data from cache files (which could potentially be tampered with), it's recommended to restrict which classes can be instantiated.
Consider using:
$data = @unserialize($contents, ['allowed_classes' => [CachedResponse::class]]);Or since you're unserializing arrays:
$data = @unserialize($contents, ['allowed_classes' => false]);
Contributor
Author
There was a problem hiding this comment.
FileCache already uses JSON serialization (json_decode/json_encode), not PHP's unserialize(). This was addressed in an earlier commit (da5c143) when we switched from PHP serialization to JSON for security.
src/Fetch/Cache/FileCache.php
Outdated
| continue; | ||
| } | ||
|
|
||
| $data = @unserialize($contents); |
There was a problem hiding this comment.
Same security concern as above - unserialize() should use the allowed_classes option to prevent object injection attacks. Consider:
$data = @unserialize($contents, ['allowed_classes' => false]);
Suggested change
| $data = @unserialize($contents); | |
| $data = @unserialize($contents, ['allowed_classes' => false]); |
Contributor
Author
There was a problem hiding this comment.
FileCache already uses JSON serialization. The prune() method uses json_decode(), not unserialize(). This was addressed in commit da5c143.
src/Fetch/Cache/FileCache.php
Outdated
Comment on lines
56
to
59
| if (! file_exists($path)) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
There's a race condition here: the code checks if the file exists (line 56) and then reads it (line 60), but the file could be deleted between these operations (e.g., by another process pruning expired entries). While the error is suppressed and handled by returning null (line 62), this could lead to inconsistent behavior.
Consider using @file_get_contents() directly without the file_exists() check, as file_get_contents() will return false if the file doesn't exist anyway.
Suggested change
| if (! file_exists($path)) { | |
| return null; | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in commit acb60ba. Removed the file_exists() check - now using @file_get_contents() directly which returns false if the file doesn't exist, avoiding the race condition.
src/Fetch/Cache/CacheControl.php
Outdated
|
|
||
| // Check response status code | ||
| $status = $response->getStatusCode(); | ||
| $cacheableStatuses = [200, 203, 204, 206, 300, 301, 404, 405, 410, 414, 501]; |
There was a problem hiding this comment.
The hardcoded list of cacheable status codes in shouldCache() (line 104) differs from the default cache_status_codes in ManagesCache trait (line 41 of ManagesCache.php shows [200, 203, 204, 206, 300, 301, 404, 410]). This inconsistency could lead to confusion:
CacheControl::shouldCache()includes:[200, 203, 204, 206, 300, 301, 404, 405, 410, 414, 501]ManagesCache::$cacheOptionsincludes:[200, 203, 204, 206, 300, 301, 404, 410]
The lists should be consistent, or the shouldCache() method should accept the configurable list as a parameter.
Contributor
Author
There was a problem hiding this comment.
Fixed in commit acb60ba. Both CacheControl and ManagesCache now use the same cacheable status codes list: [200, 203, 204, 206, 300, 301, 404, 410].
src/Fetch/Cache/FileCache.php
Outdated
Comment on lines
105
to
123
| // If TTL is provided, update the cached response with the correct expiration | ||
| if ($ttl !== null) { | ||
| $response = CachedResponse::fromArray( | ||
| array_merge($response->toArray(), [ | ||
| 'expires_at' => time() + $ttl, | ||
| ]) | ||
| ); | ||
| } elseif ($response->getExpiresAt() === null && $this->defaultTtl > 0) { | ||
| $response = CachedResponse::fromArray( | ||
| array_merge($response->toArray(), [ | ||
| 'expires_at' => time() + $this->defaultTtl, | ||
| ]) | ||
| ); | ||
| } | ||
|
|
||
| // PHPStan cannot infer that $response is non-null here after the conditionals above | ||
| // but we know $response is always a valid CachedResponse at this point | ||
| // @phpstan-ignore-next-line | ||
| $serialized = serialize($response->toArray()); |
There was a problem hiding this comment.
There's a subtle issue with this logic: CachedResponse::fromArray() can return null if the data is invalid, but the code doesn't handle this case. If fromArray() returns null (lines 107-111 or 113-117), the code will attempt to call methods on null on line 123, causing a fatal error.
The @phpstan-ignore-next-line comment acknowledges the issue but doesn't fix it. Consider:
if ($ttl !== null) {
$updatedResponse = CachedResponse::fromArray(
array_merge($response->toArray(), [
'expires_at' => time() + $ttl,
])
);
if ($updatedResponse !== null) {
$response = $updatedResponse;
}
}
// Similar for the elseif branch
Contributor
Author
There was a problem hiding this comment.
Fixed in commit acb60ba. The code now properly handles null returns from CachedResponse::fromArray() by only updating $response if the result is not null.
Comment on lines
191
to
210
| if ($handler->isAsync) { | ||
| return $handler->executeAsyncRequest($methodStr, $fullUri, $guzzleOptions); | ||
| } else { | ||
| return $handler->executeSyncRequest($methodStr, $fullUri, $guzzleOptions, $startTime); | ||
| return $this->executeSyncRequestWithCache($methodStr, $fullUri, $guzzleOptions, $startTime, $cachedResult, $handler); | ||
| } |
There was a problem hiding this comment.
The caching implementation in PerformsHttpRequests only handles synchronous requests via executeSyncRequestWithCache(). Async requests (line 206-207) bypass the caching logic entirely - they don't check the cache before making requests and don't cache responses after receiving them.
This creates an inconsistency where:
- Synchronous requests: full caching support (check cache, conditional requests, cache responses, stale-if-error)
- Async requests: no caching at all
Consider either:
- Implementing caching support for async requests
- Or documenting that caching only works for synchronous requests
- Or throwing an exception/warning when caching is enabled with async requests
Contributor
Author
There was a problem hiding this comment.
Added documentation in commit acb60ba. The ManagesCache trait now includes a note that caching is only supported for synchronous requests.
Comment on lines
+121
to
+139
| public function test_respects_no_store_cache_control(): void | ||
| { | ||
| $responses = [ | ||
| new GuzzleResponse(200, ['Content-Type' => 'application/json', 'Cache-Control' => 'no-store'], '{"data":"first"}'), | ||
| new GuzzleResponse(200, ['Content-Type' => 'application/json'], '{"data":"second"}'), | ||
| ]; | ||
|
|
||
| $handler = $this->create_handler_with_mock_responses($responses); | ||
| $handler->baseUri('https://api.example.com'); | ||
| $handler->withCache(); | ||
|
|
||
| // First request - no-store response | ||
| $response1 = $handler->get('/users'); | ||
| $this->assertEquals('{"data":"first"}', $response1->body()); | ||
|
|
||
| // Second request - should NOT be cached, get fresh response | ||
| $response2 = $handler->get('/users'); | ||
| $this->assertEquals('{"data":"second"}', $response2->body()); | ||
| } |
There was a problem hiding this comment.
The test suite doesn't include test coverage for the no-cache directive (which requires revalidation but allows caching). Only no-store is tested (line 121-139). According to RFC 7234, no-cache and no-store have different semantics:
no-store: must not store the responseno-cache: may store but must revalidate before using
Consider adding a test for no-cache behavior to ensure it's handled correctly and differently from no-store.
Contributor
Author
There was a problem hiding this comment.
Added test for no-cache directive in commit acb60ba (test_cache_requires_revalidation_with_no_cache_directive). The test verifies that responses with no-cache are cached but require revalidation, which is different from no-store.
Owner
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Contributor
Author
Fixed code style issues in commit 914eeb6 and addressed additional feedback in commit da5c143 (fixed incomplete PHPDoc, consistent TTL behavior, and switched to JSON serialization for security).
Fixed in commits 914eeb6 and da5c143. The changes include removing unused imports, fixing PHPDoc tags, reordering class elements, using JSON serialization instead of PHP serialize, and making TTL behavior consistent across cache backends. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…and clean up instantiation syntax
…n, null handling, and tests Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Contributor
Author
Applied all changes from the review thread in commit acb60ba:
|
…t and error handling
…t method; update response type hints in caching methods
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.
Pull Request
Description
What does this PR do?
Implements RFC 7234 compliant HTTP caching with configurable backends, Cache-Control header parsing, conditional requests (ETags/Last-Modified), and stale-while-revalidate support.
Fixes #40
Type of Change
Changes Made
Cache Infrastructure (
src/Fetch/Cache/)CacheInterface- Contract for cache backends with documented TTL behavior (null=default, 0=no expiration, negative=expired)CachedResponse- Value object with metadata (ETag, Last-Modified, TTL)CacheControl- Parses Cache-Control headers per RFC 7234 with configurable status codesCacheKeyGenerator- Generates keys from method, URI, headers, body hashMemoryCache- In-memory with LRU evictionFileCache- Persistent with TTL and size management, uses JSON serialization for securityIntegration
ManagesCachetrait - AddswithCache()/withoutCache()to ClientHandlerPerformsHttpRequeststo integrate cache lookup/storage in request flow (synchronous requests only)X-Cache-Statusheader (HIT/MISS/STALE/REVALIDATED)Code Quality Fixes
Testing
How has this been tested?
Test Environment:
Documentation
Checklist
Usage
Additional Notes
nulluses default TTL,0means no expiration, negative values mean already expiredOriginal prompt
This section details on the original issue you should resolve
<issue_title>RFC 7234 HTTP Caching Support</issue_title>
<issue_description>## Summary
Add RFC 7234 compliant HTTP caching support with intelligent cache management, ETags, Last-Modified headers, and configurable cache backends.
Motivation
HTTP caching is essential for:
Currently, Fetch PHP has no built-in caching mechanism, forcing developers to implement custom solutions or use separate libraries.
Proposed API
Implementation Details
Cache Backends
Cache Key Generation
Cache Control Headers