Added GlobalState #28
Conversation
WalkthroughReplaces PSR7Worker's stored original server copy with GlobalState-driven enrichment. GlobalState caches initial $_SERVER and exposes enrichServerVars(Request). PSR7Worker.configureServer/ waitRequest use GlobalState::enrichServerVars; tests updated to assert $_SERVER isolation; composer dev dependency added. Changes
Sequence Diagram(s)sequenceDiagram
participant Relay as TestRelay / RR
participant PSR7 as PSR7Worker
participant GlobalState
Relay->>PSR7: deliver Frame / Request payload
PSR7->>GlobalState: enrichServerVars(Request)
GlobalState-->>PSR7: enriched server array
alt $populateServer == true
PSR7->>+GlobalState: assign $_SERVER = enriched array
end
PSR7->>PSR7: map Request using enriched vars and process
PSR7-->>Relay: response frame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/ConfiguratorServer.php (2)
16-16: Improve return type annotation accuracy.The return type annotation
non-empty-array<array-key|string, mixed|string>is imprecise. Since$_SERVERis a superglobal with string keys and mixed values, and this method modifies it, the annotation should reflect the actual structure.Apply this diff to improve the return type annotation:
- * @return non-empty-array<array-key|string, mixed|string> + * @return array<string, mixed>
25-25: Clarify the purpose of empty HTTP_USER_AGENT initialization.Setting
HTTP_USER_AGENTto an empty string unconditionally may override actual user agent data from headers. If the request contains a User-Agent header, it will be processed in the loop and override this value, but the initialization seems unnecessary.Consider either removing this line or adding a comment explaining why it's needed:
- $_SERVER['HTTP_USER_AGENT'] = ''; + // Initialize HTTP_USER_AGENT - will be overridden if User-Agent header exists + $_SERVER['HTTP_USER_AGENT'] = '';Or remove it entirely if not needed:
- $_SERVER['HTTP_USER_AGENT'] = '';src/PSR7Worker.php (1)
45-45: Consider removing default instance for better testability.While providing a default instance of
ConfiguratorServermaintains backward compatibility, it reduces testability and makes dependency injection less explicit. Consider requiring the dependency to be explicitly provided.- private readonly ConfiguratorServer $configuratorServer = new ConfiguratorServer(), + private readonly ConfiguratorServer $configuratorServer,If backward compatibility is required, consider using a factory method or builder pattern instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ConfiguratorServer.php(1 hunks)src/PSR7Worker.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/ConfiguratorServer.php (1)
src/Request.php (1)
getRemoteAddr(55-58)
src/PSR7Worker.php (1)
src/ConfiguratorServer.php (2)
ConfiguratorServer(10-41)configure(18-40)
🔇 Additional comments (2)
src/ConfiguratorServer.php (1)
27-37: Header processing logic looks correct.The header normalization and processing logic correctly:
- Converts header names to uppercase and replaces hyphens with underscores (HTTP standard)
- Handles CONTENT_TYPE and CONTENT_LENGTH as special cases without HTTP_ prefix
- Prefixes other headers with HTTP_
- Properly concatenates multiple header values with commas
src/PSR7Worker.php (1)
70-72: Integration with ConfiguratorServer looks correct.The delegation to
$this->configuratorServer->configure($httpRequest)properly replaces the internal server configuration logic. The configured server array is then passed tomapRequest()as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/GlobalState.php (3)
15-18: Consider expanding the documentation.The current comment is brief. Consider adding more details about the method's behavior, parameters, and side effects.
- /** - * Sets ip-address, request-time and other values. - */ + /** + * Populates the global $_SERVER superglobal with data from a PSR-7 Request. + * + * Sets standard server variables (REQUEST_URI, REQUEST_TIME, etc.) and + * processes all HTTP headers, normalizing names and prefixing with HTTP_. + * Special handling for CONTENT_TYPE and CONTENT_LENGTH headers. + * + * @param Request $request The PSR-7 request object to extract data from + */
18-18: Consider the implications of directly mutating global state.While this method serves its intended purpose, directly modifying
$_SERVERcreates side effects that could make testing difficult and introduce coupling issues. Consider if this design aligns with your application's architecture principles.Alternative approaches to consider:
- Return an array instead of mutating
$_SERVERdirectly- Accept an optional array reference parameter to allow testing with mock data
- Use dependency injection to make the target array configurable
27-37: Header processing logic is mostly correct but has potential issues.The header normalization and processing logic follows HTTP standards, but there are some concerns:
- The comparison on line 30 uses
==instead of===(though this works correctly here)- Multiple header values are properly joined with commas per RFC 7230
Consider this minor improvement for strict comparison:
- if ($key == 'CONTENT_TYPE' || $key == 'CONTENT_LENGTH') { + if ($key === 'CONTENT_TYPE' || $key === 'CONTENT_LENGTH') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/GlobalState.php(1 hunks)src/PSR7Worker.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/PSR7Worker.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/GlobalState.php (1)
src/Request.php (1)
getRemoteAddr(55-58)
🔇 Additional comments (4)
src/GlobalState.php (4)
13-13: Good use offinalclass for utility functionality.The
finalmodifier appropriately prevents inheritance for this utility class that only contains static methods.
20-25: Verify the server variable assignments.The server variable assignments look correct and follow standard PHP conventions. The use of
$request->getRemoteAddr()properly delegates to the Request class for IP address resolution.
27-37: No header override risk:$_SERVERis reset in PSR7WorkerThe
populateServermethod is only ever invoked byPSR7Worker::configureServer, and immediately after, its returned array replaces the entire$_SERVERsuperglobal:
- PSR7Worker::configureServer calls
GlobalState::populateServer($request)- Then does
$_SERVER = $this->configureServer($httpRequest)Because
$_SERVERis freshly assigned there, there are no pre-existing headers to preserve or override—andpopulateServerisn’t called multiple times. You can safely disregard the header-override concern.Likely an incorrect or invalid review comment.
25-25: Verify HTTP_USER_AGENT assignment is overridden by header processingFile: src/GlobalState.php
Lines: 25Snippet:
$_SERVER['HTTP_USER_AGENT'] = '';I wasn’t able to locate the
foreach ($request->headers …)loop in this file, so it’s unclear whether a real User-Agent header ever gets written back into$_SERVER. Please confirm:
- That the code later iterates over
$request->headersand assignsHTTP_USER_AGENTwhen present- If it does not, this blank default will strip any genuine User-Agent value—consider removing this line or handling it uniformly with other headers
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/Unit/PSR7WorkerTest.php (1)
1-2: Add missing namespace to PSR7WorkerTest.phpThe other tests in tests/Unit use the
Spiral\RoadRunner\Tests\Http\Unitnamespace. To ensure consistent autoloading, update tests/Unit/PSR7WorkerTest.php as follows:--- tests/Unit/PSR7WorkerTest.php +++ tests/Unit/PSR7WorkerTest.php @@ -<?php +<?php +namespace Spiral\RoadRunner\Tests\Http\Unit;
- File affected: tests/Unit/PSR7WorkerTest.php
- Namespace to add:
Spiral\RoadRunner\Tests\Http\Unit
🧹 Nitpick comments (3)
tests/Unit/PSR7WorkerTest.php (3)
19-22: Remove misleading docblockThis method has no parameters, yet the docblock declares one. Drop or replace with a short description of the scenario instead.
- /*** - * @param array $headers - */ + /** Verifies that $_SERVER is fully re-populated per request without leaking state. */ public function testStateServerLeak(): void
33-37: Broaden fixtures to cover edge cases (CONTENT_LENGTH, multi-values)Add coverage for:
- Special-case CONTENT_LENGTH handling.
- Multi-value headers are properly joined with “, ” (e.g., Accept).
$fixtures = [ [['Content-Type' => ['application/html'], 'Connection' => ['keep-alive']], ['REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', 'CONTENT_TYPE' => 'application/html', 'HTTP_CONNECTION' => 'keep-alive',]], [['Content-Type' => ['application/json']], ['REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', 'CONTENT_TYPE' => 'application/json']], + // CONTENT_LENGTH is a special-case key; multi-value Accept must join with ", " + [['Content-Length' => ['3'], 'Accept' => ['text/html', 'application/json']], ['REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', 'CONTENT_LENGTH' => '3', 'HTTP_ACCEPT' => 'text/html, application/json']], ];
57-63: Assert request object and server params mirror the populated $_SERVERStrengthen the test: ensure waitRequest() returns a request and its server params match the global (excluding volatile time fields). Also assert the volatile fields exist before unsetting to catch regressions.
- $psrWorker->waitRequest(); + $request = $psrWorker->waitRequest(); + self::assertNotNull($request, 'waitRequest() should return a ServerRequestInterface instance'); - unset($_SERVER['REQUEST_TIME']); - unset($_SERVER['REQUEST_TIME_FLOAT']); + self::assertArrayHasKey('REQUEST_TIME', $_SERVER); + self::assertArrayHasKey('REQUEST_TIME_FLOAT', $_SERVER); + unset($_SERVER['REQUEST_TIME'], $_SERVER['REQUEST_TIME_FLOAT']); + + $requestServer = $request->getServerParams(); + unset($requestServer['REQUEST_TIME'], $requestServer['REQUEST_TIME_FLOAT']); + self::assertSame($_SERVER, $requestServer, 'Server params embedded into the PSR-7 request should equal the global $_SERVER'); self::assertEquals($expectedServer, $_SERVER);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)src/GlobalState.php(1 hunks)src/PSR7Worker.php(1 hunks)tests/Unit/PSR7WorkerTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- composer.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/GlobalState.php
- src/PSR7Worker.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Unit/PSR7WorkerTest.php (3)
src/GlobalState.php (1)
GlobalState(13-51)src/PSR7Worker.php (2)
PSR7Worker(24-240)waitRequest(63-73)tests/Unit/Stub/TestRelay.php (2)
TestRelay(10-75)addFrames(18-22)
| ); | ||
|
|
||
| self::assertInstanceOf(HttpWorker::class, $psrWorker->getHttpWorker()); | ||
| //dataProvider is always random and we need to keep the order |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the comment about DataProviders order
PHPUnit preserves the order of items provided by a data provider (unless a randomizer plugin is in use). If order is important, it’s fine to keep inline fixtures; otherwise consider a data provider for better test reporting.
Would you like to switch to a data provider? If you want to confirm ordering guarantees, run this web query:
🌐 Web query:
Does PHPUnit guarantee the iteration order of items returned by a data provider?
💡 Result:
PHPUnit does not explicitly guarantee the iteration order of items returned by a data provider. However, in practice, the data provider method returns an array or an iterable, and PHPUnit runs the test with each dataset in the order they are returned from the data provider.
Since the data provider returns a PHP array or iterable, and PHP arrays preserve insertion order (since PHP 5.4), PHPUnit will run the tests in the order the items are provided by the data provider.
So while not explicitly documented as a guarantee by PHPUnit, in typical usage and by PHP's own behavior regarding arrays, the iteration order of data provider items can be considered stable and predictable.
If you rely on a specific order, it's a good practice to:
- Return the datasets in the desired order from the data provider.
- Use numerically indexed arrays or ordered structures like generators that yield data in a specific sequence.
If you want to be absolutely certain about order, it is best to check against the PHPUnit documentation or source code to ensure no changes in future versions.
Would you like me to check for any recent documentation or discussions on this topic?
Update misleading comment about DataProvider ordering
The inline comment in tests/Unit/PSR7WorkerTest.php (line 33) is incorrect: PHPUnit runs data‐provider datasets in the order they’re returned (PHP arrays preserve insertion order). To improve clarity and test reporting, either adjust the comment or extract these cases into a named data provider.
Locations to update:
- tests/Unit/PSR7WorkerTest.php (line 33)
Suggested diff:
- //dataProvider is always random and we need to keep the order
+ // PHPUnit executes dataProvider items in the order they’re returned
+ // (PHP arrays preserve insertion order). Consider using a named data provider
+ // for clearer test reporting.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //dataProvider is always random and we need to keep the order | |
| // PHPUnit executes dataProvider items in the order they’re returned | |
| // (PHP arrays preserve insertion order). Consider using a named data provider | |
| // for clearer test reporting. |
🤖 Prompt for AI Agents
In tests/Unit/PSR7WorkerTest.php at line 33, the comment incorrectly states that
the dataProvider is always random, but PHPUnit actually runs data-provider
datasets in the order they are returned. Update the comment to accurately
reflect that the order is preserved or refactor the test to use a named data
provider method for clarity and better test reporting.
tests/Unit/PSR7WorkerTest.php
Outdated
| [['Content-Type' => ['application/html'], 'Connection' => ['keep-alive']], ['REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', 'CONTENT_TYPE' => 'application/html', 'HTTP_CONNECTION' => 'keep-alive',]], | ||
| [['Content-Type' => ['application/json']], ['REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', 'CONTENT_TYPE' => 'application/json']], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify REQUEST_URI semantics
The test expects REQUEST_URI to be the full URI (e.g., "http://localhost"). In classic PHP SAPI, REQUEST_URI usually contains the path + query, not scheme/host. If this divergence is intentional with GlobalState::populateServer(), document it to prevent future confusion.
To confirm usages and expectations across the codebase:
🏁 Script executed:
#!/bin/bash
# Find usages of REQUEST_URI to assess assumptions about its shape
rg -n "REQUEST_URI" -A2 -B2 src tests | sed -n '1,200p'Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Display the beginning of GlobalState.php to inspect the method signature and existing docblocks
sed -n '1,200p' src/GlobalState.phpLength of output: 1347
🏁 Script executed:
#!/bin/bash
# Locate the Request class definition to inspect the `uri` property
rg -n "class Request" -A10 -B5 srcLength of output: 741
🏁 Script executed:
#!/bin/bash
# Show property definitions and constructor in Request.php
sed -n '1,200p' src/Request.phpLength of output: 1969
Clarify REQUEST_URI semantics
GlobalState::populateServer currently assigns REQUEST_URI from Request::$uri, which holds the full URI (scheme + host). This deviates from PHP SAPI’s usual behavior (path + query only). If this is intentional, please update the docblock on populateServer() to document that REQUEST_URI will include the full URI from the Request object.
• File needing update: src/GlobalState.php (populateServer docblock)
• Suggested diff:
--- a/src/GlobalState.php
+++ b/src/GlobalState.php
@@ /**
- * Sets ip-address, request-time and other values.
+ * Populates $_SERVER values (ip address, request time, HTTP headers, etc.).
+ * REQUEST_URI is set to the full URI (including scheme and host) from the Request object,
+ * rather than the path+query typical of PHP SAPI.🤖 Prompt for AI Agents
In tests/Unit/PSR7WorkerTest.php around lines 35 to 36, the test data uses
REQUEST_URI with a full URI including scheme and host, which differs from
typical PHP SAPI behavior where REQUEST_URI contains only path and query. To fix
this, update the docblock of the populateServer() method in src/GlobalState.php
to clearly document that REQUEST_URI will include the full URI from the Request
object, reflecting this intentional design choice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/Unit/PSR7WorkerTest.php (2)
35-35: Update misleading comment about data provider orderPHPUnit executes data provider datasets in the order they’re returned (PHP arrays preserve insertion order). If order matters, inline fixtures are fine; otherwise, consider a data provider for clearer reporting.
- //dataProvider is always random and we need to keep the order + // PHPUnit executes dataProvider items in the order they’re returned. + // Keeping inline fixtures is fine if order matters; a named data provider could improve reporting.
37-39: Document full-URI semantics for REQUEST_URI in GlobalStateThe test expects REQUEST_URI to be a full URI (scheme+host), diverging from typical SAPI (path+query). If intentional, document it in GlobalState::populateServer() to prevent confusion.
Suggested change in src/GlobalState.php:
- /** - * Sets ip-address, request-time and other values. + /** + * Populates $_SERVER values (ip address, request time, HTTP headers, etc.). + * Note: REQUEST_URI is set to the full URI (including scheme and host) from Request::$uri, + * rather than the path+query typical of PHP SAPI.
🧹 Nitpick comments (4)
tests/Unit/PSR7WorkerTest.php (4)
21-23: Remove incorrect @param docblock on a parameterless testThe test has no parameters; the docblock is misleading. Replace with a brief summary or remove it.
- /*** - * @param array $headers - */ + /** + * Ensures GlobalState does not leak $_SERVER state between consecutive requests. + */
52-55: Clarify frame construction; prefer clearer naming (header vs body)Use clearer variable names to reflect header vs body and improve readability.
- $head = (string)\json_encode($body, \JSON_THROW_ON_ERROR); - $frame = new Frame($head .'test', [\strlen($head)]); + $header = \json_encode($body, \JSON_THROW_ON_ERROR); + $frame = new Frame($header . 'test', [\strlen($header)]);Optionally, add a tiny helper to avoid re-encoding boilerplate:
private static function makeFrame(array $context, string $body = ''): Frame { $header = \json_encode($context, \JSON_THROW_ON_ERROR); return new Frame($header . $body, [\strlen($header)]); }
57-57: Be mindful of GlobalState’s cached baseline of $_SERVERGlobalState::populateServer() snapshots $_SERVER on first call (static cache). Clearing $_SERVER before the first waitRequest keeps the baseline minimal, which this test relies on. If more tests are added, ensure no earlier call sets a non-empty baseline, or consider introducing a test-only reset in GlobalState.
64-64: Prefer assertSame for strict array equalityUse assertSame to enforce both value and type equality.
- self::assertEquals($expectedServer, $_SERVER); + self::assertSame($expectedServer, $_SERVER);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/GlobalState.php(1 hunks)src/PSR7Worker.php(2 hunks)tests/Unit/PSR7WorkerTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/PSR7Worker.php
- src/GlobalState.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Unit/PSR7WorkerTest.php (4)
src/GlobalState.php (1)
GlobalState(13-52)src/PSR7Worker.php (2)
PSR7Worker(24-242)waitRequest(65-75)tests/Unit/Stub/TestRelay.php (2)
TestRelay(10-75)addFrames(18-22)src/HttpWorker.php (1)
waitRequest(56-80)
🔇 Additional comments (2)
tests/Unit/PSR7WorkerTest.php (2)
16-18: Good use of attributes for coverage and isolationCovering PSR7Worker and GlobalState, and running in a separate process, matches the static-baseline behavior of GlobalState and prevents cross-test leakage.
41-65: Overall test flow looks solidLooping over fixtures, resetting $_SERVER, and unsetting time-based keys yields deterministic assertions and validates no state leakage between requests.
|
@roxblnfk ping |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Unit/PSR7WorkerTest.php (1)
34-34: Fix misleading comment about data provider orderPHPUnit iterates datasets in the order they’re returned; the “always random” note is inaccurate.
Apply:
- //dataProvider is always random and we need to keep the order + // PHPUnit executes data provider datasets in the order they are returned + // (PHP arrays preserve insertion order). Keeping inline fixtures is fine here.
🧹 Nitpick comments (2)
tests/Unit/PSR7WorkerTest.php (2)
35-62: Broaden fixtures to cover header normalization, multi-values, and special casesAdd cases to assert:
- CONTENT_LENGTH is set without HTTP_ prefix.
- Multi-value headers are joined with ", ".
- User-Agent overwrites the default empty HTTP_USER_AGENT.
Also, prefer a real media type for HTML.$fixtures = [ [ [ - 'Content-Type' => ['application/html'], + 'Content-Type' => ['text/html'], 'Connection' => ['keep-alive'] ], [ 'REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', - 'CONTENT_TYPE' => 'application/html', + 'CONTENT_TYPE' => 'text/html', 'HTTP_CONNECTION' => 'keep-alive', ], ], [ [ 'Content-Type' => ['application/json'] ], [ 'REQUEST_URI' => 'http://localhost', 'REMOTE_ADDR' => '127.0.0.1', 'REQUEST_METHOD' => 'GET', 'HTTP_USER_AGENT' => '', 'CONTENT_TYPE' => 'application/json' ], ], + [ + [ + 'User-Agent' => ['rr-test'], + 'Content-Type' => ['application/json'], + 'Content-Length' => ['123'], + 'Accept' => ['text/html', 'application/json'], + 'X-Custom-Header' => ['A', 'B'], + ], + [ + 'REQUEST_URI' => 'http://localhost', + 'REMOTE_ADDR' => '127.0.0.1', + 'REQUEST_METHOD' => 'GET', + 'HTTP_USER_AGENT' => 'rr-test', + 'CONTENT_TYPE' => 'application/json', + 'CONTENT_LENGTH' => '123', + 'HTTP_ACCEPT' => 'text/html, application/json', + 'HTTP_X_CUSTOM_HEADER' => 'A, B', + ], + ], ];
64-87: Make isolation explicit and assert PSR-7 request typeReset $_SERVER inside the loop to decouple the test from GlobalState’s “first-call snapshot” behavior, and assert that waitRequest() returns a ServerRequestInterface.
- $_SERVER = []; - foreach ($fixtures as [$headers, $expectedServer]) { + foreach ($fixtures as [$headers, $expectedServer]) { + $_SERVER = []; $body = [ 'headers' => $headers, 'rawQuery' => '', 'remoteAddr' => '127.0.0.1', 'protocol' => 'HTTP/1.1', 'method' => 'GET', 'uri' => 'http://localhost', 'parsed' => false, ]; $head = (string)\json_encode($body, \JSON_THROW_ON_ERROR); $frame = new Frame($head .'test', [\strlen($head)]); $relay->addFrames($frame); - $psrWorker->waitRequest(); + $request = $psrWorker->waitRequest(); + self::assertNotNull($request); + self::assertInstanceOf(\Psr\Http\Message\ServerRequestInterface::class, $request); - unset($_SERVER['REQUEST_TIME']); - unset($_SERVER['REQUEST_TIME_FLOAT']); + unset($_SERVER['REQUEST_TIME'], $_SERVER['REQUEST_TIME_FLOAT']); self::assertEquals($expectedServer, $_SERVER); }Add import:
+use Psr\Http\Message\ServerRequestInterface;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/Unit/PSR7WorkerTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/PSR7WorkerTest.php (3)
src/GlobalState.php (1)
GlobalState(13-52)src/PSR7Worker.php (2)
PSR7Worker(24-242)waitRequest(65-75)tests/Unit/Stub/TestRelay.php (2)
TestRelay(10-75)addFrames(18-22)
🔇 Additional comments (2)
tests/Unit/PSR7WorkerTest.php (2)
18-21: Good use of attributes for isolation and coverageSeparate process is appropriate given GlobalState’s static snapshot; class-level CoversClass is also correct.
42-47: Clarify REQUEST_URI semantics vs classic PHP SAPIThe test asserts a full URI in REQUEST_URI (scheme+host). If intentional, document this in GlobalState::populateServer to prevent confusion.
Proposed doc update in src/GlobalState.php:
- /** - * Sets ip-address, request-time and other values. - * - * @return non-empty-array<array-key|string, mixed|string> - */ + /** + * Populates a new $_SERVER array snapshot (IP address, request time, HTTP headers, etc.). + * Note: REQUEST_URI is set to the full URI from Request::$uri (including scheme and host), + * not just path+query as in classic PHP SAPI. + * + * @return non-empty-array<array-key|string, mixed|string> + */
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/GlobalState.php (2)
60-60: Avoid side effects at autoload; lazily cache $_SERVER instead.Running code on include can surprise consumers. Prefer lazy init and remove the global call.
final class GlobalState { /** * @var array<array-key, mixed> Cached state of the $_SERVER superglobal. */ private static array $cachedServer = []; + /** + * Whether initial cache has been taken. + */ + private static bool $initialized = false; /** * Cache superglobal $_SERVER to avoid state leaks between requests. */ public static function cacheServerVars(): void { - self::$cachedServer = $_SERVER; + self::$cachedServer = $_SERVER; + self::$initialized = true; } @@ public static function enrichServerVars(Request $request): array { - $server = self::$cachedServer; + if (!self::$initialized) { + self::cacheServerVars(); + } + $server = self::$cachedServer; @@ -GlobalState::cacheServerVars(); +// Intentionally no side effects at autoload.Also applies to: 18-26, 33-36
37-54: Header mapping nits: add SERVER_PROTOCOL; use strict compare; avoid defaulting HTTP_USER_AGENT.Align closer to SAPI semantics and tighten comparisons.
- $server['REQUEST_METHOD'] = $request->method; - $server['HTTP_USER_AGENT'] = ''; + $server['REQUEST_METHOD'] = $request->method; + // Keep protocol in sync with the current request + $server['SERVER_PROTOCOL'] = $request->protocol; foreach ($request->headers as $key => $value) { $key = strtoupper(str_replace('-', '_', $key)); - if ($key == 'CONTENT_TYPE' || $key == 'CONTENT_LENGTH') { + if ($key === 'CONTENT_TYPE' || $key === 'CONTENT_LENGTH') { $server[$key] = implode(', ', $value); continue; } $server['HTTP_' . $key] = implode(', ', $value); }Note: Removing the default for HTTP_USER_AGENT lets it remain unset when absent, matching typical PHP behavior.
src/PSR7Worker.php (2)
126-135: Deprecate configureServer as discussed.Per prior discussion about BC on protected methods, mark this method deprecated and point callers to GlobalState::enrichServerVars.
- /** - * Returns altered copy of _SERVER variable. Sets ip-address, - * request-time and other values. - * - * @return non-empty-array<array-key|string, mixed|string> - */ + /** + * Returns altered copy of _SERVER variable. Sets ip-address, + * request-time and other values. + * + * @deprecated 3.x Will be removed in 4.0. Use GlobalState::enrichServerVars(). + * @return non-empty-array<array-key|string, mixed|string> + */
137-151: Include version in deprecation notes for timeInt/timeFloat.Helps downstream users plan removals.
- /** - * @deprecated - */ + /** + * @deprecated 3.x Will be removed in 4.0. + */ @@ - /** - * @deprecated - */ + /** + * @deprecated 3.x Will be removed in 4.0. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
composer.json(1 hunks)src/GlobalState.php(1 hunks)src/PSR7Worker.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/GlobalState.php (1)
src/Request.php (1)
getRemoteAddr(55-58)
src/PSR7Worker.php (4)
src/HttpWorker.php (1)
waitRequest(56-80)src/HttpWorkerInterface.php (1)
waitRequest(19-19)src/PSR7WorkerInterface.php (1)
waitRequest(13-13)src/GlobalState.php (2)
GlobalState(13-58)enrichServerVars(33-57)
🔇 Additional comments (2)
composer.json (1)
53-55: Dev-only addition looks fine.Adding spiral/dumper under require-dev is safe and scoped to development.
src/PSR7Worker.php (1)
74-81: Nice: explicit vars + opt-in $_SERVER population removes double mutation.This resolves earlier concerns about hidden global side effects and makes data flow clear.
| * @throws \JsonException | ||
| */ | ||
| public function waitRequest(): ?ServerRequestInterface | ||
| public function waitRequest(bool $populateServer = true): ?ServerRequestInterface |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm interface compatibility for waitRequest signature.
PSR7Worker implements PSR7WorkerInterface; adding a parameter may break signature compatibility if the interface wasn’t updated.
If the interface still declares waitRequest(): ?ServerRequestInterface, either:
- update the interface to include the optional parameter, or
- revert the parameter here and expose an alternative opt-out (e.g., a new method or a constructor flag).
🏁 Script executed:
#!/bin/bash
# Verify interface vs implementation signatures
rg -nP -C3 'interface\s+PSR7WorkerInterface\b' --type php
rg -nP -C1 'function\s+waitRequest\s*\(' --type php | sed -n '1,120p'Length of output: 1158
Align waitRequest signature between PSR7WorkerInterface and PSR7Worker.
PSR7WorkerInterface declares
public function waitRequest(): ?ServerRequestInterface;but PSR7Worker implements
public function waitRequest(bool $populateServer = true): ?ServerRequestInterface— update the interface to include the bool $populateServer = true parameter or remove it from the implementation to restore compatibility.
🤖 Prompt for AI Agents
In src/PSR7Worker.php around line 67, the method signature public function
waitRequest(bool $populateServer = true): ?ServerRequestInterface is
incompatible with PSR7WorkerInterface which declares public function
waitRequest(): ?ServerRequestInterface; update the interface to match this
implementation by adding the parameter signature (bool $populateServer = true)
to PSR7WorkerInterface or remove the optional parameter from the PSR7Worker
implementation so both signatures are identical; pick one approach and update
the corresponding declaration, run static analysis/tests to ensure no other
callers break.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores