Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .claude/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,23 @@ Why not use key-value for `GuardInterface`? Because key-value (`GuardInterface::

**When singletons matter:** Any service that holds state across the request must be a singleton. For example, if a boot callback registers policies into a `PolicyRegistry`, that registry must be a singleton — otherwise boot writes to one instance and request handlers get a different, empty one.

### Resettable Singletons and Long-Running Processes

A singleton that caches per-request state (the current session, the authenticated user, a sticky read/write routing flag) is harmless under PHP-FPM, where the process ends after each request. Under a long-running process — a worker or event loop reusing one PHP process across many requests — the same state leaks across requests unless something clears it, which is a cross-user data leak (one user's session or identity bleeding into another user's request).

Any singleton with this shape should implement `Marko\Core\Contracts\ResettableInterface`:

```php
interface ResettableInterface
{
public function reset(): void;
}
```

`reset()` must be non-destructive — it clears the instance's in-memory per-request tracking without destroying anything persisted (e.g. resetting a session service forgets which session it was serving, it does not delete the stored session). A long-running process discovers what to reset via `Container::resolvedInstances(ResettableInterface::class)`, which returns only instances the container has already built — never triggering resolution — instead of requiring a hardcoded list. `resolvedInstances()` lives on the concrete `Container` class, not on `ContainerInterface`.

Current implementors: `Session`, `SessionGuard` (`marko/authentication`), and `ReadWriteConnection` (`marko/database-readwrite`).

### Preferences

Preferences replace one concrete class with another globally. Unlike bindings (interface → implementation), preferences swap class → class.
Expand Down
1 change: 1 addition & 0 deletions .claude/plans/response-decoration/.hook-fingerprint
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
discover-hooks-fingerprint-v1 2864d776f7830845948617a7a7473e0749f16de8b38d6d84f3a5ab64ab406895
48 changes: 48 additions & 0 deletions .claude/plans/response-decoration/001-cookie-value-object.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Task 001: Cookie Value Object

**Status**: completed
**Depends on**: none
**Retry count**: 0

## Description
Create `Marko\Routing\Http\Cookie`, an immutable value object describing a single HTTP cookie and rendering itself as a `Set-Cookie` header value. This is the foundation for first-class cookie support on `Response`; nothing else in the plan can proceed without it.

## Context
- New file: `packages/routing/src/Http/Cookie.php`
- New test: `packages/routing/tests/Http/CookieTest.php`
- Attributes needed by `Session`: name, value, expires, path, domain, secure, httpOnly, sameSite
- Follow the existing style in `packages/routing/src/Http/` — constructor property promotion, `declare(strict_types=1)`, full type declarations, no final classes
- This class MAY be `readonly` — the readonly restriction only blocks `Response`, which needs `clone`-based decoration. `Cookie` is constructed whole and never decorated.
- Loud errors: a malformed cookie name must throw, not silently emit a broken header. Add a `Cookie`-specific exception in `packages/routing/src/Exceptions/` following the existing exception style.

**Three encoding/semantics decisions that must be made in this class, not left to callers:**

1. **Value encoding.** A `Set-Cookie` value cannot contain `;`, `,`, whitespace or control characters. Session IDs happen to be safe but arbitrary application cookie values are not. Decide raw vs. `urlencode` here and cover it with a test — a caller passing `a; b` must not be able to inject a second cookie attribute.
2. **No-expiry (browser-session) cookies.** `SessionConfig::expireOnClose()` maps to `lifetime => 0`, which means a cookie with **no** `Expires` and **no** `Max-Age` attribute at all — not `Expires: <epoch>`. The `expires` attribute needs an explicit representation for "omit entirely" (`null` or `0`).
3. **`SameSite=None` requires `Secure`.** Browsers silently drop a `SameSite=None` cookie that is not `Secure`. Per the loud-errors principle this must throw at construction rather than emit a cookie that vanishes.

## Requirements (Test Descriptions)
- [x] `it renders name and value as a set-cookie string`
- [x] `it renders the path attribute when provided`
- [x] `it renders the domain attribute when provided`
- [x] `it renders expires as an rfc 7231 formatted date`
- [x] `it omits the expires attribute entirely for a browser session cookie`
- [x] `it renders secure and httponly flags only when enabled`
- [x] `it renders the samesite attribute when provided`
- [x] `it encodes a value containing characters that are illegal in a set-cookie header`
- [x] `it throws when the cookie name contains an invalid character`
- [x] `it throws when samesite is none without the secure flag`

## Acceptance Criteria
- All requirements have passing tests
- Code follows code standards
- No decrease in test coverage

## Implementation Notes
- Value encoding: `rawurlencode()` applied to the cookie value in `toSetCookieString()`. This strips `;`, `,`, whitespace, and control characters from the rendered header, preventing attribute injection from arbitrary application values while leaving safe values (e.g. session IDs) unchanged.
- No-expiry cookies: `expires` is `?int` (unix timestamp), defaulting to `null`. Both `null` and `0` are treated as "omit the `Expires` attribute entirely" — `0` is included so a raw `SessionConfig::lifetime()` value (which is `0` for expire-on-close) can be passed straight through by a future caller without conversion.
- `SameSite=None` without `secure: true` throws `CookieException::sameSiteNoneRequiresSecure()` at construction (loud errors, not a silently dropped cookie).
- Cookie name validation rejects control characters, whitespace, and RFC 2616 token separators (`()<>@,;:\"/[]?={}`) via `CookieException::invalidName()`; empty names are also rejected.
- Used a literal `gmdate()` format string (`'D, d M Y H:i:s \G\M\T'`) instead of the `DATE_RFC7231` constant, which is deprecated as of PHP 8.5.
- New exception `Marko\Routing\Exceptions\CookieException` added following the existing `MarkoException` static-factory style used by `RouteException`/`RouteConflictException`.
- Full suite (`composer test` equivalent: `pest -c phpunit.xml --parallel --exclude-group=integration-destructive`) passes: 6908 passed, 0 failed. `phpcs`, `php-cs-fixer`, and `phpstan` all clean on touched files.
72 changes: 72 additions & 0 deletions .claude/plans/response-decoration/002-response-decoration-api.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Task 002: Response Decoration API

**Status**: completed
**Depends on**: 001
**Retry count**: 0

## Description
Give `Response` a decoration API (`withHeader()`, `withHeaders()`, `withStatus()`, `withCookie()`) that returns modified copies while preserving the concrete subclass, and add a cookie collection alongside the existing headers. This is the change that fixes the `StreamingResponse` downgrade at its root.

## Context
- Modify: `packages/routing/src/Http/Response.php`
- Modify: `packages/sse/src/StreamingResponse.php` (must also drop the `readonly` class modifier so `clone` works through the hierarchy)
- Tests: `packages/routing/tests/Http/ResponseTest.php`

**Verified on PHP 8.5.1 — read this before implementing.** `clone $this with { ... }` does not exist in 8.5.1 (parse error). Modifying a readonly property on a clone fails both by direct assignment and via `ReflectionProperty::setValue`. The only approach that works is: **plain class (no `readonly` class modifier), private non-readonly properties, `clone $this` then assign on the clone.**

- MUST use `clone $this`, never `new static(...)`. `StreamingResponse::__construct(SseStream $stream, int $statusCode)` has a different signature from its parent, so `new static(...)` cannot reconstruct it. `clone` copies all subclass state automatically — this is the entire mechanism behind the SSE fix.
- Return type on `with*()` methods must be `static`, not `self`.
- Immutability is now enforced by API design rather than the `readonly` keyword: properties stay private and no setters are added. Every `with*()` needs a test asserting the original instance is unchanged.
- Cookies are a SEPARATE collection (`list<Cookie>`), NOT multi-valued headers. `headers()` must keep its `array<string, string>` return type so every existing `->headers()` call site stays source-compatible (eight in production code, ~80 across the test suites).
- The 3-arg constructor and `json()` / `html()` / `redirect()` must remain source-compatible.

### Required public surface (other tasks build against this — do not rename)

```php
public function withHeader(string $name, string $value): static;
public function withHeaders(array $headers): static; // array<string, string>, merged over existing
public function withStatus(int $statusCode): static;
public function withCookie(Cookie $cookie): static;
public function cookies(): array; // list<Cookie>
```

- **`withStatus()` is not optional.** `InertiaMiddleware.php:55` rebuilds the response specifically to change 302 → 303 for PUT/PATCH/DELETE. Without `withStatus()`, task 004 is hard-blocked on that branch.
- **`withHeaders()` is not optional either.** `SecurityHeadersMiddleware`, both `CorsMiddleware`s and `RateLimitMiddleware` all apply a *map* of headers via `array_merge`. A bulk method keeps task 004 a mechanical one-line change instead of a hand-unrolled chain.
- **`cookies()` is the accessor tasks 003 and 005 consume.** Task 003 needs it to build `Set-Cookie` lines; task 005 needs it to refuse caching. Name it exactly as above.
- **Duplicate-cookie semantics: replace, don't append.** `withCookie()` with a cookie matching an existing one on (name, path, domain) REPLACES it. This mirrors `withHeader()` and avoids emitting two competing `Set-Cookie` lines for the same name. Cookies differing in name, path, or domain accumulate.

### Do NOT convert the named constructors

`json()`, `html()` and `redirect()` stay `static ...: self` using `new self(...)`. Do **not** "improve" them to `new static(...)` for consistency with the `with*()` methods — `StreamingResponse`'s constructor cannot accept `(body:, statusCode:, headers:)`, so `StreamingResponse::json()` would fatal at runtime. That signature mismatch is precisely why `clone` is required. Late-static-bound named constructors are a separate follow-up, out of scope here.

## Requirements (Test Descriptions)
- [x] `it returns a new instance from withHeader leaving the original unchanged`
- [x] `it merges the new header into the existing headers`
- [x] `it merges a map of headers with withHeaders`
- [x] `it returns a new instance from withStatus leaving the original unchanged`
- [x] `it preserves the concrete subclass when decorating with a header`
- [x] `it preserves the concrete subclass when decorating with a status`
- [x] `it preserves subclass state such as the streaming payload when decorating`
- [x] `it still streams from a decorated streaming response`
- [x] `it does not clobber the sse headers when adding a header to a streaming response`
- [x] `it returns a new instance from withCookie leaving the original unchanged`
- [x] `it accumulates cookies that differ in name path or domain`
- [x] `it replaces a cookie matching an existing name path and domain`
- [x] `it keeps cookies out of the headers collection`

## Acceptance Criteria
- All requirements have passing tests
- `Response::json()`, `html()`, `redirect()` and the 3-arg constructor still work unchanged, and still use `new self()`
- `packages/sse/src/StreamingResponse.php` drops the `readonly` class modifier and its existing tests stay green
- Code follows code standards
- No decrease in test coverage

## Implementation Notes

- `Response` converted from `readonly class` to a plain `class` with a class-level docblock explaining why (clone-then-assign is required; readonly blocks it on PHP 8.5.1). Added `withHeader()`, `withHeaders()`, `withStatus()`, `withCookie()` (all returning `static`, all `clone $this` then assign on the clone) plus a private `list<Cookie> $cookies` property and `cookies(): array` accessor.
- `withCookie()` uses `array_find_key()` to locate a cookie matching on (name, path, domain) and replaces it in place; otherwise appends.
- `Cookie` (still `readonly class`, unchanged otherwise) gained narrow public accessors `name()`, `path()`, `domain()` — the minimum needed for `withCookie()`'s replace-matching logic. Added dedicated tests for these accessors in `CookieTest.php`, including a defaults-to-null case.
- `StreamingResponse` lost the `readonly` class modifier only; its constructor and `send()` are unchanged. `clone` now naturally carries over the private `$stream` property.
- Most requirements after the first (`withHeader` unchanged-original test) passed immediately on writing the test — the single structural change (clone-based `with*()` methods) needed for requirement 1 already implemented the full behavior for `withHeaders`, `withStatus`, `withCookie`, subclass preservation, and header non-clobbering. This was expected and noted per requirement rather than treated as a problem.
- `it still streams from a decorated streaming response` could not use `ob_start()`/`ob_get_clean()` to capture `send()`'s output: `StreamingResponse::send()` runs `while (ob_get_level() > 0) { ob_end_flush(); }` before echoing stream chunks, which forcibly closes any buffer the test itself started (including PHPUnit's), so chunks are echoed with no active buffer to capture them in-process. Test spawns a real PHP subprocess via `proc_open()` (using the project's `vendor/autoload.php`) and asserts on its actual stdout instead.
- Full suite: `composer test` → 6927 passed, 0 failed. `composer phpstan` → 0 errors project-wide. `phpcs` clean on all touched files after one `phpcbf` auto-fix (multi-line method signature) on `Response::withHeader()`.
38 changes: 38 additions & 0 deletions .claude/plans/response-decoration/003-header-line-emission.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Task 003: Header Line Emission and Cookie-Aware send()

**Status**: completed
**Depends on**: 002
**Retry count**: 0

## Description
Extract response header emission into a testable method that returns the complete set of outbound header lines including one `Set-Cookie` per cookie, and reduce `send()` to a thin loop over it. `header()` is unobservable under the CLI SAPI, so emission logic cannot be tested while it lives inside `send()`.

## Context
- Modify: `packages/routing/src/Http/Response.php` (`send()` at line 69)
- Modify: `packages/sse/src/StreamingResponse.php` (overrides `send()` and duplicates the header loop at lines 35-40 — it must consume the same method)
- Tests: `packages/routing/tests/Http/ResponseTest.php`
- This seam is also what the future RoadRunner bridge (#151) will consume to map a Marko response onto a PSR-7 response, so keep it free of any SAPI calls.
- `send()` keeps its existing `headers_sent()` guard and `http_response_code()` call; only the header-line construction moves.
- The extracted method reads `headers()` and `cookies()` (the accessor defined in task 002) and returns a `list<string>` of complete header lines. Name it `headerLines(): array` — task 006's tests assert against it by name.
- Emit regular headers first, then one `Set-Cookie` line per cookie, in insertion order.

## Requirements (Test Descriptions)
- [x] `it returns regular headers as name colon value lines`
- [x] `it returns a distinct set-cookie line for each cookie`
- [x] `it returns only regular header lines when the response has no cookies`
- [x] `it preserves cookie order in the emitted lines`
- [x] `it emits the same header lines for a streaming response subclass`

## Acceptance Criteria
- All requirements have passing tests
- No SAPI functions (`header()`, `setcookie()`) are called by the extracted method
- Code follows code standards
- No decrease in test coverage

## Implementation Notes
- Added `Response::headerLines(): array` (returns `list<string>`) that emits regular headers as `"Name: value"` lines followed by one `"Set-Cookie: ..."` line per cookie (via `Cookie::toSetCookieString()`), in insertion order. Contains no SAPI calls.
- `Response::send()` and `StreamingResponse::send()` were reduced to loop `header($line)` over `headerLines()` instead of duplicating the header-construction logic.
- Requirements 3, 4, and 5 passed immediately once requirements 1-2 were implemented (order preservation and no-cookie behavior fall out naturally from the simple two-loop implementation; `headerLines()` is inherited unchanged by `StreamingResponse` since it is not overridden). Noted as expected, not over-implementation, since each test asserts a genuinely distinct requirement.
- The streaming-response test (`it emits the same header lines for a streaming response subclass`) was added to `packages/sse/tests/StreamingResponseTest.php` rather than `packages/routing/tests/Http/ResponseTest.php`, because `packages/routing` does not depend on `packages/sse` (dependency direction is the reverse) and `StreamingResponse` lives in the `sse` package.
- Verified: `composer test` (6932 passed), `composer phpstan` (0 errors), `./vendor/bin/phpcs` on touched files (0 errors), `php-cs-fixer fix` on touched files (only a pre-existing brace-placement fix unrelated to this change).

Loading
Loading