-
Notifications
You must be signed in to change notification settings - Fork 79
Fix DST transition issue causing forced caching of non-cacheable responses #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
<?php | ||
|
||
namespace Kevinrob\GuzzleCache\Tests; | ||
|
||
use Kevinrob\GuzzleCache\CacheEntry; | ||
use GuzzleHttp\Psr7\Request; | ||
use GuzzleHttp\Psr7\Response; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
/** | ||
* Test for DST transition issue where DateTime('-1 seconds') can create | ||
* future timestamps during timezone transitions. | ||
* | ||
* This test verifies the fix for issue #194. | ||
*/ | ||
class DstTransitionTest extends TestCase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These test cases do not actually test the changes. They are just testing the behavior of the PHP I.e. the tests pass even if I revert the changes to $ git restore --source origin/master src/Strategy/PrivateCacheStrategy.php
$ ./vendor/bin/phpunit ./tests/DstTransitionTest.php
PHPUnit 9.6.25 by Sebastian Bergmann and contributors.
Warning: No code coverage driver available
Testing Kevinrob\GuzzleCache\Tests\DstTransitionTest
.. 2 / 2 (100%)
Time: 00:00.008, Memory: 6.00 MB
OK (2 tests, 10 assertions) |
||
{ | ||
private $originalTimezone; | ||
|
||
protected function setUp(): void | ||
{ | ||
// Save original timezone | ||
$this->originalTimezone = date_default_timezone_get(); | ||
} | ||
|
||
protected function tearDown(): void | ||
{ | ||
// Restore original timezone | ||
date_default_timezone_set($this->originalTimezone); | ||
} | ||
|
||
/** | ||
* Test CacheEntry behavior with UTC timestamp approach | ||
* This ensures that cache entries marked for immediate expiry | ||
* behave consistently across timezones. | ||
*/ | ||
public function testCacheEntryWithUtcTimestampIsAlwaysStale() | ||
{ | ||
$timezones = ['UTC', 'Europe/Berlin', 'America/New_York']; | ||
|
||
foreach ($timezones as $timezone) { | ||
date_default_timezone_set($timezone); | ||
|
||
$request = new Request('GET', 'http://example.com'); | ||
$response = new Response(200, [], 'test content'); | ||
|
||
// Create entry with UTC timestamp approach (the fix) | ||
$entry = new CacheEntry($request, $response, new \DateTime('@' . (time() - 1))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the problem only occurs during a DST transition; i.e. these test cases also pass with the old $ git diff
diff --git a/tests/DstTransitionTest.php b/tests/DstTransitionTest.php
index 7e6757e..bfdf896 100644
--- a/tests/DstTransitionTest.php
+++ b/tests/DstTransitionTest.php
@@ -45,7 +45,7 @@ class DstTransitionTest extends TestCase
$response = new Response(200, [], 'test content');
// Create entry with UTC timestamp approach (the fix)
- $entry = new CacheEntry($request, $response, new \DateTime('@' . (time() - 1)));
+ $entry = new CacheEntry($request, $response, new \DateTime('-1 seconds'));
// Should always be stale regardless of timezone
$this->assertTrue($entry->isStale(), "Entry should be stale in timezone: $timezone");
@@ -65,7 +65,7 @@ class DstTransitionTest extends TestCase
$response = new Response(200, [], 'test content');
// Create entry with UTC timestamp approach
- $entry = new CacheEntry($request, $response, new \DateTime('@' . (time() - 1)));
+ $entry = new CacheEntry($request, $response, new \DateTime('-1 seconds'));
$ttl = $entry->getTTL();
$ ./vendor/bin/phpunit ./tests/DstTransitionTest.php
PHPUnit 9.6.25 by Sebastian Bergmann and contributors.
Warning: No code coverage driver available
Testing Kevinrob\GuzzleCache\Tests\DstTransitionTest
.. 2 / 2 (100%)
Time: 00:00.009, Memory: 6.00 MB
OK (2 tests, 10 assertions) |
||
|
||
// Should always be stale regardless of timezone | ||
$this->assertTrue($entry->isStale(), "Entry should be stale in timezone: $timezone"); | ||
$this->assertFalse($entry->isFresh(), "Entry should not be fresh in timezone: $timezone"); | ||
$this->assertGreaterThan(0, $entry->getStaleAge(), "Stale age should be positive in timezone: $timezone"); | ||
} | ||
} | ||
|
||
/** | ||
* Test that validates the TTL calculation is correct | ||
*/ | ||
public function testCacheEntryTtlWithUtcTimestamp() | ||
{ | ||
date_default_timezone_set('Europe/Berlin'); | ||
|
||
$request = new Request('GET', 'http://example.com'); | ||
$response = new Response(200, [], 'test content'); | ||
|
||
// Create entry with UTC timestamp approach | ||
$entry = new CacheEntry($request, $response, new \DateTime('@' . (time() - 1))); | ||
|
||
$ttl = $entry->getTTL(); | ||
|
||
// TTL should be -1 for expired entries without validation info | ||
$this->assertEquals(-1, $ttl, "TTL should be -1 for expired entries without validation info"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering a bit about these changes:
Why is it important that the timestamp is exactly one second in the past?
I would expect it to be simpler to initialize it to a timestamp that is always in the past?