Symfony 8 / PHP 8.5: runtime extensions, decoupled label, improved caching

.claude/agent-memory/code-improvement-reviewer/MEMORY.md

@@ -1,40 +1,45 @@
 # Menu Bundle - Code Review Memory
 
 ## Architecture
-- Namespace: `ChamberOrchestra\MenuBundle` (PSR-4 from package root, no src/)
+- Namespace: `ChamberOrchestra\MenuBundle` (PSR-4 from `src/`, package root is project root)
 - PHP ^8.5, ext-ds required
-- Legacy alias bundle: `DevMenuBundle` (BC alias for `ChamberOrchestraMenuBundle`)
-- No test suite present as of Feb 2026
+- Legacy alias bundle: `DevMenuBundle` — NOT present in current codebase (removed or never implemented — only `ChamberOrchestraMenuBundle` exists)
+- Test suite: present as of Feb 2026 (Unit + Integrational, no DI kernel test)
 
 ## Key Patterns
 - Navigation: implement `NavigationInterface::build()`, auto-tagged `chamber_orchestra_menu.navigation`
 - Factory: `Factory` applies `ExtensionInterface` plugins sorted by priority (krsort = higher int = higher priority)
 - `CoreExtension` is priority -10 (runs last, sets defaults like uri/extras/current/attributes)
-- Caching: `AbstractCachedNavigation` uses Symfony Cache Contracts, 24h TTL, tag `navigation`
-- Matching: voter-based `SplObjectStorage` cache; `RouteVoter` treats route values as regex patterns
+- Caching: `AbstractCachedNavigation` uses Symfony Cache Contracts, 24h TTL, tag `navigation`; tags ARE set via `$item->tag()`
+- Matching: voter-based `SplObjectStorage` cache; `RouteVoter` treats route values as regex patterns; null-request guard is present
 - Access: `Ds\Map` caches `isGranted()` results per item object; role grants cached in plain array too
+- `Item` uses `__serialize`/`__unserialize` (modern PHP serialization — old `Serializable` interface NOT used)
+- `MenuBuilder::children()` and `end()` both throw `LogicException` on misuse (guard clauses present)
+- `Item::getFirstChild()`/`getLastChild()` use `?: null` coercion — correct for `?self` return type
 
