Make Cors service coroutine-safe
#293
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Hypervel\Http; | ||
|
|
||
| /** | ||
| * Immutable value object for normalized CORS options. | ||
| * | ||
| * Used for coroutine-safe storage in Context. Type validation is handled | ||
| * automatically by PHP's type system via typed properties - passing invalid | ||
| * types to the constructor will throw a TypeError. | ||
| */ | ||
| final readonly class CorsOptions | ||
| { | ||
| /** | ||
| * @param string[] $allowedOrigins Specific origins allowed to make requests | ||
| * @param string[] $allowedOriginsPatterns Regex patterns for matching allowed origins | ||
| * @param bool $supportsCredentials Whether to allow credentials (cookies, auth headers) | ||
| * @param string[] $allowedHeaders Headers the client is allowed to send | ||
| * @param string[] $allowedMethods HTTP methods the client is allowed to use | ||
| * @param string[] $exposedHeaders Headers the client is allowed to read from response | ||
| * @param null|int $maxAge How long preflight results can be cached (null = don't send header) | ||
| * @param bool $allowAllOrigins Whether '*' was specified for origins | ||
| * @param bool $allowAllHeaders Whether '*' was specified for headers | ||
| * @param bool $allowAllMethods Whether '*' was specified for methods | ||
| */ | ||
| public function __construct( | ||
| public array $allowedOrigins = [], | ||
| public array $allowedOriginsPatterns = [], | ||
| public bool $supportsCredentials = false, | ||
| public array $allowedHeaders = [], | ||
| public array $allowedMethods = [], | ||
| public array $exposedHeaders = [], | ||
| public ?int $maxAge = 0, | ||
| public bool $allowAllOrigins = false, | ||
| public bool $allowAllHeaders = false, | ||
| public bool $allowAllMethods = false, | ||
| ) { | ||
|
Comment on lines
+28
to
+39
There was a problem hiding this comment. The array properties lack runtime type enforcement for their elements. While the docblocks specify string[] for most arrays, PHP's type system cannot enforce array element types in constructor parameters. Consider adding validation in the constructor to ensure all elements are strings for string[] properties, or at minimum add a note in the docblock that the caller is responsible for ensuring correct array element types.
Contributor
Author
There was a problem hiding this comment. @albertcht This is technically true but not worth addressing in my opinion. The docblock accurately describes PHP's behavior, and |
||
| } | ||
| } | ||
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.
The constructor initializing options by calling setOptions directly introduces an anti-pattern in the coroutine-safe design. When Cors is instantiated as a singleton service (likely during app bootstrap), passing options to the constructor will store them in the bootstrap coroutine's context, which is never used since HandleCors middleware calls setOptions per-request. Consider either: (1) removing the ability to pass options to the constructor and documenting that setOptions must be called explicitly, or (2) deprecating constructor parameters in favor of always calling setOptions in the middleware. The current design is confusing because it suggests options can be set during construction, but those options may not be available in request coroutines.
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.
@albertcht The observation is partially correct - constructor options do write to bootstrap context - but it's harmless. I don't think it's an anti-pattern. The middleware calls
setOptions()per-request which writes to the correct request context, so the bootstrap write is just ignored.Constructor options are useful for testing (used throughout this PR's tests) and standalone usage. A deprecation warning would just spam test output for no real benefit.