diff --git a/.claude/agent-memory/code-improvement-reviewer/MEMORY.md b/.claude/agent-memory/code-improvement-reviewer/MEMORY.md index 9bc8ef0..ddd108e 100644 --- a/.claude/agent-memory/code-improvement-reviewer/MEMORY.md +++ b/.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` 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 diff --git a/CLAUDE.md b/CLAUDE.md index 651a9f4..004a224 100644 --- a/CLAUDE.md +++ b/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,7 +61,7 @@ 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()`: @@ -70,7 +69,7 @@ Navigation services are auto-tagged `chamber_orchestra_menu.navigation` and reso |---|---|---| | `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` diff --git a/README.md b/README.md index d067c58..977b94e 100644 --- a/README.md +++ b/README.md @@ -1,11 +1,14 @@ # ChamberOrchestra MenuBundle -[![PHP](https://img.shields.io/badge/PHP-8.5%2B-8892BF?logo=php)](https://php.net) -[![Symfony](https://img.shields.io/badge/Symfony-8.0%2B-000000?logo=symfony)](https://symfony.com) -[![License](https://img.shields.io/badge/License-MIT-blue.svg)](LICENSE) -[![CI](https://github.com/chamber-orchestra/menu-bundle/actions/workflows/php.yml/badge.svg)](https://github.com/chamber-orchestra/menu-bundle/actions/workflows/php.yml) +[![PHP Composer](https://github.com/chamber-orchestra/menu-bundle/actions/workflows/php.yml/badge.svg)](https://github.com/chamber-orchestra/menu-bundle/actions/workflows/php.yml) +[![PHPStan](https://img.shields.io/badge/PHPStan-max-brightgreen.svg)](https://phpstan.org/) +[![PHP-CS-Fixer](https://img.shields.io/badge/code%20style-PER--CS%20%2F%20Symfony-blue.svg)](https://cs.symfony.com/) +[![Latest Stable Version](https://img.shields.io/packagist/v/chamber-orchestra/menu-bundle.svg)](https://packagist.org/packages/chamber-orchestra/menu-bundle) +[![License: MIT](https://img.shields.io/badge/License-MIT-blue.svg)](LICENSE) +[![PHP 8.5+](https://img.shields.io/badge/PHP-8.5%2B-777BB4.svg)](https://www.php.net/) +[![Symfony 8.0](https://img.shields.io/badge/Symfony-8.0-000000.svg)](https://symfony.com/) -A **Symfony 8** bundle for building navigation menus and sidebars — fluent tree builder, route-based active-item matching, role-based access control, PSR-6 tag-aware caching, and Twig rendering. +A **Symfony 8** bundle for building navigation menus, sidebars, and breadcrumbs — fluent tree builder, route-based active-item matching, role-based access control, runtime extensions for dynamic badges, PSR-6 tag-aware caching, and Twig rendering. --- @@ -14,10 +17,11 @@ A **Symfony 8** bundle for building navigation menus and sidebars — fluent tre - **Fluent builder API** — `add()`, `children()`, `end()` for deeply-nested trees - **Route-based matching** — `RouteVoter` marks the current item and its ancestors active; route values are treated as regex patterns - **Role-based access** — `Accessor` gates items by Symfony security roles; results are memoized per request -- **PSR-6 caching** — `AbstractCachedNavigation` caches the item tree for 24 h with tag-based invalidation; `AbstractStaticNavigation` rebuilds every request for dynamic content -- **Badge support** — attach dynamic counts to items via `int` or `\Closure`; closures are resolved at build time +- **PSR-6 caching** — `AbstractCachedNavigation` caches the item tree for 24 h with tag-based invalidation +- **Runtime extensions** — `RuntimeExtensionInterface` runs post-cache on every request for fresh dynamic data without rebuilding the tree +- **Badge support** — `BadgeExtension` resolves `int` and `\Closure` badges at runtime; implement `RuntimeExtensionInterface` for service-injected dynamic badges - **Twig integration** — `render_menu()` function with fully customisable templates -- **Extension system** — plug in custom `ExtensionInterface` to enrich item options before creation +- **Extension system** — build-time `ExtensionInterface` for cached option enrichment, runtime `RuntimeExtensionInterface` for post-cache processing - **DI autoconfiguration** — implement an interface, done; no manual service tags required --- @@ -81,7 +85,7 @@ final class SidebarNavigation extends AbstractCachedNavigation } ``` -The class is auto-tagged as a navigation service — no YAML service definition needed. +The class is auto-tagged as a navigation service — no YAML/XML service definition needed. ### 2. Create a Twig template @@ -109,19 +113,18 @@ The class is auto-tagged as a navigation service — no YAML service definition Options are passed as the second argument to `MenuBuilder::add()`: -| Option | Type | Extension | Description | -|---|---|---|---| -| `label` | `string` | `LabelExtension` | Display text; falls back to item name if absent | -| `translation_domain` | `string` | `LabelExtension` | Symfony translation domain for the label | -| `route` | `string` | `RoutingExtension` | Route name; generates `uri` and appends to `routes` | -| `route_params` | `array` | `RoutingExtension` | Route parameters passed to the URL generator | -| `route_type` | `int` | `RoutingExtension` | `UrlGeneratorInterface::ABSOLUTE_PATH` (default) or `ABSOLUTE_URL` | -| `routes` | `array` | — | Additional routes that activate this item (supports regex) | -| `uri` | `string` | — | Raw URI; set directly if not using `route` | -| `roles` | `array` | — | Security roles **all** required to display the item (AND logic) | -| `badge` | `int\|\Closure` | `BadgeExtension` | Badge count; closures are resolved at build time; stored in `extras['badge']` | -| `attributes` | `array` | `CoreExtension` | HTML attributes merged onto the rendered element | -| `extras` | `array` | `CoreExtension` | Arbitrary extra data attached to the item | +| Option | Type | Description | +|---|---|---| +| `label` | `string` | Display text; falls back to item name if absent (`LabelExtension`) | +| `route` | `string` | Route name; generates `uri` and appends to `routes` (`RoutingExtension`) | +| `route_params` | `array` | Route parameters passed to the URL generator (`RoutingExtension`) | +| `route_type` | `int` | `UrlGeneratorInterface::ABSOLUTE_PATH` (default) or `ABSOLUTE_URL` (`RoutingExtension`) | +| `routes` | `array` | Additional routes that activate this item (supports regex) | +| `uri` | `string` | Raw URI; set directly if not using `route` | +| `roles` | `array` | Security roles **all** required to display the item (AND logic) | +| `badge` | `int\|\Closure` | Badge count; resolved post-cache by `BadgeExtension` (a runtime extension); stored in `extras['badge']` | +| `attributes` | `array` | HTML attributes merged onto the rendered element (`CoreExtension`) | +| `extras` | `array` | Arbitrary extra data attached to the item (`CoreExtension`) | ### Section items @@ -143,16 +146,17 @@ Navigation classes form a hierarchy — extend the one that fits your use case: ``` AbstractNavigation (base: 0 TTL, no tags) -├── AbstractCachedNavigation (24 h TTL, 'chamber_orchestra_menu' tag) -└── AbstractStaticNavigation (0 TTL, no tags — explicit non-cached) +└── AbstractCachedNavigation (24 h TTL, 'chamber_orchestra_menu' tag) ``` | Base class | TTL | Tags | Use case | |---|---|---|---| -| `AbstractCachedNavigation` | 24 h | `chamber_orchestra_menu` | Static menu structures | -| `AbstractStaticNavigation` | 0 | none | Menus with dynamic data (badges, counters) | +| `AbstractCachedNavigation` | 24 h | `chamber_orchestra_menu` | Menu structures (recommended) | +| `AbstractNavigation` | 0 | none | Base class, no caching across requests | -Both are deduped within the same request via `NavigationFactory`. When a PSR-6 `CacheInterface` (tag-aware) is wired in, `AbstractCachedNavigation` stores the tree across requests. Without one, an in-memory `ArrayAdapter` is used automatically. +All navigations are deduped within the same request via `NavigationFactory`. When a PSR-6 `CacheInterface` (tag-aware) is wired in, `AbstractCachedNavigation` stores the tree across requests. Without one, an in-memory `ArrayAdapter` is used automatically. + +Dynamic data (badges, counters) does not require sacrificing the cache — use runtime extensions instead. ```php add('news', ['label' => 'News', 'badge' => 3]) + ->add('inbox', ['label' => 'Inbox', 'badge' => fn (): int => $this->messages->countUnread()]); +``` + +### Via a custom runtime extension + +For service-injected dynamic data, implement `RuntimeExtensionInterface`. The tree stays cached; the extension runs post-cache on every request: ```php add('inbox', [ - 'label' => 'Inbox', - 'route' => 'app_inbox', - 'badge' => fn (): int => $this->messages->countUnread(), - ]); + if ('inbox' === $item->getName()) { + $item->setExtra('badge', $this->messages->countUnread()); + } } } ``` @@ -271,7 +284,9 @@ In Twig, read the badge via `item.badge`: ## Factory Extensions -Implement `ExtensionInterface` to enrich item options before the `Item` is created. Extensions are auto-tagged and sorted by `priority` (higher runs first; `CoreExtension` runs last at `-10`): +### Build-time extensions (cached) + +Implement `ExtensionInterface` to enrich item options before the `Item` is created. Results are cached with the tree. Extensions are auto-tagged and sorted by `priority` (higher runs first; `CoreExtension` runs last at `-10`): ```php use ChamberOrchestra\MenuBundle\Factory\Extension\ExtensionInterface; @@ -288,9 +303,30 @@ final class IconExtension implements ExtensionInterface } ``` +### Runtime extensions (post-cache) + +Implement `RuntimeExtensionInterface` to apply fresh data after every cache fetch. `processItem()` is called on every `Item` in the tree: + +```php +use ChamberOrchestra\MenuBundle\Factory\Extension\RuntimeExtensionInterface; +use ChamberOrchestra\MenuBundle\Menu\Item; + +final class NotificationBadgeExtension implements RuntimeExtensionInterface +{ + public function __construct(private readonly NotificationRepository $notifications) {} + + public function processItem(Item $item): void + { + if ('alerts' === $item->getName()) { + $item->setExtra('badge', $this->notifications->countUnread()); + } + } +} +``` + --- -## DI Auto-configuration +## DI Autoconfiguration Implement an interface and you're done — no manual service tags required: @@ -298,6 +334,7 @@ Implement an interface and you're done — no manual service tags required: |---|---| | `NavigationInterface` | `chamber_orchestra_menu.navigation` | | `ExtensionInterface` | `chamber_orchestra_menu.factory.extension` | +| `RuntimeExtensionInterface` | `chamber_orchestra_menu.factory.runtime_extension` | --- diff --git a/composer.json b/composer.json index 024a982..c1ea780 100644 --- a/composer.json +++ b/composer.json @@ -1,12 +1,12 @@ { "name": "chamber-orchestra/menu-bundle", "type": "symfony-bundle", - "description": "Symfony 8 bundle for building navigation menus — fluent tree builder, route-based active matching, role-based access control, PSR-6 tag-aware caching, and Twig rendering", + "description": "Symfony 8 navigation menu bundle with fluent tree builder, route-based active-item matching, role-based access control, runtime extensions for dynamic badges, PSR-6 tag-aware caching, and Twig rendering", "keywords": [ "symfony", "symfony-bundle", - "symfony8", - "php8", + "symfony-8", + "php-8", "menu", "menu-bundle", "menu-builder", @@ -15,15 +15,25 @@ "navigation-menu", "navbar", "sidebar", + "breadcrumb", "tree-menu", "menu-item", "active-menu", + "route-matching", + "badge", + "dynamic-badge", "twig", + "twig-extension", "voter", + "psr-6", "caching", + "tag-aware-cache", "role-based-access", "access-control", "authorization", + "fluent-api", + "builder-pattern", + "runtime-extension", "php" ], "license": "MIT", @@ -57,7 +67,6 @@ "symfony/http-kernel": "^8.0", "symfony/routing": "^8.0", "symfony/security-core": "^8.0", - "symfony/translation-contracts": "^3.4", "twig/twig": "^3.0" }, "require-dev": { @@ -76,8 +85,7 @@ "Tests\\": "tests/" } }, - "minimum-stability": "dev", - "prefer-stable": true, + "minimum-stability": "stable", "config": { "sort-packages": true }, diff --git a/src/Factory/Extension/BadgeExtension.php b/src/Factory/Extension/BadgeExtension.php index 91a8481..ee487f5 100644 --- a/src/Factory/Extension/BadgeExtension.php +++ b/src/Factory/Extension/BadgeExtension.php @@ -4,27 +4,18 @@ namespace ChamberOrchestra\MenuBundle\Factory\Extension; -class BadgeExtension implements ExtensionInterface +use ChamberOrchestra\MenuBundle\Menu\Item; + +class BadgeExtension implements RuntimeExtensionInterface { - /** - * @param array $options - * - * @return array - */ - public function buildOptions(array $options): array + public function processItem(Item $item): void { - if (!\array_key_exists('badge', $options)) { - return $options; - } + $badge = $item->getOption('badge'); - $badge = $options['badge']; - unset($options['badge']); - - /** @var array $extras */ - $extras = $options['extras'] ?? []; - $extras['badge'] = $badge instanceof \Closure ? $badge() : $badge; - $options['extras'] = $extras; + if (null === $badge) { + return; + } - return $options; + $item->setExtra('badge', $badge instanceof \Closure ? $badge() : $badge); } } diff --git a/src/Factory/Extension/CoreExtension.php b/src/Factory/Extension/CoreExtension.php index c9e4ee1..e07778b 100644 --- a/src/Factory/Extension/CoreExtension.php +++ b/src/Factory/Extension/CoreExtension.php @@ -4,6 +4,9 @@ namespace ChamberOrchestra\MenuBundle\Factory\Extension; +use Symfony\Component\DependencyInjection\Attribute\AutoconfigureTag; + +#[AutoconfigureTag('chamber_orchestra_menu.factory.extension', ['priority' => -10])] class CoreExtension implements ExtensionInterface { /** diff --git a/src/Factory/Extension/LabelExtension.php b/src/Factory/Extension/LabelExtension.php index 6e22c50..b2aef30 100644 --- a/src/Factory/Extension/LabelExtension.php +++ b/src/Factory/Extension/LabelExtension.php @@ -4,14 +4,8 @@ namespace ChamberOrchestra\MenuBundle\Factory\Extension; -use Symfony\Contracts\Translation\TranslatorInterface; - class LabelExtension implements ExtensionInterface { - public function __construct(private readonly TranslatorInterface $translator) - { - } - /** * @param array $options * @@ -25,14 +19,6 @@ public function buildOptions(array $options = []): array $options['label'] = $key; } - if (isset($options['translation_domain'])) { - /** @var string $label */ - $label = $options['label']; - /** @var string $domain */ - $domain = $options['translation_domain']; - $options['label'] = $this->translator->trans($label, [], $domain); - } - return $options; } } diff --git a/src/Factory/Extension/RuntimeExtensionInterface.php b/src/Factory/Extension/RuntimeExtensionInterface.php new file mode 100644 index 0000000..de0f783 --- /dev/null +++ b/src/Factory/Extension/RuntimeExtensionInterface.php @@ -0,0 +1,15 @@ +section; } + public function setExtra(string $key, mixed $value): self + { + /** @var array $extras */ + $extras = $this->options['extras'] ?? []; + $extras[$key] = $value; + $this->options['extras'] = $extras; + + return $this; + } + public function getBadge(): ?int { /** @var array $extras */ diff --git a/src/Navigation/AbstractStaticNavigation.php b/src/Navigation/AbstractStaticNavigation.php deleted file mode 100644 index 15b8938..0000000 --- a/src/Navigation/AbstractStaticNavigation.php +++ /dev/null @@ -1,15 +0,0 @@ -expiresAfter(0); - } -} diff --git a/src/NavigationFactory.php b/src/NavigationFactory.php index 8396943..f8e9ef4 100644 --- a/src/NavigationFactory.php +++ b/src/NavigationFactory.php @@ -4,7 +4,9 @@ namespace ChamberOrchestra\MenuBundle; +use ChamberOrchestra\MenuBundle\Factory\Extension\RuntimeExtensionInterface; use ChamberOrchestra\MenuBundle\Factory\Factory; +use ChamberOrchestra\MenuBundle\Menu\Item; use ChamberOrchestra\MenuBundle\Menu\MenuBuilder; use ChamberOrchestra\MenuBundle\Navigation\NavigationInterface; use ChamberOrchestra\MenuBundle\Registry\NavigationRegistry; @@ -15,13 +17,15 @@ class NavigationFactory { - /** @var array */ + /** @var array */ private array $built = []; /** @var array */ private array $options = [ 'namespace' => '$NAVIGATION$', ]; private readonly CacheInterface $cache; + /** @var list */ + private array $runtimeExtensions = []; /** * @param array $options @@ -30,7 +34,7 @@ public function __construct( private readonly NavigationRegistry $registry, private readonly Factory $factory, ?CacheInterface $cache = null, - array $options = [] + array $options = [], ) { $this->cache = $cache ?? new TagAwareAdapter(new ArrayAdapter()); $this->options = \array_replace($this->options, $options); @@ -41,7 +45,7 @@ public function __construct( * * @throws \Psr\Cache\InvalidArgumentException */ - public function create(NavigationInterface|string $nav, array $options): Menu\Item + public function create(NavigationInterface|string $nav, array $options): Item { if (\is_string($nav)) { $nav = $this->registry->get($nav); @@ -55,7 +59,7 @@ public function create(NavigationInterface|string $nav, array $options): Menu\It $cached = $this->cache->get( $this->createCacheKey($nav), - function (ItemInterface $item) use ($nav, $options): Menu\Item { + function (ItemInterface $item) use ($nav, $options): Item { $nav->configureCacheItem($item); $builder = $this->createNewBuilder(); @@ -66,9 +70,32 @@ function (ItemInterface $item) use ($nav, $options): Menu\Item { $nav->getCacheBeta(), ); + $this->applyRuntimeExtensions($cached); + return $this->built[$key] = $cached; } + /** + * @param iterable $extensions + */ + public function addRuntimeExtensions(iterable $extensions): void + { + foreach ($extensions as $extension) { + $this->runtimeExtensions[] = $extension; + } + } + + private function applyRuntimeExtensions(Item $item): void + { + foreach ($this->runtimeExtensions as $extension) { + $extension->processItem($item); + } + + foreach ($item as $child) { + $this->applyRuntimeExtensions($child); + } + } + private function createNewBuilder(): MenuBuilder { return new MenuBuilder($this->factory); diff --git a/src/Resources/config/services.php b/src/Resources/config/services.php index 2958f78..2933322 100644 --- a/src/Resources/config/services.php +++ b/src/Resources/config/services.php @@ -4,12 +4,12 @@ namespace Symfony\Component\DependencyInjection\Loader\Configurator; -use ChamberOrchestra\MenuBundle\Factory\Extension\BadgeExtension; -use ChamberOrchestra\MenuBundle\Factory\Extension\CoreExtension; use ChamberOrchestra\MenuBundle\Factory\Extension\ExtensionInterface; +use ChamberOrchestra\MenuBundle\Factory\Extension\RuntimeExtensionInterface; use ChamberOrchestra\MenuBundle\Factory\Factory; use ChamberOrchestra\MenuBundle\Matcher\Matcher; use ChamberOrchestra\MenuBundle\Navigation\NavigationInterface; +use ChamberOrchestra\MenuBundle\NavigationFactory; return static function (ContainerConfigurator $container): void { $services = $container->services() @@ -21,6 +21,9 @@ ->instanceof(ExtensionInterface::class) ->tag('chamber_orchestra_menu.factory.extension') + ->instanceof(RuntimeExtensionInterface::class) + ->tag('chamber_orchestra_menu.factory.runtime_extension') + ->instanceof(NavigationInterface::class) ->tag('chamber_orchestra_menu.navigation') ; @@ -31,10 +34,8 @@ $services->set(Factory::class) ->call('addExtensions', [\tagged_iterator('chamber_orchestra_menu.factory.extension')]); - $services->set(BadgeExtension::class); - - $services->set(CoreExtension::class) - ->tag('chamber_orchestra_menu.factory.extension', ['priority' => -10]); + $services->set(NavigationFactory::class) + ->call('addRuntimeExtensions', [\tagged_iterator('chamber_orchestra_menu.factory.runtime_extension')]); $services->set(Matcher::class) ->call('addVoters', [\tagged_iterator('chamber_orchestra_menu.matcher.voter')]); diff --git a/tests/Integrational/NavigationBuildTest.php b/tests/Integrational/NavigationBuildTest.php index a6b2354..3560b6c 100644 --- a/tests/Integrational/NavigationBuildTest.php +++ b/tests/Integrational/NavigationBuildTest.php @@ -13,7 +13,6 @@ use ChamberOrchestra\MenuBundle\Registry\NavigationRegistry; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; -use Symfony\Contracts\Translation\TranslatorInterface; /** * Tests the full pipeline: Navigation → MenuBuilder → Item tree, using real classes. @@ -25,11 +24,8 @@ final class NavigationBuildTest extends TestCase protected function setUp(): void { - $translator = $this->createStub(TranslatorInterface::class); - $translator->method('trans')->willReturnArgument(0); // identity translator - $this->factory = new Factory(); - $this->factory->addExtension(new LabelExtension($translator), priority: 10); + $this->factory->addExtension(new LabelExtension(), priority: 10); $this->factory->addExtension(new CoreExtension(), priority: -10); $registry = $this->createStub(NavigationRegistry::class); diff --git a/tests/Integrational/RuntimeExtensionTest.php b/tests/Integrational/RuntimeExtensionTest.php new file mode 100644 index 0000000..5d53813 --- /dev/null +++ b/tests/Integrational/RuntimeExtensionTest.php @@ -0,0 +1,181 @@ +factory = new Factory(); + $this->registry = $this->createStub(NavigationRegistry::class); + $this->cache = new TagAwareAdapter(new ArrayAdapter()); + } + + #[Test] + public function runtimeExtensionAppliesBadgeToItem(): void + { + $nav = new class extends AbstractCachedNavigation { + public function build(MenuBuilder $builder, array $options = []): void + { + $builder->add('inbox', ['label' => 'Inbox']); + } + }; + + $runtimeExt = new class implements RuntimeExtensionInterface { + public function processItem(Item $item): void + { + if ('inbox' === $item->getName()) { + $item->setExtra('badge', 42); + } + } + }; + + $navFactory = $this->makeFactory($this->cache); + $navFactory->addRuntimeExtensions([$runtimeExt]); + $root = $navFactory->create($nav, []); + + self::assertSame(42, $root->getFirstChild()->getBadge()); + } + + #[Test] + public function runtimeExtensionRunsOnEveryRequest(): void + { + $counter = new class { + public int $count = 0; + }; + + $nav = new class extends AbstractCachedNavigation { + public int $buildCount = 0; + + public function build(MenuBuilder $builder, array $options = []): void + { + ++$this->buildCount; + $builder->add('scores', ['label' => 'Scores']); + } + }; + + $runtimeExt = new class($counter) implements RuntimeExtensionInterface { + public function __construct(private readonly object $counter) + { + } + + public function processItem(Item $item): void + { + if ('scores' === $item->getName()) { + ++$this->counter->count; + $item->setExtra('badge', $this->counter->count); + } + } + }; + + // First request + $factory1 = $this->makeFactory($this->cache); + $factory1->addRuntimeExtensions([$runtimeExt]); + $root1 = $factory1->create($nav, []); + + // Second request (new factory, same PSR-6 cache) + $factory2 = $this->makeFactory($this->cache); + $factory2->addRuntimeExtensions([$runtimeExt]); + $root2 = $factory2->create($nav, []); + + self::assertSame(1, $nav->buildCount, 'Tree must be built only once (cached)'); + self::assertSame(1, $root1->getFirstChild()->getBadge()); + self::assertSame(2, $root2->getFirstChild()->getBadge()); + } + + #[Test] + public function runtimeExtensionAppliesToNestedChildren(): void + { + $nav = new class extends AbstractCachedNavigation { + public function build(MenuBuilder $builder, array $options = []): void + { + $builder + ->add('compositions', ['label' => 'Compositions']) + ->children() + ->add('rehearsals', ['label' => 'Rehearsals']) + ->end(); + } + }; + + $runtimeExt = new class implements RuntimeExtensionInterface { + public function processItem(Item $item): void + { + if ('rehearsals' === $item->getName()) { + $item->setExtra('badge', 5); + } + } + }; + + $navFactory = $this->makeFactory(); + $navFactory->addRuntimeExtensions([$runtimeExt]); + $root = $navFactory->create($nav, []); + + $rehearsals = $root->getFirstChild()->getFirstChild(); + self::assertSame(5, $rehearsals->getBadge()); + } + + #[Test] + public function multipleRuntimeExtensionsAreApplied(): void + { + $nav = new class extends AbstractCachedNavigation { + public function build(MenuBuilder $builder, array $options = []): void + { + $builder + ->add('inbox', ['label' => 'Inbox']) + ->add('instruments', ['label' => 'Instruments']); + } + }; + + $ext1 = new class implements RuntimeExtensionInterface { + public function processItem(Item $item): void + { + if ('inbox' === $item->getName()) { + $item->setExtra('badge', 10); + } + } + }; + + $ext2 = new class implements RuntimeExtensionInterface { + public function processItem(Item $item): void + { + if ('instruments' === $item->getName()) { + $item->setExtra('badge', 7); + } + } + }; + + $navFactory = $this->makeFactory(); + $navFactory->addRuntimeExtensions([$ext1, $ext2]); + $root = $navFactory->create($nav, []); + + self::assertSame(10, $root->getFirstChild()->getBadge()); + self::assertSame(7, $root->getLastChild()->getBadge()); + } + + private function makeFactory(?CacheInterface $cache = null): NavigationFactory + { + return new NavigationFactory($this->registry, $this->factory, $cache); + } +} diff --git a/tests/Unit/Factory/Extension/BadgeExtensionTest.php b/tests/Unit/Factory/Extension/BadgeExtensionTest.php index 3241a5b..ed943e7 100644 --- a/tests/Unit/Factory/Extension/BadgeExtensionTest.php +++ b/tests/Unit/Factory/Extension/BadgeExtensionTest.php @@ -5,6 +5,7 @@ namespace Tests\Unit\Factory\Extension; use ChamberOrchestra\MenuBundle\Factory\Extension\BadgeExtension; +use ChamberOrchestra\MenuBundle\Menu\Item; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; @@ -18,56 +19,60 @@ protected function setUp(): void } #[Test] - public function returnsOptionsUnchangedWhenBadgeMissing(): void + public function skipsItemWithoutBadgeOption(): void { - $options = ['label' => 'Scores']; + $item = new Item('scores', ['label' => 'Scores']); + $this->ext->processItem($item); - self::assertSame($options, $this->ext->buildOptions($options)); + self::assertNull($item->getBadge()); } #[Test] public function intBadgeIsMovedToExtras(): void { - $result = $this->ext->buildOptions(['badge' => 5]); + $item = new Item('scores', ['badge' => 5]); + $this->ext->processItem($item); - self::assertArrayNotHasKey('badge', $result); - self::assertSame(5, $result['extras']['badge']); + self::assertSame(5, $item->getBadge()); } #[Test] public function closureBadgeIsResolvedAndMovedToExtras(): void { - $result = $this->ext->buildOptions(['badge' => static fn (): int => 42]); + $item = new Item('scores', ['badge' => static fn (): int => 42]); + $this->ext->processItem($item); - self::assertArrayNotHasKey('badge', $result); - self::assertSame(42, $result['extras']['badge']); + self::assertSame(42, $item->getBadge()); } #[Test] public function zeroBadgeIsPreserved(): void { - $result = $this->ext->buildOptions(['badge' => 0]); + $item = new Item('scores', ['badge' => 0]); + $this->ext->processItem($item); - self::assertSame(0, $result['extras']['badge']); + self::assertSame(0, $item->getBadge()); } #[Test] public function closureReturningZeroIsPreserved(): void { - $result = $this->ext->buildOptions(['badge' => static fn (): int => 0]); + $item = new Item('scores', ['badge' => static fn (): int => 0]); + $this->ext->processItem($item); - self::assertSame(0, $result['extras']['badge']); + self::assertSame(0, $item->getBadge()); } #[Test] public function existingExtrasArePreserved(): void { - $result = $this->ext->buildOptions([ + $item = new Item('scores', [ 'extras' => ['icon' => 'rehearsal'], 'badge' => 3, ]); + $this->ext->processItem($item); - self::assertSame('rehearsal', $result['extras']['icon']); - self::assertSame(3, $result['extras']['badge']); + self::assertSame('rehearsal', $item->getOption('extras')['icon']); + self::assertSame(3, $item->getBadge()); } } diff --git a/tests/Unit/Factory/Extension/LabelExtensionTest.php b/tests/Unit/Factory/Extension/LabelExtensionTest.php index 0de25f7..de51123 100644 --- a/tests/Unit/Factory/Extension/LabelExtensionTest.php +++ b/tests/Unit/Factory/Extension/LabelExtensionTest.php @@ -5,28 +5,21 @@ namespace Tests\Unit\Factory\Extension; use ChamberOrchestra\MenuBundle\Factory\Extension\LabelExtension; -use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; -use Symfony\Contracts\Translation\TranslatorInterface; -#[AllowMockObjectsWithoutExpectations] final class LabelExtensionTest extends TestCase { - private TranslatorInterface $translator; private LabelExtension $ext; protected function setUp(): void { - $this->translator = $this->createMock(TranslatorInterface::class); - $this->ext = new LabelExtension($this->translator); + $this->ext = new LabelExtension(); } #[Test] public function setsLabelFromKeyWhenLabelAbsent(): void { - $this->translator->expects(self::never())->method('trans'); - $result = $this->ext->buildOptions(['key' => 'home']); self::assertSame('home', $result['label']); @@ -48,50 +41,6 @@ public function keepsExplicitLabelOverKey(): void self::assertSame('Home', $result['label']); } - #[Test] - public function translatesLabelWhenDomainProvided(): void - { - $this->translator - ->expects(self::once()) - ->method('trans') - ->with('nav.home', [], 'navigation') - ->willReturn('Главная'); - - $result = $this->ext->buildOptions([ - 'label' => 'nav.home', - 'translation_domain' => 'navigation', - ]); - - self::assertSame('Главная', $result['label']); - } - - #[Test] - public function skipsTranslationWhenNoDomainProvided(): void - { - $this->translator->expects(self::never())->method('trans'); - - $result = $this->ext->buildOptions(['label' => 'Home']); - - self::assertSame('Home', $result['label']); - } - - #[Test] - public function translatesKeyDerivedLabelWhenDomainProvided(): void - { - $this->translator - ->expects(self::once()) - ->method('trans') - ->with('home', [], 'menu') - ->willReturn('Главная'); - - $result = $this->ext->buildOptions([ - 'key' => 'home', - 'translation_domain' => 'menu', - ]); - - self::assertSame('Главная', $result['label']); - } - #[Test] public function preservesOtherOptions(): void { diff --git a/tests/Unit/Menu/ItemTest.php b/tests/Unit/Menu/ItemTest.php index 6a7474e..c0a29d1 100644 --- a/tests/Unit/Menu/ItemTest.php +++ b/tests/Unit/Menu/ItemTest.php @@ -165,6 +165,35 @@ public function isIterableViaForeach(): void self::assertSame([$child], $collected); } + #[Test] + public function setExtraAddsToExtras(): void + { + $item = new Item('scores'); + $result = $item->setExtra('badge', 7); + + self::assertSame($item, $result); + self::assertSame(7, $item->getBadge()); + } + + #[Test] + public function setExtraPreservesExistingExtras(): void + { + $item = new Item('scores', ['extras' => ['icon' => 'rehearsal']]); + $item->setExtra('badge', 3); + + self::assertSame('rehearsal', $item->getOption('extras')['icon']); + self::assertSame(3, $item->getBadge()); + } + + #[Test] + public function setExtraOverwritesExistingKey(): void + { + $item = new Item('scores', ['extras' => ['badge' => 1]]); + $item->setExtra('badge', 99); + + self::assertSame(99, $item->getBadge()); + } + #[Test] public function serializationRoundTripPreservesAllFields(): void {