From ff919e89bc12fe8edeb0b41ed6344b3f64e9ae9a Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Thu, 29 Jan 2026 22:19:08 +0900 Subject: [PATCH 1/3] Add ServerContextInterface for coroutine-safe request handling Introduce ServerContextInterface to abstract $_SERVER access, enabling thread-safe operation in concurrent environments like Swoole/RoadRunner. Changes: - Add ServerContextInterface with get() and has() methods - Add GlobalServerContext for traditional PHP-FPM (default) - Update ResourceStorage to use ServerContextInterface for X_VARY handling - Add RepositoryLoggerInterface::reset() for long-running environments Note: MobileEtagSetter is not modified (uses global state, rarely used) --- src/GlobalServerContext.php | 40 +++++++++++++++++ src/QueryRepositoryModule.php | 2 + src/RepositoryLogger.php | 9 ++++ src/RepositoryLoggerInterface.php | 9 ++++ src/ResourceStorage.php | 23 ++++++---- src/ServerContextInterface.php | 33 ++++++++++++++ tests/GlobalServerContextTest.php | 72 +++++++++++++++++++++++++++++++ tests/RepositoryLoggerTest.php | 21 +++++++++ tests/ResourceRepositoryTest.php | 2 + tests/ResourceStorageTest.php | 1 + 10 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 src/GlobalServerContext.php create mode 100644 src/ServerContextInterface.php create mode 100644 tests/GlobalServerContextTest.php diff --git a/src/GlobalServerContext.php b/src/GlobalServerContext.php new file mode 100644 index 00000000..57c97f43 --- /dev/null +++ b/src/GlobalServerContext.php @@ -0,0 +1,40 @@ +bind(RefreshAnnotatedCommand::class); $this->bind(RefreshSameCommand::class); $this->bind(ResourceStorageSaver::class); + // Server context for thread safety (Swoole, RoadRunner, etc.) + $this->bind(ServerContextInterface::class)->to(GlobalServerContext::class)->in(Scope::SINGLETON); // #[Cacheable] $this->install(new CacheableModule()); // #[CacheableResponse] diff --git a/src/RepositoryLogger.php b/src/RepositoryLogger.php index 57dbea33..29be391e 100644 --- a/src/RepositoryLogger.php +++ b/src/RepositoryLogger.php @@ -28,6 +28,15 @@ public function log(string $operation, array $context = []): void $this->logs[] = ['op' => $operation, ...$context]; } + /** + * {@inheritDoc} + */ + #[Override] + public function reset(): void + { + $this->logs = []; + } + #[Override] public function __toString(): string { diff --git a/src/RepositoryLoggerInterface.php b/src/RepositoryLoggerInterface.php index 2668352a..c696bbff 100644 --- a/src/RepositoryLoggerInterface.php +++ b/src/RepositoryLoggerInterface.php @@ -9,5 +9,14 @@ interface RepositoryLoggerInterface /** @param array $context */ public function log(string $operation, array $context = []): void; + /** + * Reset the logger state + * + * This method clears all accumulated logs. It should be called at the end of + * each request in long-running environments (Swoole, RoadRunner) to prevent + * log accumulation across requests. + */ + public function reset(): void; + public function __toString(): string; } diff --git a/src/ResourceStorage.php b/src/ResourceStorage.php index b28bd5ad..d3f1ac8c 100644 --- a/src/ResourceStorage.php +++ b/src/ResourceStorage.php @@ -21,7 +21,6 @@ use function explode; use function implode; use function is_array; -use function is_string; use function sprintf; use function strtoupper; @@ -32,7 +31,8 @@ * uriTag: UriTag, * saver: ResourceStorageSaver, * roProvider:ProviderInterface, - * etagProvider: ProviderInterface + * etagProvider: ProviderInterface, + * serverContext: ServerContextInterface * } */ final class ResourceStorage implements ResourceStorageInterface @@ -64,6 +64,7 @@ public function __construct( private PurgerInterface $purger, private UriTagInterface $uriTag, private ResourceStorageSaver $saver, + private ServerContextInterface $serverContext, #[Set(TagAwareAdapterInterface::class, ResourceObjectPool::class)] ProviderInterface $roPoolProvider, #[Set(TagAwareAdapterInterface::class, EtagPool::class)] @@ -239,19 +240,23 @@ private function evaluateBody(mixed $body): mixed private function getUriKey(AbstractUri $uri, string $type): string { - return $type . ($this->uriTag)($uri) . (isset($_SERVER['X_VARY']) ? $this->getVary() : ''); + return $type . ($this->uriTag)($uri) . ($this->serverContext->has('X_VARY') ? $this->getVary() : ''); } private function getVary(): string { - $xvary = $_SERVER['X_VARY']; - /** @psalm-suppress RedundantCast */ - $varys = explode(',', (string) $xvary); // @phpstan-ignore-line + $xvary = $this->serverContext->get('X_VARY'); + if ($xvary === null) { + return ''; + } + + $varys = explode(',', $xvary); $varyString = ''; foreach ($varys as $vary) { $phpVaryKey = sprintf('X_%s', strtoupper($vary)); - if (isset($_SERVER[$phpVaryKey]) && is_string($_SERVER[$phpVaryKey])) { - $varyString .= $_SERVER[$phpVaryKey]; + $value = $this->serverContext->get($phpVaryKey); + if ($value !== null) { + $varyString .= $value; } } @@ -279,6 +284,7 @@ public function __serialize(): array 'saver' => $this->saver, 'roProvider' => $this->roPoolProvider, 'etagProvider' => $this->etagPoolProvider, + 'serverContext' => $this->serverContext, ]; } @@ -293,6 +299,7 @@ public function __unserialize(array $data): void $this->purger = $data['purger']; $this->uriTag = $data['uriTag']; $this->saver = $data['saver']; + $this->serverContext = $data['serverContext']; $this->initializePools($data['roProvider'], $data['etagProvider']); } } diff --git a/src/ServerContextInterface.php b/src/ServerContextInterface.php new file mode 100644 index 00000000..2f27db55 --- /dev/null +++ b/src/ServerContextInterface.php @@ -0,0 +1,33 @@ +context = new GlobalServerContext(); + } + + protected function tearDown(): void + { + unset($_SERVER['TEST_KEY'], $_SERVER['TEST_STRING'], $_SERVER['TEST_INT']); + + parent::tearDown(); + } + + public function testGetReturnsValue(): void + { + $_SERVER['TEST_KEY'] = 'test_value'; + + $this->assertSame('test_value', $this->context->get('TEST_KEY')); + } + + public function testGetReturnsNullForNonExistentKey(): void + { + $this->assertNull($this->context->get('NON_EXISTENT_KEY')); + } + + public function testGetReturnsNullForNonStringValue(): void + { + $_SERVER['TEST_INT'] = 123; + + $this->assertNull($this->context->get('TEST_INT')); + } + + public function testHasReturnsTrueForExistingKey(): void + { + $_SERVER['TEST_KEY'] = 'value'; + + $this->assertTrue($this->context->has('TEST_KEY')); + } + + public function testHasReturnsFalseForNonExistentKey(): void + { + $this->assertFalse($this->context->has('NON_EXISTENT_KEY')); + } + + public function testGetHttpUserAgent(): void + { + $userAgent = 'Mozilla/5.0 Test'; + $_SERVER['HTTP_USER_AGENT'] = $userAgent; + + $this->assertSame($userAgent, $this->context->get('HTTP_USER_AGENT')); + } + + public function testGetXVary(): void + { + $_SERVER['X_VARY'] = 'val1,val2'; + + $this->assertSame('val1,val2', $this->context->get('X_VARY')); + } +} diff --git a/tests/RepositoryLoggerTest.php b/tests/RepositoryLoggerTest.php index 76c79bda..69082b7f 100644 --- a/tests/RepositoryLoggerTest.php +++ b/tests/RepositoryLoggerTest.php @@ -65,4 +65,25 @@ public function testLogWithNullValue(): void $logger->log('save-donut', ['uri' => 'app://self/page', 'sMaxAge' => null]); $this->assertSame('{"op":"save-donut","uri":"app://self/page","sMaxAge":null}', (string) $logger); } + + public function testReset(): void + { + $logger = new RepositoryLogger(); + $logger->log('operation1', ['id' => 1]); + $logger->log('operation2', ['id' => 2]); + $this->assertNotEmpty((string) $logger); + + $logger->reset(); + $this->assertSame('', (string) $logger); + } + + public function testResetAllowsNewLogs(): void + { + $logger = new RepositoryLogger(); + $logger->log('old-operation'); + $logger->reset(); + $logger->log('new-operation'); + + $this->assertSame('{"op":"new-operation"}', (string) $logger); + } } diff --git a/tests/ResourceRepositoryTest.php b/tests/ResourceRepositoryTest.php index abc0a607..a121cf4f 100644 --- a/tests/ResourceRepositoryTest.php +++ b/tests/ResourceRepositoryTest.php @@ -44,6 +44,7 @@ public function get() new NullPurger(), new UriTag(), new ResourceStorageSaver(), + new GlobalServerContext(), $tagAwareAdapterProvider, $tagAwareAdapterProvider, ), @@ -106,6 +107,7 @@ public function get() new NullPurger(), new UriTag(), new ResourceStorageSaver(), + new GlobalServerContext(), $tagAwareAdapterProvider, $tagAwareAdapterProvider, ), diff --git a/tests/ResourceStorageTest.php b/tests/ResourceStorageTest.php index 4ca68b9f..71854af0 100644 --- a/tests/ResourceStorageTest.php +++ b/tests/ResourceStorageTest.php @@ -35,6 +35,7 @@ public function get() new NullPurger(), new UriTag(), new ResourceStorageSaver(), + new GlobalServerContext(), $tagAwareAdapterProvider, $tagAwareAdapterProvider, ); From 31485c2c2c7fcf3896858f22519dd51936a9239f Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Thu, 29 Jan 2026 23:10:21 +0900 Subject: [PATCH 2/3] Address CodeRabbit review feedback - Fix test tearDown to restore HTTP_USER_AGENT and X_VARY - Trim X_VARY tokens to handle optional whitespace --- src/ResourceStorage.php | 6 ++++++ tests/GlobalServerContextTest.php | 21 ++++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/ResourceStorage.php b/src/ResourceStorage.php index d3f1ac8c..174a47a3 100644 --- a/src/ResourceStorage.php +++ b/src/ResourceStorage.php @@ -23,6 +23,7 @@ use function is_array; use function sprintf; use function strtoupper; +use function trim; /** * @psalm-type Props = array{ @@ -253,6 +254,11 @@ private function getVary(): string $varys = explode(',', $xvary); $varyString = ''; foreach ($varys as $vary) { + $vary = trim($vary); + if ($vary === '') { + continue; + } + $phpVaryKey = sprintf('X_%s', strtoupper($vary)); $value = $this->serverContext->get($phpVaryKey); if ($value !== null) { diff --git a/tests/GlobalServerContextTest.php b/tests/GlobalServerContextTest.php index b3547ccd..e02f90ae 100644 --- a/tests/GlobalServerContextTest.php +++ b/tests/GlobalServerContextTest.php @@ -6,20 +6,39 @@ use PHPUnit\Framework\TestCase; +use function array_key_exists; + class GlobalServerContextTest extends TestCase { private GlobalServerContext $context; + /** @var array */ + private array $originalServer = []; + protected function setUp(): void { parent::setUp(); + // Save original values for keys we'll modify + foreach (['TEST_KEY', 'TEST_STRING', 'TEST_INT', 'HTTP_USER_AGENT', 'X_VARY'] as $key) { + if (array_key_exists($key, $_SERVER)) { + $this->originalServer[$key] = $_SERVER[$key]; + } + } + $this->context = new GlobalServerContext(); } protected function tearDown(): void { - unset($_SERVER['TEST_KEY'], $_SERVER['TEST_STRING'], $_SERVER['TEST_INT']); + // Restore original values or unset if they didn't exist + foreach (['TEST_KEY', 'TEST_STRING', 'TEST_INT', 'HTTP_USER_AGENT', 'X_VARY'] as $key) { + if (array_key_exists($key, $this->originalServer)) { + $_SERVER[$key] = $this->originalServer[$key]; + } else { + unset($_SERVER[$key]); + } + } parent::tearDown(); } From 82fd0203173c97dd60798e536449b48c902ce522 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Fri, 30 Jan 2026 10:38:04 +0900 Subject: [PATCH 3/3] Fix coverage: remove dead code in getVary(), add edge case test Replace unreachable null-return with assert since getVary() is only called when has('X_VARY') is true. Add test for empty X_VARY segment. --- src/ResourceStorage.php | 4 +--- tests/GetInterceptorTest.php | 11 +++++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/ResourceStorage.php b/src/ResourceStorage.php index 174a47a3..a3d3b1da 100644 --- a/src/ResourceStorage.php +++ b/src/ResourceStorage.php @@ -247,9 +247,7 @@ private function getUriKey(AbstractUri $uri, string $type): string private function getVary(): string { $xvary = $this->serverContext->get('X_VARY'); - if ($xvary === null) { - return ''; - } + assert($xvary !== null, 'getVary() is only called when X_VARY exists'); $varys = explode(',', $xvary); $varyString = ''; diff --git a/tests/GetInterceptorTest.php b/tests/GetInterceptorTest.php index 114d958d..d5038a5f 100644 --- a/tests/GetInterceptorTest.php +++ b/tests/GetInterceptorTest.php @@ -101,4 +101,15 @@ public function testHttpCacheVary(): void unset($_SERVER['X_VARY'], $_SERVER['X_VAL1'], $_SERVER['X_VAL2']); } + + public function testHttpCacheVaryWithEmptySegment(): void + { + $_SERVER['X_VARY'] = 'val1, , val2'; + $_SERVER['X_VAL1'] = '1'; + $_SERVER['X_VAL2'] = '2'; + $ro = $this->resource->get('app://self/etag'); + $this->assertArrayNotHasKey('Age', $ro->headers); + + unset($_SERVER['X_VARY'], $_SERVER['X_VAL1'], $_SERVER['X_VAL2']); + } }