-## Known Issues Found (Feb 2026 review)
-See `patterns.md` for full details. Summary:
-- CRITICAL: `Matcher::isCurrent()` uses uninitialized variable `$current` — PHP TypeError if no voters
-- HIGH: `Accessor::hasAccessToChildren()` logic is wrong — returns false if ANY child is denied (should be: returns true only if AT LEAST ONE child is accessible for "show section" use-case)
-- HIGH: `RouteVoter::matchItem()` crashes with NPE if no current request (null check missing before `->attributes->get()`)
-- HIGH: `Item::getFirstChild()`/`getLastChild()` return `false` (not `null`) when collection is empty — violates `?ItemInterface` return type, causes TypeError
-- HIGH: `MenuBuilder::children()` calls `getLastChild()` which returns false — assigns false to `$this->current`, causing crash on next `add()` call
-- HIGH: `Item::serialize()`/`unserialize()` uses deprecated `Serializable` interface; `$section` not included in serialized data (lost after deserialization)
-- MEDIUM: `NavigationFactory::create()` takes untyped `$nav` parameter — should be `NavigationInterface|string`
-- MEDIUM: `MenuBuilder::end()` silently fails — if called with no parents, `array_pop` returns null and assigns null to `$this->current`
-- MEDIUM: `AbstractCachedNavigation::configureCacheItem()` does not set cache tags despite having `$this->cache['tags']`
-- MEDIUM: `Factory::addExtension()` has no return type declared
-- MEDIUM: `LabelExtension` uses `#[Required]` for optional translator injection — NPE if translator not set and translation_domain is used
-- MEDIUM: `TwigRenderer::render()` options merge order — user `$options` can be silently overridden by root/matcher/accessor keys
+## Issues Found in Feb 2026 Review (current code state)
+See `patterns.md` for full details. Summary of REMAINING issues:
+- HIGH: `TwigRenderer::render()` merge order — reserved keys `root`/`matcher`/`accessor` silently overwrite user `$options` of same name
+- HIGH: `RouteVoter::isMatchingRoute()` calls `preg_match()` with unvalidated user-supplied regex pattern — no `preg_match` error handling; malformed regex throws a PHP Warning, not a catchable exception (strict types won't help here); should use `@preg_match` + error check or `preg_match` inside try/catch with `set_error_handler`
+- HIGH: `NavigationFactory` `$built` instance cache is keyed only by class name — two different anonymous class instances of the same cached nav class share the wrong cache slot (edge case in tests, but `getCacheKey()` returns `static::class` so anonymous classes each get a unique class name — actually fine for anonymous classes, but two named-class instances with different runtime state would silently share the cached tree)
+- MEDIUM: `ClosureNavigation` wraps callable via `Closure::fromCallable()` — unnecessary since PHP 8.1+; direct `\Closure` typehint on constructor param eliminates the conversion
+- MEDIUM: `NavigationFactory::create()` passes empty `[]` options from `Helper::render()` — `$options` parameter of `create()` is unused by all non-closure navigations (AbstractNavigation ignores it); `ClosureNavigation` is the only consumer; API is misleading
+- MEDIUM: `Accessor::isGranted()` logic — if a role is cached as `true`, it `continue`s; if `false`, returns `false`; but the assignment expression `$this->grants[$role] = $isGranted = $this->authorizationChecker->isGranted($role)` is a side-effect in a condition — hard to read
+- MEDIUM: `AbstractNavigation::build()` signature has `array $options = []` default that differs from `NavigationInterface::build(array $options)` — interface has no default, abstract class adds one; inconsistency could confuse static analysis
+- MEDIUM: `services.php` excludes `Navigation/` directory from autoloading scan but navigations are user-defined outside the bundle — the exclusion is correct for the bundle's own abstract base classes, but the comment is missing, which could confuse maintainers
+- LOW: `Factory::addExtensions()` accepts `iterable<ExtensionInterface>` but `addExtension()` always uses priority 0 — callers cannot set priority via the batch method
+- LOW: `NavigationFactory::sanitizeCacheKeyPart()` replaces `.` with `_` and `\\` with `.` but the resulting key could still collide for class names that differ only in `_` vs `.` separators after transformation
+- LOW: `RouteVoter` caches `lastRequest`/`lastRoute`/`lastRouteParams` as instance state — safe for Symfony request lifecycle, but `clear()` exists only on `Matcher`, not on `RouteVoter`; stale voter state across request boundary in CLI or test contexts
+- LOW: No `NavigationFactory` unit test; no `NavigationRegistry` unit test; no `TwigRenderer` test; no `Helper`/`MenuRuntime` test; no `services.php` DI smoke test
 
 ## File Locations
-- `NavigationFactory.php` — orchestration, caching logic, $built instance cache
-- `Menu/MenuBuilder.php` — fluent tree builder (children/end state machine)
-- `Menu/Item.php` — tree node, uses ArrayCollection, implements deprecated Serializable
-- `Matcher/Matcher.php` — SplObjectStorage cache, voter chain (uninitialized $current bug)
-- `Matcher/Voter/RouteVoter.php` — regex route matching, fetches request twice
-- `Accessor/Accessor.php` — Ds\Map + array dual cache for role checks
-- `Factory/Factory.php` — priority-sorted extension chain (krsort)
-- `Resources/config/services.yml` — tagged_iterator wiring, instance tags
+- `src/NavigationFactory.php` — orchestration, in-request + PSR-6 caching
+- `src/Menu/MenuBuilder.php` — fluent tree builder (children/end now throws LogicException correctly)
+- `src/Menu/Item.php` — tree node, uses ArrayCollection, modern __serialize/__unserialize
+- `src/Matcher/Matcher.php` — SplObjectStorage cache, voter chain
+- `src/Matcher/Voter/RouteVoter.php` — regex route matching, request caching, null guard present
+- `src/Accessor/Accessor.php` — Ds\Map + array dual cache for role checks
+- `src/Factory/Factory.php` — priority-sorted extension chain (krsort)
+- `src/Resources/config/services.php` — tagged_iterator wiring, instance tags (PHP format, not YAML)
+- `src/Exception/` — LogicException, InvalidArgumentException, ExceptionInterface marker
+- `tests/Unit/` and `tests/Integrational/` — PHPUnit 13, #[Test] attributes

CLAUDE.md

@@ -51,9 +51,8 @@ composer run-script cs-check
 ### Navigation Layer (`src/Navigation/`)
 
 - `NavigationInterface` — implement `build()`, `getCacheKey()`, `configureCacheItem()`, `getCacheBeta()` to populate the builder and control caching
-- `AbstractNavigation` — minimal base class with default cache key (FQCN) and 0 TTL; two subclasses provide caching strategies:
-  - `AbstractCachedNavigation` — 24 h TTL, `chamber_orchestra_menu` tag by default; for static menu structures that survive PSR-6 cache
-  - `AbstractStaticNavigation` — 0 TTL, no tags; rebuilds every request (still deduped within same request via `NavigationFactory::$built`); for menus with dynamic data (e.g. badge closures)
+- `AbstractNavigation` — minimal base class with default cache key (FQCN) and 0 TTL
+  - `AbstractCachedNavigation` — 24 h TTL, `chamber_orchestra_menu` tag by default; for menu structures that survive PSR-6 cache
 - `ClosureNavigation` — wraps a closure as a one-off navigation (0 TTL, never cached across requests)
 
 Navigation services are auto-tagged `chamber_orchestra_menu.navigation` and resolved by `NavigationRegistry` (a `ServiceLocator` keyed by class name).
@@ -62,15 +61,15 @@ Navigation services are auto-tagged `chamber_orchestra_menu.navigation` and reso
 
 - `MenuBuilder` — fluent builder: `add(name, options, prepend, section)` → `children()` → `end()` → `build()`
 - `Factory` — creates `Item` instances; applies `ExtensionInterface` plugins sorted by priority
-- `Item` — tree node with: `name`, `label`, `uri`, `roles`, `attributes`, `badge`, children (`Collection`), `isSection()`
+- `Item` — tree node with: `name`, `label`, `uri`, `roles`, `attributes`, `badge`, `setExtra()`, children (`Collection`), `isSection()`
 
 **Item options** passed to `MenuBuilder::add()`:
 
 | Option | Extension | Effect |
 |---|---|---|
 | `route`, `route_params`, `route_type` | `RoutingExtension` | Generates `uri`; appends to `routes` array |
 | `routes` | — | Routes that activate the item (supports regex) |
-| `label`, `translation_domain` | `LabelExtension` | Label with optional Symfony translation |
+| `label` | `LabelExtension` | Display text; falls back to item name if absent |
 | `roles` | — | Security roles required to show the item |
 | `badge` | `BadgeExtension` | Badge count (`int` or `\Closure`); resolved and stored in `extras['badge']` |
 | `attributes` | `CoreExtension` | HTML attributes |
@@ -99,10 +98,13 @@ The built-in `RouteVoter` handles route-based matching.
 Implementing these interfaces is enough — no manual service config needed:
 
 - `NavigationInterface` → tagged `chamber_orchestra_menu.navigation`
-- `ExtensionInterface` → tagged `chamber_orchestra_menu.factory.extension`
+- `ExtensionInterface` → tagged `chamber_orchestra_menu.factory.extension` (build-time, results cached with tree)
+- `RuntimeExtensionInterface` → tagged `chamber_orchestra_menu.factory.runtime_extension` (post-cache, runs every request)
 
 `CoreExtension` is registered with priority `-10` (runs last, sets defaults).
 
+**Extension split:** `ExtensionInterface` runs during build and results are cached with the Item tree. `RuntimeExtensionInterface::processItem(Item)` runs after every cache fetch, walking the entire tree — use it for dynamic data like badge counts that must be fresh every request.
+
 ### Service Configuration
 
 Services are autowired and autoconfigured via `src/Resources/config/services.php`. Directories excluded from autowiring: `DependencyInjection`, `Resources`, `Exception`, `Navigation`.
@@ -121,7 +123,7 @@ Services are autowired and autoconfigured via `src/Resources/config/services.php
 
 ## Dependencies
 
-- Requires PHP 8.5, `ext-ds`, Symfony 8.0 components (`config`, `dependency-injection`, `http-foundation`, `http-kernel`, `routing`, `security-core`), Symfony contracts (`cache-contracts`, `translation-contracts`), `doctrine/collections`, `twig/twig`
+- Requires PHP 8.5, `ext-ds`, Symfony 8.0 components (`config`, `dependency-injection`, `http-foundation`, `http-kernel`, `routing`, `security-core`), `symfony/cache-contracts`, `doctrine/collections`, `twig/twig`
 - Dev: PHPUnit 13, PHPStan, `php-cs-fixer`, `symfony/cache`
 - Main branch is `main`