Make Cors service coroutine-safe

[binaryfire]

Summary

The Cors class appears to be a direct port from the fruitcake/php-cors package. This PR refactors it to be coroutine-safe, which also allows per-request CORS configuration overrides.

Changes

1. Coroutine-safe state storage: Options are now stored in Context using an immutable CorsOptions value object, following the same patterns used by AuthManager, LogManager, and other Hypervel services.
2. Per-request configuration: The HandleCors middleware now calls setOptions() in process() rather than the constructor, enabling configuration to vary per-request.
3. Extensibility hook: Added getCorsConfig() method to HandleCors for easy customization (e.g., tenant-specific CORS origins).

Files Changed

• src/http/src/Cors.php: Refactored to use Context for state storage
• src/http/src/CorsOptions.php: New readonly value object for type-safe options
• src/http/src/Middleware/HandleCors.php: Added getCorsConfig() method, moved setOptions() to process()
• tests/Http/CorsTest.php: Added tests for header/method normalization, origin matching, Vary header handling, and coroutine isolation

Usage Example

Extending for tenant-aware CORS:

  class TenantAwareHandleCors extends HandleCors
  {
      protected function getCorsConfig(): array
      {
          $config = parent::getCorsConfig();

          if ($origins = tenancy()->corsAllowedOrigins()) {
              $config['allowed_origins'] = $origins;
          }

          return $config;
      }
  }

Breaking Changes

None. The public API remains unchanged.

[Copilot]

Pull request overview

This PR refactors the Cors service to be coroutine-safe by moving from instance properties to coroutine-local Context storage. This enables safe concurrent request handling in Swoole's coroutine environment and allows per-request CORS configuration overrides.

Key changes:

• Options are now stored in Context as an immutable CorsOptions value object instead of instance properties
• The HandleCors middleware now calls setOptions() per-request rather than in the constructor
• Added getCorsConfig() extensibility hook for custom CORS configuration (e.g., tenant-specific origins)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/http/src/CorsOptions.php	New readonly value object providing type-safe, immutable storage for normalized CORS options
src/http/src/Cors.php	Refactored to use Context for coroutine-local storage instead of instance properties
src/http/src/Middleware/HandleCors.php	Moved setOptions() call from constructor to process() method for per-request configuration; added getCorsConfig() hook
tests/Http/CorsTest.php	Updated tests to work with Context-based storage; added new tests for header/method normalization, origin matching, Vary header behavior, and coroutine isolation

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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.

[binaryfire]

@albertcht This is technically true but not worth addressing in my opinion. The docblock accurately describes PHP's behavior, and setOptions() already calls array_map('strtolower', ...) on headers/methods which would fail on non-strings. Since these values come from config files (developer-controlled, not user input), any type issues would surface immediately in dev. Adding runtime validation for every array element is overkill here.

[Copilot]

The hardcoded context key should match the constant defined in the Cors class. Currently, the test uses '__cors.options' but if the Cors class constant changes, tests will silently break. Consider making the CONTEXT_KEY constant public or providing a getter method in the Cors class to access it for testing purposes.

Suggested change

	private const CORS_CONTEXT_KEY = '__cors.options';
	private const CORS_CONTEXT_KEY = Cors::CONTEXT_KEY;

[binaryfire]

@albertcht The suggested fix won't work - CONTEXT_KEY is private, so Cors::CONTEXT_KEY would be a fatal error. Making it public just for tests would leak implementation details. If someone changes the key in Cors, the tests would fail loudly anyway (options would return defaults instead of set values), so there's no silent breakage risk.

[Copilot]

The test is creating a coroutine but not waiting for it to complete before asserting the final state. This can lead to race conditions where the test completes before the coroutine executes, making the test unreliable. Add coroutine synchronization using Coroutine::create's return value and wait for completion, or use a more explicit synchronization mechanism like WaitGroup.

[binaryfire]

@albertcht Copilot misunderstands Swoole coroutines here. With RunTestsInCoroutine, the parent yields while the child runs - it doesn't return immediately like spawning a thread. The assertions inside the child throw immediately if they fail, and the assertion after verifies the parent's context wasn't polluted. That's exactly what we're testing - no synchronization needed.

[Copilot]

The test assumes that the Context in a new coroutine will have empty/default CORS options, but this isn't explicitly initialized. While the fallback in getOptionsFromContext returns a new CorsOptions(), this test validates coroutine isolation by checking the state before any explicit setOptions call. Consider explicitly documenting this assumption or initializing the context in the spawned coroutine to make the test intention clearer.

[binaryfire]

@albertcht Same as above - Copilot misunderstands Swoole coroutines here

[Copilot]

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.

Suggested change

	$this->setOptions($options);
	// Constructor options are deprecated and ignored to preserve coroutine-safe design.
	// Use setOptions() per request (e.g. in middleware) instead.
	@trigger_error(
	'Passing options to ' . __CLASS__ . '::__construct() is deprecated and ignored. '
	. 'Call setOptions() per request instead.',
	E_USER_DEPRECATED
	);

[binaryfire]

@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.

[binaryfire]

@albertcht I don't think any of Copilots suggestions are worth implementing here (and some are incorrect). But let me know if you want any changes - happy to make them.