From dc0ecc751b625b5c43d550f8584ba458958ae8db Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 12 Jun 2026 15:06:26 -0400 Subject: [PATCH 1/3] fix(core): tokenizer-based ClassFileParser::extractClassName (recovered tier2 F3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tier 2 task 003 (token_get_all parser) was reported complete but its edit was lost before the tier2 commit — develop still had the buggy preg_match parser that matches 'class' in docblocks/strings and misses interfaces/traits/enums. Re-implemented here because it is the HARD prerequisite for the compiled discovery cache (a cache compiled against the buggy parser would permanently bake in silently-skipped files). token_get_all scan: tracks T_NAMESPACE, detects T_CLASS/T_INTERFACE/T_TRAIT/T_ENUM, skips ::class and anonymous classes. Co-Authored-By: Claude Fable 5 --- .../core/src/Discovery/ClassFileParser.php | 86 +++++- .../Unit/Discovery/ClassFileParserTest.php | 286 ++++++++++++++++++ 2 files changed, 364 insertions(+), 8 deletions(-) diff --git a/packages/core/src/Discovery/ClassFileParser.php b/packages/core/src/Discovery/ClassFileParser.php index 2f62d3a3..e8c3b51d 100644 --- a/packages/core/src/Discovery/ClassFileParser.php +++ b/packages/core/src/Discovery/ClassFileParser.php @@ -34,22 +34,92 @@ public function extractClassName( return null; } + $tokens = token_get_all($contents); $namespace = null; - $class = null; + $typeName = null; + $prevSignificant = null; + $tokenCount = count($tokens); - if (preg_match('/namespace\s+([^;]+);/', $contents, $matches)) { - $namespace = $matches[1]; - } + for ($i = 0; $i < $tokenCount; $i++) { + $token = $tokens[$i]; + + if (!is_array($token)) { + $prevSignificant = $token; + continue; + } + + [$id] = $token; + + // Skip whitespace and comments for tracking purposes + if (in_array($id, [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + continue; + } + + if ($id === T_NAMESPACE) { + // Consume namespace name tokens until ';' or '{' + $namespaceParts = []; + for ($j = $i + 1; $j < $tokenCount; $j++) { + $t = $tokens[$j]; + if (!is_array($t)) { + // ';' or '{' ends the namespace + break; + } + [$tid, $tval] = $t; + if (in_array($tid, [T_STRING, T_NAME_QUALIFIED, T_NS_SEPARATOR], true)) { + $namespaceParts[] = $tval; + } elseif ($tid !== T_WHITESPACE) { + break; + } + } + $namespace = implode('', $namespaceParts); + $prevSignificant = $token; + continue; + } + + if (in_array($id, [T_CLASS, T_INTERFACE, T_TRAIT, T_ENUM], true)) { + // Skip ::class constant references + if ($prevSignificant === '::' || (is_array( + $prevSignificant, + ) && $prevSignificant[0] === T_DOUBLE_COLON)) { + $prevSignificant = $token; + continue; + } + + // Skip anonymous classes (new class) + if (is_array($prevSignificant) && $prevSignificant[0] === T_NEW) { + $prevSignificant = $token; + continue; + } + + // Find the name token (T_STRING) after the type keyword, skipping whitespace + for ($j = $i + 1; $j < $tokenCount; $j++) { + $t = $tokens[$j]; + if (!is_array($t)) { + break; + } + [$tid, $tval] = $t; + if ($tid === T_WHITESPACE) { + continue; + } + if ($tid === T_STRING) { + $typeName = $tval; + } + break; + } + + if ($typeName !== null) { + break; + } + } - if (preg_match('/class\s+(\w+)/', $contents, $matches)) { - $class = $matches[1]; + $prevSignificant = $token; } - if ($class === null) { + if ($typeName === null) { return null; } - return $namespace !== null ? $namespace . '\\' . $class : $class; + return $namespace !== null ? $namespace . '\\' . $typeName : $typeName; } /** diff --git a/packages/core/tests/Unit/Discovery/ClassFileParserTest.php b/packages/core/tests/Unit/Discovery/ClassFileParserTest.php index 250a3461..23d3ba85 100644 --- a/packages/core/tests/Unit/Discovery/ClassFileParserTest.php +++ b/packages/core/tests/Unit/Discovery/ClassFileParserTest.php @@ -120,6 +120,292 @@ function helper(): void {} expect($files)->toBeEmpty(); }); +it('extracts the class name when a docblock above the class contains the word "class"', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/RealClass.php'); + + expect($className)->toBe('App\\Services\\RealClass'); + + unlink($tempDir . '/RealClass.php'); + rmdir($tempDir); +}); + +it('extracts the class name when the file body contains the word "class" inside a string literal', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/Controller.php'); + + expect($className)->toBe('App\\Http\\Controller'); + + unlink($tempDir . '/Controller.php'); + rmdir($tempDir); +}); + +it('returns the fully qualified name combining namespace and class', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/Entity.php'); + + expect($className)->toBe('Vendor\\Module\\Domain\\Entity'); + + unlink($tempDir . '/Entity.php'); + rmdir($tempDir); +}); + +it('extracts the name of a final class declaration', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/FirewallRule.php'); + + expect($className)->toBe('App\\Security\\FirewallRule'); + + unlink($tempDir . '/FirewallRule.php'); + rmdir($tempDir); +}); + +it('extracts the name of an abstract class declaration', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/AbstractHandler.php'); + + expect($className)->toBe('App\\Base\\AbstractHandler'); + + unlink($tempDir . '/AbstractHandler.php'); + rmdir($tempDir); +}); + +it('extracts the name of a readonly class declaration', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/UserDto.php'); + + expect($className)->toBe('App\\Dto\\UserDto'); + + unlink($tempDir . '/UserDto.php'); + rmdir($tempDir); +}); + +it('extracts the name of an interface, a trait, and an enum declaration', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $interfaceCode = <<<'PHP' +extractClassName($tempDir . '/HandlerInterface.php'))->toBe('App\\Contracts\\HandlerInterface') + ->and($parser->extractClassName($tempDir . '/HasTimestamps.php'))->toBe('App\\Concerns\\HasTimestamps') + ->and($parser->extractClassName($tempDir . '/Status.php'))->toBe('App\\Enums\\Status'); + + unlink($tempDir . '/HandlerInterface.php'); + unlink($tempDir . '/HasTimestamps.php'); + unlink($tempDir . '/Status.php'); + rmdir($tempDir); +}); + +it('returns null for a file that declares no class, interface, trait, or enum', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/helpers.php'); + + expect($className)->toBeNull(); + + unlink($tempDir . '/helpers.php'); + rmdir($tempDir); +}); + +it( + 'ignores ::class constant references and new class anonymous-class expressions when no real top-level type is declared', + function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir, 0755, true); + + $code = <<<'PHP' +extractClassName($tempDir . '/factories.php'); + + expect($className)->toBeNull(); + + unlink($tempDir . '/factories.php'); + rmdir($tempDir); + }, +); + it('handles deeply nested namespaces', function (): void { $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); mkdir($tempDir, 0755, true); From 97283c124d41beb1d17eb69fcc50483a99b76357 Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 12 Jun 2026 15:13:53 -0400 Subject: [PATCH 2/3] docs(discovery-cache): apply devil's advocate review Verified the tokenizer prerequisite is present; pinned DISCOVERY_CACHE_ENABLED false-set coercion, the corrupt-cache exception path (names the file path, write() throws notWritable), the version-from-constant invariant, four independent boot forks preserving subsystem ordering (observers before dispatcher, commands after module-repo), and mandatory $_ENV + temp-file test isolation. Corrected the task table (002 depends on 001). Co-Authored-By: Claude Fable 5 --- .../discovery-cache/001-discovery-config.md | 14 +- .../discovery-cache/002-discovery-cache.md | 10 +- .../discovery-cache/003-discovery-compiler.md | 5 +- .../004-discovery-cache-command.md | 4 +- .../005-discovery-clear-command.md | 2 + .../discovery-cache/006-boot-integration.md | 38 ++++ .../plans/discovery-cache/_devils_advocate.md | 214 ++++++++++++++++++ .claude/plans/discovery-cache/_plan.md | 19 +- 8 files changed, 291 insertions(+), 15 deletions(-) create mode 100644 .claude/plans/discovery-cache/_devils_advocate.md diff --git a/.claude/plans/discovery-cache/001-discovery-config.md b/.claude/plans/discovery-cache/001-discovery-config.md index 6be80288..119ac070 100644 --- a/.claude/plans/discovery-cache/001-discovery-config.md +++ b/.claude/plans/discovery-cache/001-discovery-config.md @@ -39,18 +39,24 @@ for the CLI commands (which run after full boot). Core's boot never reads this f - Standards: strict types, constructor promotion, no final, no magic methods, full type declarations, `readonly` where appropriate. ## Implementation detail -- `DiscoveryEnvironment` getters and defaults (read from `$_ENV`): - - `enabled(): bool` ← `DISCOVERY_CACHE_ENABLED`, default `true`. Treat the string values `'0'`, `'false'`, `''` (case-insensitive `false`) as `false`; everything else truthy. Document the exact coercion in the test. +- `DiscoveryEnvironment` getters and defaults (read from `$_ENV`). Getters read `$_ENV` LIVE at call time (not cached in the constructor), so tests can set env, then construct, then assert; and so the bootstrap sequence (EnvLoader populates `$_ENV` before the gate reads it) holds: + - `enabled(): bool` ← `DISCOVERY_CACHE_ENABLED`, default `true`. **Coercion (production-safety):** treat the case-insensitive string values `'0'`, `'false'`, `'no'`, `'off'`, and `''` as `false`; every other present value is `true`; an absent key defaults to `true`. This wider false-set matches operator intent (a deploy that sets `=no`/`=off` to disable the cache actually disables it, avoiding a stale-cache-in-prod surprise). Document the exact coercion in the test. - `environment(): string` ← `APP_ENV`, default `'production'`. - `cachePath(): string` ← `DISCOVERY_CACHE_PATH`, default `'storage/cache/discovery.php'` (relative — resolved against `ProjectPaths::$base` by `DiscoveryCache`, Task 002). -- `config/discovery.php` returns `['enabled' => ..., 'environment' => ..., 'cache_path' => ...]` reading the SAME three `$_ENV` keys with the SAME defaults so the two layers never disagree. +- `config/discovery.php` returns `['enabled' => ..., 'environment' => ..., 'cache_path' => ...]` reading the SAME three `$_ENV` keys with the SAME defaults/coercion so the two layers never disagree. NOTE: the file basename becomes the top-level config key, so `marko/config` consumers read `discovery.enabled` / `discovery.environment` / `discovery.cache_path`. + +## Test isolation (MANDATORY) +- `$_ENV` is process-global and Pest runs in parallel per worker. Every test that sets `DISCOVERY_CACHE_ENABLED`, `APP_ENV`, or `DISCOVERY_CACHE_PATH` MUST snapshot those three keys in `beforeEach` and restore them exactly (including unsetting keys that were absent) in `afterEach` (or use `try/finally`). Leaking a mutated `$_ENV` would silently flip the ~40 existing `Application::initialize()` tests in `packages/core/tests/Unit/ApplicationTest.php` into "cache enabled, production" mode. Exercise the no-env-set default path explicitly to prove the defaults. ## Requirements (Test Descriptions) -- [ ] `it returns enabled() true by default and false when DISCOVERY_CACHE_ENABLED is "0", "false", or empty` +- [ ] `it returns enabled() true by default (no env set) and treats DISCOVERY_CACHE_ENABLED values 0, false, no, off, and empty (case-insensitive) as disabled` +- [ ] `it returns enabled() true for any other present DISCOVERY_CACHE_ENABLED value (e.g. "1", "true", "yes")` - [ ] `it returns environment() from APP_ENV and defaults to production when APP_ENV is unset` - [ ] `it returns cachePath() from DISCOVERY_CACHE_PATH and defaults to storage/cache/discovery.php` +- [ ] `it reads $_ENV live at call time (a value set after construction is reflected)` - [ ] `it reads no value from marko/config and has no Marko\Config import (boot-time reader is config-package-free)` - [ ] `the shipped config/discovery.php returns an array with enabled, environment, and cache_path keys matching the DiscoveryEnvironment defaults when no env vars are set` +- [ ] `it snapshots and restores the three $_ENV keys around each test so no env state leaks (verify $_ENV unchanged after the suite for keys that were originally absent)` ## Acceptance Criteria - All requirements have passing tests diff --git a/.claude/plans/discovery-cache/002-discovery-cache.md b/.claude/plans/discovery-cache/002-discovery-cache.md index b9e1e9e3..34c304ad 100644 --- a/.claude/plans/discovery-cache/002-discovery-cache.md +++ b/.claude/plans/discovery-cache/002-discovery-cache.md @@ -20,11 +20,13 @@ Add `DiscoveryCache`, the component that owns the compiled cache file at `storag - `/Users/markshust/Sites/marko/packages/core/src/Path/ProjectPaths.php` (`$base`) - `/Users/markshust/Sites/marko/packages/core/src/Discovery/DiscoveryEnvironment.php` (Task 001 — provides `cachePath()`) - Patterns to follow: - - Existing core exceptions in `packages/core/src/Exceptions/` (e.g. `CommandException`) — `extends MarkoException`, named static factory methods, three-part `message`/`context`/`suggestion`. Mirror that for `DiscoveryCacheException::unreadable($path)`, `::malformed($path, $reason)`, `::versionMismatch($path, $found, $expected)`, `::notWritable($path)`. The `suggestion` for corrupt/version cases must name `vendor/bin/marko discovery:clear`. + - Existing core exceptions in `packages/core/src/Exceptions/` (e.g. `CommandException`) — `extends MarkoException`, named static factory methods, three-part `message`/`context`/`suggestion`. Mirror that for `DiscoveryCacheException::unreadable($path)`, `::malformed($path, $reason)`, `::versionMismatch($path, $found, $expected)`, `::notWritable($path)`. The `suggestion` for corrupt/version cases MUST name the cache FILE PATH to delete AND `vendor/bin/marko discovery:clear` — but note the ordering: a corrupt cache throws during `Application::initialize()`, which runs BEFORE any command can dispatch (see `CliKernel::doRun()`), so `discovery:clear` cannot itself run through a corrupt boot. Deleting the named file is therefore the primary recovery instruction; `discovery:clear` is the convenience path once boot succeeds. Make the path the first, most prominent part of the suggestion. - `ProjectPaths` for resolving a relative `cache_path` against `$base`; use an absolute `cache_path` as-is. Detect absolute via a leading `/` (or a Windows drive prefix) — do NOT assume relative. - Serialize via `var_export($payload, true)` wrapped as `"modules)` (whole module list at once). - Because `ObserverDiscovery`/`CommandDiscovery` take the module LIST while `PreferenceDiscovery`/`PluginDiscovery` take a single module, the compiler must loop modules for the latter two and pass the full list to the former two — mirroring `Application` exactly, preserving module order (do not sort or dedupe). - Test setup `createTestModule` + temp `src/` fixtures in `packages/core/tests/Unit/Module/ModuleDiscoveryTest.php` for building module fixtures with attribute-bearing classes. -- Standards: strict types, constructor promotion, no final, no magic methods, full type declarations. Payload keys must match `DiscoveryCache` exactly (`version`, `preferences`, `plugins`, `observers`, `commands`). +- Standards: strict types, constructor promotion, no final, no magic methods, full type declarations. Payload keys must match `DiscoveryCache` exactly (`version`, `preferences`, `plugins`, `observers`, `commands`). The `'version'` value MUST be `DiscoveryCache::CACHE_VERSION` (the constant from Task 002) — NEVER a hardcoded literal, or a future bump silently desyncs the compiler from the loader and every fresh cache fails its own version check. +- **Equivalence invariant (load-bearing).** `Application` resolves `ObserverDiscovery` through the container (`$this->container->get(ObserverDiscovery::class)`, autowired), while the compiler `new`s it directly. These are equivalent ONLY because no `#[Preference]` currently targets a discovery class. Add a code comment in `DiscoveryCompiler` documenting that it intentionally bypasses container/preference resolution for the discovery classes themselves, and why that is safe (discovery runs before preferences apply to discovery classes; no preference targets them). The equivalence test below must compare the compiled output against the SAME construction the reference uses so this invariant is guarded, not assumed. ## Requirements (Test Descriptions) - [ ] `it compiles an empty payload (empty preference, plugin, observer, command arrays) for modules with no attribute-bearing classes` -- [ ] `it includes the current cache schema version key in the compiled payload` +- [ ] `it includes the current cache schema version key sourced from DiscoveryCache::CACHE_VERSION (not a hardcoded literal) in the compiled payload` - [ ] `it compiles preferences discovered across all modules into the payload preferences array` - [ ] `it compiles plugins, observers, and commands discovered across all modules into their respective payload arrays` - [ ] `it produces a payload that, written and reloaded through DiscoveryCache, yields discovery objects identical to running the four discovery passes directly over the same modules` diff --git a/.claude/plans/discovery-cache/004-discovery-cache-command.md b/.claude/plans/discovery-cache/004-discovery-cache-command.md index 714d1fdc..935d7063 100644 --- a/.claude/plans/discovery-cache/004-discovery-cache-command.md +++ b/.claude/plans/discovery-cache/004-discovery-cache-command.md @@ -26,13 +26,15 @@ ad-hoc paths. - `Output` exposes only `write()` / `writeLine()` — use `writeLine` for all messages; return `0` on success, non-zero on failure. - How the command obtains the resolved module list to compile: inject `ModuleRepositoryInterface` (bound in the container at boot, line ~171 of `Application`) and pass `->all()` to `DiscoveryCompiler`. `ModuleListCommand` (`packages/core/src/Commands/ModuleListCommand.php`) is the exact precedent — it constructor-injects `ModuleRepositoryInterface` and calls `$this->moduleRepository->all()`. NOTE: when `discovery:cache` runs, the cache is being (re)built, so boot itself rescanned (or used a stale cache); either way `ModuleRepositoryInterface->all()` returns the live resolved module list, which is what the compiler needs. - `DiscoveryCompiler::compile($modules)` returns the payload; `DiscoveryCache::write($payload)` persists it; count each section from the payload arrays for the report. + - Write-failure contract: `DiscoveryCache::write()` THROWS `DiscoveryCacheException::notWritable` (Task 002) on any write failure. The command catches it and returns a non-zero exit code with `$e->getMessage()` (and suggestion) via `writeLine`. Do not rely on a boolean return. +- Test isolation: any test that writes a real cache file MUST use a UNIQUE per-test temp directory (point `ProjectPaths`/`DISCOVERY_CACHE_PATH` at it) and delete it in cleanup (mirror `appTestCleanupDirectory` in `packages/core/tests/Unit/ApplicationTest.php`). A leftover `storage/cache/discovery.php` under a shared base would silently flip later `Application::initialize()` tests onto the cache path. - Standards: strict types, constructor promotion, no final, no magic methods, full type declarations. `DiscoveryCacheCommand` is in `marko/core` and MUST NOT reference `Marko\Config` (it injects `DiscoveryCache`/`DiscoveryCompiler`/`ModuleRepositoryInterface` only). ## Requirements (Test Descriptions) - [ ] `it compiles discovery and writes the cache file, returning exit code 0` - [ ] `it writes a cache file that DiscoveryCache reports as existing after the command runs` - [ ] `it reports the cache file path and the counts of cached preferences, plugins, observers, and commands` -- [ ] `it returns a non-zero exit code and a helpful message when the cache cannot be written` +- [ ] `it returns a non-zero exit code and a helpful message (catching DiscoveryCacheException::notWritable) when the cache cannot be written` - [ ] `it overwrites a pre-existing cache file with freshly compiled content` ## Acceptance Criteria diff --git a/.claude/plans/discovery-cache/005-discovery-clear-command.md b/.claude/plans/discovery-cache/005-discovery-clear-command.md index 4c2f3a62..5049d845 100644 --- a/.claude/plans/discovery-cache/005-discovery-clear-command.md +++ b/.claude/plans/discovery-cache/005-discovery-clear-command.md @@ -9,6 +9,8 @@ ## Description Add the `discovery:clear` CLI command. It removes the compiled discovery cache file via `DiscoveryCache::clear()`, reporting success. Clearing is idempotent: clearing when no cache exists is reported as success, not an error. +**Recovery-path limitation (must be documented in the command and the corrupt-cache exception, Task 002).** `discovery:clear` runs AFTER `Application::initialize()` (see `CliKernel::doRun()`), and Task 006 makes a CORRUPT cache throw during `initialize()`. So `discovery:clear` CANNOT recover a corrupt cache — boot throws before the command dispatches. It only works when boot succeeds (cache valid or absent). The recovery instruction for a corrupt cache is to delete the file directly; the `DiscoveryCacheException` suggestion (Task 002) names that path first. `discovery:clear` is the convenience path for a valid cache you simply want to remove. + ## Context - Related files: - `/Users/markshust/Sites/marko/packages/core/src/Commands/DiscoveryClearCommand.php` (NEW) diff --git a/.claude/plans/discovery-cache/006-boot-integration.md b/.claude/plans/discovery-cache/006-boot-integration.md index fd2043a6..83073bff 100644 --- a/.claude/plans/discovery-cache/006-boot-integration.md +++ b/.claude/plans/discovery-cache/006-boot-integration.md @@ -23,6 +23,27 @@ gated by `class_exists`), so `$_ENV` is populated before the gate reads it. When `marko/env` is absent, `DiscoveryEnvironment` falls back to its documented defaults (enabled, production) — which is the safe production default. Document this in a test. +**The gate runs during CLI boot too.** `CliKernel::doRun()` calls `$app->initialize()` BEFORE +dispatching ANY command — including `discovery:cache` and `discovery:clear`. Consequences: +- On a cache HIT, the CLI itself boots from the cache. `discovery:cache` still recompiles correctly: + it reads `ModuleRepositoryInterface->all()` (always the LIVE resolved module list — module discovery + is never cached, `$this->modules` is set at line 130 before the fork) and runs a fresh + `DiscoveryCompiler` scan, independent of the hydrated registries. +- On a CORRUPT cache, `initialize()` throws — so `discovery:clear` CANNOT run through a corrupt boot + (boot precedes dispatch). This is intentional (loud errors), but it means the corrupt-cache recovery + story is "delete the file directly," which is why `DiscoveryCacheException`'s suggestion (Task 002) + names the file path first. Document this in a test and keep the `discovery:clear` limitation note + (Task 005) consistent. + +**Test isolation (MANDATORY).** `$_ENV` is process-global and Pest runs in parallel per worker. Every +test here that sets `APP_ENV`/`DISCOVERY_CACHE_ENABLED`/`DISCOVERY_CACHE_PATH` MUST snapshot and +restore those three keys (beforeEach/afterEach or try/finally), leaving `$_ENV` as found — otherwise it +flips the ~40 existing `Application::initialize()` tests in `ApplicationTest.php` into cache mode. Every +test that writes a real cache file MUST use a UNIQUE per-test temp base and delete it in cleanup +(mirror `appTestCleanupDirectory`), so a leftover `storage/cache/discovery.php` never bleeds into +another boot test. `$cache->exists()` is resolved against `ProjectPaths::$base` (the per-test temp +base), so a unique+cleaned temp base gives isolation for free. + ## Description of refactor Split each of `discoverPreferences/Plugins/Observers/Commands` into two halves: a "scan → definitions" half (existing discovery call) and a "register definitions into the registry" half. Boot then sources the definition lists from EITHER the live scan OR `DiscoveryCache::load()`, and runs the same registration half for both paths. @@ -33,6 +54,19 @@ Split each of `discoverPreferences/Plugins/Observers/Commands` into two halves: The cleanest refactor: keep the four `discoverX()` methods doing scan-and-register (unchanged), and add four parallel `registerXFromCache(array $definitions)` (or a single hydrate-and-register path) that perform ONLY the registry construction + registration + container wiring halves, fed by `DiscoveryCache::load()`. The fork chooses which to call per subsystem. +**Preserve the EXACT existing interleaving — do NOT wrap all four scans in one `if`.** Verified against +`Application::initialize()`: preferences (line 157) → plugins (160) → observers (163) → [create +`EventDispatcher` + bind `EventDispatcherInterface`, lines 166-167; create `ModuleRepository` + bind +`ModuleRepositoryInterface`, lines 170-171] → commands (174). The event-dispatcher and module-repository +wiring sits BETWEEN observers and commands and is unchanged. Therefore each subsystem must choose +scan-vs-cache INDEPENDENTLY at its current call site (four small forks, or four `registerXFromCache` +methods invoked in place), NOT one monolithic branch around all four — otherwise the ordering breaks. + +**Each subsystem's two halves are MUTUALLY EXCLUSIVE — exactly one runs per boot.** On a cache hit, run +only `registerXFromCache`; on a miss/dev/disabled, run only `discoverX()`. Registering from BOTH would +double-register and `CommandRegistry::register()` throws `duplicateCommandName` (and the preference/plugin +registries would conflict too). This is the most likely refactor bug — guard it with a test. + All other boot wiring (module discovery, autoloaders, container/registry construction, bindings, event dispatcher creation, `discoverRoutes`, module `boot` callbacks) is unchanged. Only the source of the four definition lists changes. NOTE: `discoverRoutes` (RoutingBootstrapper) is NOT cached and still runs its own scan every request — that is in-scope-excluded and unchanged. ## Context @@ -50,13 +84,17 @@ All other boot wiring (module discovery, autoloaders, container/registry constru ## Requirements (Test Descriptions) - [ ] `it hydrates preferences, plugins, observers, and commands from the cache when enabled and environment is not development and the cache exists` - [ ] `it binds CommandRegistry in the container and creates the CommandRunner on a cache hit (commands are runnable without a scan)` +- [ ] `it registers each cached command exactly once and never throws duplicateCommandName on a cache hit (scan and cache halves are mutually exclusive)` +- [ ] `it preserves the existing boot ordering on a cache hit (observers registered before the event dispatcher is created; commands after the module repository is bound)` - [ ] `it does not consult the filesystem for markers on a cache hit (a class present on disk but absent from the cache is not registered)` - [ ] `it always rescans the filesystem in development even when a cache file exists (a class present on disk is registered regardless of cache contents)` - [ ] `it rescans the filesystem when caching is enabled but no cache file exists` - [ ] `it rescans the filesystem when caching is disabled by env (DISCOVERY_CACHE_ENABLED=false)` - [ ] `it throws DiscoveryCacheException during boot when the cache exists but is corrupt and caching is enabled outside development` +- [ ] `it skips the corrupt-cache throw in development (rescans instead) so a corrupt cache never blocks dev boot` - [ ] `it produces the same registered preferences, plugins, observers, and commands from a valid cache as from a fresh scan of the same modules` - [ ] `it rescans (defaults to enabled production) when marko/env is not installed and no env vars are set` +- [ ] `it restores $_ENV and removes any temp cache file after each test so other Application::initialize() tests are unaffected` ## Acceptance Criteria - All requirements have passing tests diff --git a/.claude/plans/discovery-cache/_devils_advocate.md b/.claude/plans/discovery-cache/_devils_advocate.md new file mode 100644 index 00000000..ea8d16fe --- /dev/null +++ b/.claude/plans/discovery-cache/_devils_advocate.md @@ -0,0 +1,214 @@ +# Devil's Advocate Review: discovery-cache + +Reviewed against current source on branch `feature/discovery-cache`. The tokenizer-based +`ClassFileParser::extractClassName` (the hard prerequisite) is **confirmed present and correct** +(`packages/core/src/Discovery/ClassFileParser.php` uses `token_get_all()`, not `preg_match`). +`marko/core/composer.json` confirms it requires only `php` + `psr/container` (no `marko/config`). +The four value objects, four discovery classes, and `Application::initialize()` structure all match +the plan's cited shapes. + +## Critical (Must fix before building) + +### C1 — Task 006 gate runs in the CLI boot too, and `discovery:cache`/`discovery:clear` would self-block on a stale/corrupt cache (activation + dead-lock trap) +`packages/cli/src/CliKernel.php::doRun()` calls `$app->initialize()` *before* dispatching ANY +command, including `discovery:cache` and `discovery:clear`. With Task 006's gate, the CLI's own +boot will read the cache. Two failure modes the plan does not address: + +1. **Corrupt cache locks you out of the fix.** Task 006 makes a corrupt cache throw + `DiscoveryCacheException` during `initialize()`. But `initialize()` is what runs *before* + `discovery:clear` can execute. So the one command whose entire job is to remove a corrupt cache + can never run — boot throws first. The loud-error policy is right, but it must not brick the + recovery path. +2. **Stale cache silently feeds the recompile.** On a cache *hit*, the four registries are + hydrated from the cache and the four scans are skipped — but `discovery:cache` recompiles from + `ModuleRepositoryInterface->all()` + a fresh `DiscoveryCompiler` scan (Task 003/004), which is + correct and independent of the hydrated registries. So the recompile itself is fine. The real + problem is only #1 (corrupt cache) plus the conceptual surprise that the CLI always boots through + the gate. + +**Fix:** Task 006 must state that the gate runs during CLI boot as well (the same `initialize()`), +AND that `discovery:clear` must remain runnable when the cache is corrupt. The cleanest resolution: +the corrupt-cache `throw` happens at hydration time, and the CLI entrypoint already wraps +`initialize()` in `try/catch (Throwable)` (CliKernel lines 52-56) — but that catch turns the throw +into exit code 1 and the command never runs. So Task 006 must require that the gate's corrupt-cache +detection is **bypassed in development** (already specified) AND document the operator recovery +contract: a corrupt cache is cleared by deleting the file (`discovery:clear` cannot self-heal a +corrupt cache because boot precedes it). Add an explicit requirement/test: +`it throws a DiscoveryCacheException whose suggestion tells the operator to delete the cache file +(discovery:clear cannot run through a corrupt boot)`. Update Task 005 to note `discovery:clear` only +works when boot succeeds (cache valid or absent), so the corrupt-recovery story is "delete the +file" — and `DiscoveryCacheException`'s suggestion (Task 002) must therefore name the **file path +to delete**, not only `vendor/bin/marko discovery:clear`. + +### C2 — `$_ENV` mutation in Task 006 tests will leak into the ~40 existing `initialize()` tests +`packages/core/tests/Unit/ApplicationTest.php` already contains ~40 tests that call +`$app->initialize()` against temp module fixtures (lines 98-557). Task 001 and Task 006 introduce a +gate that reads `$_ENV['APP_ENV']`, `$_ENV['DISCOVERY_CACHE_ENABLED']`, `$_ENV['DISCOVERY_CACHE_PATH']`. +Pest runs tests in parallel **per worker process**, and `$_ENV` is process-global. Any Task 001/006 +test that sets `$_ENV['APP_ENV'] = 'production'` (to exercise the cache-hit path) without restoring it +will silently flip every *subsequent* `initialize()` test in the same worker into "cache enabled, +production" mode — and if a `storage/cache/discovery.php` happens to exist under that test's temp base, +those tests will hydrate from a cache instead of scanning, producing confusing, order-dependent +failures. + +**Fix:** Add an explicit requirement to Tasks 001 and 006: every test that mutates `$_ENV` MUST +snapshot and restore the three keys in `beforeEach`/`afterEach` (or use `try/finally`), leaving +`$_ENV` exactly as found. State that the default (no env set) path must be exercised to prove the +existing boot tests are unaffected. This is a test-isolation contract, not optional polish — without +it the suite becomes flaky. + +### C3 — Default `enabled=true` + `environment='production'` means EVERY existing `initialize()` test now takes the cache path by default whenever a cache file exists under the temp base +Task 001 specifies `enabled()` defaults to `true` and `environment()` defaults to `'production'`. +That means the gate predicate `enabled() && environment() !== 'development' && exists()` is satisfied +by default the moment a `discovery.php` exists at `{base}/storage/cache/`. The existing tests use a +temp base with no such file, so they're safe **today**. But Task 004's `discovery:cache` test and +Task 006's cache-hit tests will *write* a `discovery.php` under a temp base; if any of them reuse a +shared base path or don't clean up the file, a later `initialize()` test under the same base silently +switches to the cache path. + +**Fix:** Require (Tasks 004, 006) that every test using a real cache file writes it under a +**unique per-test temp directory** and deletes it in cleanup (mirror `appTestCleanupDirectory` in +`ApplicationTest.php`). Also state in Task 006 that the gate's `$cache->exists()` is resolved against +`ProjectPaths::$base` (the temp base), so tests get isolation for free *as long as* the temp base is +unique and cleaned. Make this an explicit acceptance criterion, not an implementation detail. + +### C4 — `discovery:cache` and `discovery:clear` are core `#[Command]`s, so they are discovered by `CommandDiscovery` — but `CommandDiscovery` scans `src/`, and these commands live in `src/Commands/`. Confirm the registry does not collide on a cache hit. +On a cache *hit* (Task 006), `discoverCommands()` is skipped and commands come from the cache file. +The cache was compiled (Task 003) by scanning `src/` — which includes `DiscoveryCacheCommand` and +`DiscoveryClearCommand` themselves (they carry `#[Command]`). Good: they end up in the cache, so +they remain runnable on a cache hit. But verify the **ordering**: `CommandRegistry::register()` +throws `CommandException::duplicateCommandName` on a duplicate. If the cache-hit path constructs a +fresh `CommandRegistry` and registers each cached `CommandDefinition` exactly once, there is no +collision. The risk is a refactor that registers BOTH from cache AND re-scans (double registration). + +**Fix:** Task 006's `registerCommandsFromCache()` (or equivalent) MUST be mutually exclusive with +`discoverCommands()` — exactly one runs. Add a requirement: +`it registers each cached command exactly once and never throws duplicateCommandName on a cache hit`. +This guards the most likely refactor bug (calling both halves). + +## Important (Should fix before building) + +### I1 — Task 002's hydration must reject associative-array corruption that `var_export` round-trips silently, and the "missing required field" check needs to cover the nested plugin-method shape +`PluginDefinition::$beforeMethods`/`$afterMethods` are associative arrays keyed by target method, +values `['pluginMethod' => string, 'sortOrder' => int]` (verified in `PluginDefinition.php` and +`PluginDiscovery::parsePluginClass`). Task 002 already has a round-trip test for this. But the +"missing required field" validation (`it throws ... when a record within a section is missing a +required field`) currently names only `targetClass`. A hand-edited or version-skewed cache could +have a plugin entry whose `beforeMethods` value is missing `sortOrder`, or whose `aliases` got +coerced to a non-array. Hydration via `new PluginDefinition(...)` would then pass a malformed array +straight into the registry, surfacing as a confusing error deep in `PluginInterceptor` rather than a +loud `DiscoveryCacheException` at load time. + +**Fix:** Broaden Task 002's malformed-record requirement to assert that each section validates its +record shape before constructing the value object: preferences need `replacement`+`replaces`; +plugins need `pluginClass`+`targetClass`+array `beforeMethods`/`afterMethods`; observers need +`observerClass`+`eventClass`+int `priority`+bool `async`; commands need `commandClass`+`name`+string +`description`+array `aliases`. Add a requirement: +`it throws DiscoveryCacheException when a record field has the wrong type (e.g. observer priority is +a string or command aliases is not an array)`. + +### I2 — Task 003 equivalence test must construct discovery classes EXACTLY as `Application` does, but `Application` resolves `ObserverDiscovery` through the container (autowired) while the compiler `new`s it — the plan asserts these are equivalent but provides no guard +The plan's risk note acknowledges `Application` does +`$this->container->get(ObserverDiscovery::class)` whereas the compiler will `new +ObserverDiscovery(new ClassFileParser())`. They ARE equivalent today because no preference/plugin +targets `ObserverDiscovery`. But that equivalence is an *invariant the compiler silently depends on*. +If someone later writes a `#[Preference]` replacing `ObserverDiscovery`, `Application` would honor it +(container resolution) and the compiler would not (direct `new`), producing a cache that disagrees +with a live boot — exactly the bug this whole plan exists to avoid. + +**Fix:** Task 003's equivalence test (`it produces a payload that ... yields discovery objects +identical to running the four discovery passes directly`) must compare against the **same +construction `Application` uses**, i.e. run the reference discovery for observers via the container +(or document loudly in `DiscoveryCompiler` that it intentionally bypasses preferences for discovery +classes and why that is safe). Add a one-line note to Task 003 and a code comment requirement so the +invariant is discoverable, not buried in `_plan.md`. This is the single most important correctness +guard in the plan. + +### I3 — Task 006 refactor: the `CommandRegistry` container binding + `CommandRunner` creation currently lives at the END of `discoverCommands()` (lines 301-304). The cache path must replicate BOTH, and the event dispatcher + module repository are created BETWEEN observers and commands. +Verified against `Application.php`: `discoverObservers()` (line 163) → create `EventDispatcher` + +bind `EventDispatcherInterface` (166-167) → create `ModuleRepository` + bind +`ModuleRepositoryInterface` (170-171) → `discoverCommands()` (174). The command-registry container +binding (`$this->container->instance(CommandRegistry::class, ...)`) and `CommandRunner` creation are +at lines 302-304, INSIDE `discoverCommands()`. Task 006 already flags this, but it must be explicit +that the fork cannot simply wrap all four scans in one `if` — observers run BEFORE the event +dispatcher/module repository wiring, and commands run AFTER. The cleanest structure is **four +independent forks** (or four `registerXFromCache` methods) interleaved at the exact same positions as +today, NOT one monolithic branch around all four. + +**Fix:** Reword Task 006's refactor section to require preserving the **exact interleaving**: +preferences (157) → plugins (160) → observers (163) → [dispatcher+module-repo wiring, unchanged] → +commands (174). Each subsystem chooses scan-vs-cache independently at its current call site. Add a +requirement: `it preserves the existing boot ordering (observers before event-dispatcher creation, +commands after module-repository binding) on both the scan and cache paths`. + +### I4 — Task 001/006 env-coercion for `DISCOVERY_CACHE_ENABLED` is under-specified at the boundary that matters most: a present-but-unparseable value +Task 001 specifies `'0'`, `'false'`, `''` → false, "everything else truthy." But `$_ENV` values are +always strings (or absent). The dangerous case is a deploy that sets `DISCOVERY_CACHE_ENABLED=no` or +`=off` expecting it to disable the cache — under the spec, `'no'`/`'off'` are "everything else" → +**true**, so the cache stays enabled contrary to operator intent, and in production that means a +stale-cache-in-prod surprise. This is a production-safety footgun. + +**Fix:** Either (a) explicitly document in Task 001 that ONLY `'0'`/`'false'`/`''` disable and all +other values (including `'no'`/`'off'`) enable — and make `discovery:cache`/the docs say so loudly — +or (b) expand the false-set to the conventional `{'0','false','no','off',''}` (case-insensitive). +Option (b) matches operator expectations and is the safer default. Pick one and pin it in the test: +`it treats DISCOVERY_CACHE_ENABLED values {0,false,no,off,empty} (case-insensitive) as disabled`. +(Recommend option b.) + +### I5 — Task 004 reports "counts of cached preferences/plugins/observers/commands" but counts from the payload BEFORE write confirmation — and `var_export` of class-strings is fine, but the atomic-write `rename()` can fail silently on cross-device temp dirs +Task 002 specifies atomic write via temp-file-then-`rename()` "in the same directory." That is +correct and must stay (a temp file in the system temp dir then `rename()` across filesystems fails +with a warning and a `false` return that is easy to ignore). Task 004's "non-zero exit on write +failure" requirement depends on `DiscoveryCache::write()` actually surfacing the failure. + +**Fix:** Task 002 must require `write()` to throw `DiscoveryCacheException::notWritable($path)` (not +return `false` / not emit a warning) when the directory cannot be created, the temp write fails, or +the `rename()` fails — so Task 004's command can catch it and exit non-zero with a real message. Add +to Task 002: `it throws DiscoveryCacheException::notWritable when the target directory cannot be +created or the file cannot be written`. Then Task 004's "helpful message when the cache cannot be +written" test asserts it catches that exception. Tie the two together explicitly so the failure +contract is not split across tasks with a gap. + +### I6 — Task 003 dependency on Task 002 is for the schema-version constant only; flag the coupling so a parallel worker doesn't hardcode `version => 1` +Task 003 must emit `'version' => DiscoveryCache::CACHE_VERSION` in the payload (the plan says the +compiler "includes the current cache schema version key"). If Task 003's worker hardcodes `1` instead +of referencing the constant defined in Task 002, a future bump in Task 002 silently desyncs the +compiler from the loader and every freshly compiled cache fails its own version check on load. + +**Fix:** Task 003 requirement `it includes the current cache schema version key in the compiled +payload` must specify the value comes from `DiscoveryCache::CACHE_VERSION`, not a literal. Add the +note to Task 003's standards line. (Dependency 003←002 already exists, so the constant is available.) + +## Minor (Nice to address) + +### M1 — `config/discovery.php` top-level key will be `discovery` (file basename), so config consumers read `discovery.enabled`, `discovery.cache_path`, `discovery.environment` +Confirmed via `ConfigDiscovery::mergeConfigFromDirectory` (`$key = pathinfo($file, FILENAME)`). Not a +defect — just worth noting in Task 001 so the CLI-consumer story (commands reading via +`ConfigRepositoryInterface`) uses the right dotted keys. No fix applied (informational). + +### M2 — `CommandRegistry::all()` sorts alphabetically; cache order vs scan order won't matter for commands, but `preferences`/`plugins`/`observers` registration order CAN matter (conflict detection, sort order). The compiler preserves module order (good), and hydration should preserve array order (PHP arrays are ordered) — just confirm no `ksort`/`array_unique` sneaks into hydration. +Informational; Task 002's "don't turn associative arrays into lists" test partially covers this. + +### M3 — The plan ships `config/discovery.php` as core's first `config/` dir. `marko/core/composer.json` does not list it anywhere (config is discovered by glob, not declared), so no composer change is needed — but the package-structure test (if any) that asserts core's file layout may need updating. +Worth a quick check during Task 001; not blocking. + +## Questions for the Team + +### Q1 — Should `discovery:cache` refuse to run (or warn) in development? +In development the cache is never read, so compiling it is a no-op for boot. Running `discovery:cache` +in dev silently writes a file that nothing consumes until `APP_ENV` changes. Harmless, but possibly +surprising. Should the command print a notice ("APP_ENV=development; this cache will not be used until +you deploy to a non-development environment")? Not specified. (Leaving as a question — no behavior +change applied.) + +### Q2 — Is there a deploy/release hook that should auto-run `discovery:cache`? +The plan's stale-cache risk rests entirely on "document that `discovery:cache` must run on deploy." +`.claude/release-process.md` exists. Should this plan add a step there, or is that a separate docs +task owned by the doc-updater pipeline? (Out of scope for the task files; flagging for the team.) + +### Q3 — `DiscoveryEnvironment` reads `$_ENV` at call time, not construct time. Confirm intended. +If `DiscoveryEnvironment` reads `$_ENV` in its getters (not the constructor), then a `module.php` +boot callback that mutates `$_ENV` after the gate has run cannot affect the already-made gate +decision (the gate runs before boot callbacks). That's the correct sequencing, but the plan should +confirm getters read live `$_ENV` so tests that set env before constructing it behave predictably. +Informational. diff --git a/.claude/plans/discovery-cache/_plan.md b/.claude/plans/discovery-cache/_plan.md index d7f8f906..769b7dd7 100644 --- a/.claude/plans/discovery-cache/_plan.md +++ b/.claude/plans/discovery-cache/_plan.md @@ -100,7 +100,7 @@ This plan MUST land AFTER `tier2-high-correctness/003-classfileparser-tokenizer. | Task | Description | Depends On | Status | |------|-------------|------------|--------| | 001 | `DiscoveryEnvironment` env reader (core, NO marko/config) + shipped `config/discovery.php` defaults | - | pending | -| 002 | `DiscoveryCacheException` + `DiscoveryCache` read/write/exists/clear + hydrate/serialize | - | pending | +| 002 | `DiscoveryCacheException` + `DiscoveryCache` read/write/exists/clear + hydrate/serialize | 001 | pending | | 003 | `DiscoveryCompiler` — run the four scans, produce the plain-array payload | 002 | pending | | 004 | `discovery:cache` command (compile + write) | 002, 003 | pending | | 005 | `discovery:clear` command (remove cache file) | 002 | pending | @@ -114,15 +114,24 @@ This plan MUST land AFTER `tier2-high-correctness/003-classfileparser-tokenizer. - `DiscoveryCache` depends on `ProjectPaths` (for base path) and `DiscoveryEnvironment` (for cache path); both via constructor promotion, `readonly` where appropriate. No `final`, no magic methods, no traits, strict types, full type declarations. - `DiscoveryCompiler` reuses the EXISTING `PreferenceDiscovery`, `PluginDiscovery`, `ObserverDiscovery`, `CommandDiscovery` classes unchanged — it just collects their `array<...Definition>` output across the resolved module list and maps each value object to its array form. This guarantees cached output is byte-identical in meaning to a fresh scan (same classes, same code path). - Serialization: write via a deterministic array literal (e.g. `var_export` of the payload wrapped in ` ..., 'environment' => ..., 'cache_path' => ... ]` reading the same `$_ENV` keys). It exists solely so `marko/config` consumers can see/override these values; the CLI commands MAY read it via `ConfigRepositoryInterface` since they run after full boot. Core's boot path does NOT read it. -- Boot integration is a single fork in `initialize()`, placed BEFORE the four discovery calls (lines ~157-174): construct `DiscoveryEnvironment` + `DiscoveryCache` (the latter needs the `ProjectPaths` already registered at line 149), compute `$useCache = $env->enabled() && $env->environment() !== 'development' && $cache->exists()`. If true, hydrate the four registries from `$cache->load()`; else run the existing four `discoverX()` methods. The `discoverCommands` registry/runner container wiring (binding `CommandRegistry`, creating `CommandRunner`) MUST still run on BOTH paths — only the SOURCE of the command DEFINITIONS changes, not the wiring. Keep `ModuleDiscovery`, autoloader registration, container/registry setup, binding registration, event-dispatcher creation, `discoverRoutes`, and module `boot` callbacks exactly as today. The `discoverX()` methods are refactored so the "register definitions into the registry" half is reusable for both the scan path and the cache path. +- Boot integration computes `$useCache = $env->enabled() && $env->environment() !== 'development' && $cache->exists()` once (after `ProjectPaths` is registered at line 149), then forks PER SUBSYSTEM at the existing call sites — NOT one monolithic branch around all four. The existing interleaving must be preserved EXACTLY: preferences (157) → plugins (160) → observers (163) → [event-dispatcher + module-repository wiring, 166-171, unchanged] → commands (174). Each subsystem runs EITHER its `discoverX()` scan OR its `registerXFromCache()` hydration — the two halves are mutually exclusive (registering from both would double-register and `CommandRegistry` would throw `duplicateCommandName`). The `discoverCommands` registry/runner container wiring (binding `CommandRegistry`, creating `CommandRunner`) MUST still run on BOTH paths — only the SOURCE of the command DEFINITIONS changes, not the wiring. Keep `ModuleDiscovery`, autoloader registration, container/registry setup, binding registration, event-dispatcher creation, `discoverRoutes`, and module `boot` callbacks exactly as today. +- The gate runs during CLI boot too (`CliKernel::doRun()` calls `initialize()` before dispatching any command). `discovery:cache` recompiles correctly regardless (module list is always live; the compiler runs a fresh scan). A CORRUPT cache throws during `initialize()`, so `discovery:clear` cannot self-heal a corrupt boot — the recovery instruction is to delete the named file (the `DiscoveryCacheException` suggestion names the path first). +- Test isolation is a hard requirement (Tasks 001, 004, 006): `$_ENV` is process-global and Pest runs in parallel per worker, so every test mutating `APP_ENV`/`DISCOVERY_CACHE_ENABLED`/`DISCOVERY_CACHE_PATH` must snapshot+restore those keys, and every test writing a real cache file must use a unique per-test temp base and clean it up — otherwise the ~40 existing `Application::initialize()` tests silently flip onto the cache path. - Commands follow the `ClearCommand`/`ListCommand` pattern: `readonly` class, `#[Command(name: 'discovery:cache'|'discovery:clear', description: ...)]`, implements `CommandInterface`, `execute(Input, Output): int`, dependencies injected (`DiscoveryCompiler`, `DiscoveryCache`). `Output` only exposes `write`/`writeLine` — use `writeLine` for all messaging; non-zero return for failure. ## Risks & Mitigations -- **Stale cache after deploy.** If a deploy adds packages/markers but the cache is not recompiled, boot would serve stale discovery. Mitigation: document that `discovery:cache` must run on deploy (release step); the `'version'` key guards against format drift; dev never reads the cache. (No mtime invalidation in this cut — explicit recompile is the contract.) +- **Stale cache after deploy.** If a deploy adds packages/markers but the cache is not recompiled, boot would serve stale discovery. Mitigation: document that `discovery:cache` must run on deploy (release step); the `'version'` key guards against format drift; dev never reads the cache. (No mtime invalidation in this cut — explicit recompile is the contract.) Operator-intent footgun: `DISCOVERY_CACHE_ENABLED=no`/`off` now disables (wider false-set), matching the obvious expectation rather than silently staying enabled. +- **Corrupt cache bricks recovery via the CLI.** `initialize()` runs before any command dispatches, so a corrupt cache throws before `discovery:clear` can execute. Mitigation: the corrupt-cache exception names the file path to delete as the primary recovery; `discovery:clear` is documented (Task 005) as only working when boot succeeds; dev never reads the cache so dev boot is never blocked. +- **`$_ENV`/cache-file test leakage flips existing boot tests.** Mitigation: mandatory snapshot+restore of the three env keys and unique per-test temp bases with cleanup (Tasks 001, 004, 006). - **Cache compiled against buggy parser.** Mitigated by the hard prerequisite on tier2 task 003 (tokenizer parser) — stated above; do not start before it merges. - **`DiscoveryCompiler` must instantiate the same discovery classes the same way as `Application`.** `ObserverDiscovery`/`CommandDiscovery` take a `ClassFileParser`; `PreferenceDiscovery`/`PluginDiscovery` are `new`-ed per module. The compiler must mirror `Application`'s construction exactly so cached output equals scanned output. NOTE: `Application` resolves `ObserverDiscovery` via `$this->container->get(ObserverDiscovery::class)` (autowired), while `CommandDiscovery` is `new CommandDiscovery($this->classFileParser)`. Since both only need a `ClassFileParser` (no preferences/plugins applied to discovery classes), the compiler may `new` all four directly — covered by an equivalence test (Task 003/006). - **Closure-bearing `ModuleManifest`.** Explicitly NOT cached; module discovery stays live each request. The per-request module scan reads only each module's `composer.json` (+ optional `module.php`) — O(modules), NOT O(all PHP files) — so the perf win from caching the four file-content scans is preserved. Mitigation is the documented scope boundary. - **Inverted-dependency trap (was the original plan's fatal flaw).** The first draft put a `DiscoveryConfig` wrapping `ConfigRepositoryInterface` into `marko/core` and read it during boot. That is impossible: core cannot require `marko/config`, and `ConfigRepositoryInterface` is not bound until module boot callbacks run (after discovery). Resolved by the `DiscoveryEnvironment` boot-time reader (no `marko/config` dependency). The shipped `config/discovery.php` is for `marko/config` consumers only. -- **First-ever `config/` dir in core.** `config/discovery.php` is harmless when `marko/config` is absent (core never reads it) and discoverable by `ConfigDiscovery` (globs `{modulePath}/config/*.php`) when present. Task 001 does NOT assert it loads through `ConfigRepositoryInterface` (core tests must not depend on `marko/config`); it asserts the file returns the expected default shape. +- **First-ever `config/` dir in core.** `config/discovery.php` is harmless when `marko/config` is absent (core never reads it) and discoverable by `ConfigDiscovery` (globs `{modulePath}/config/*.php`) when present. The file basename is the top-level config key, so consumers read `discovery.enabled`/`discovery.environment`/`discovery.cache_path`. Task 001 does NOT assert it loads through `ConfigRepositoryInterface` (core tests must not depend on `marko/config`); it asserts the file returns the expected default shape. +- **Corrupt cache vs the recovery command (CLI boot trap).** `CliKernel::doRun()` calls `initialize()` before dispatching any command. A corrupt cache throws during `initialize()`, so `discovery:clear` cannot run through a corrupt boot. The corrupt-cache `DiscoveryCacheException` suggestion therefore names the FILE PATH to delete first (primary recovery), with `discovery:clear` as the convenience path once boot succeeds. The gate predicate excludes development, so a corrupt cache never blocks dev boot (it is never read in dev). +- **Test isolation / `$_ENV` leakage.** The gate reads `$_ENV`; the ~40 existing `Application::initialize()` tests would flip onto the cache path if a test leaks a mutated `APP_ENV`/`DISCOVERY_CACHE_ENABLED` or a leftover cache file. Snapshot+restore env keys and use unique per-test temp bases with cleanup (Tasks 001, 004, 006). +- **Schema-version constant single-sourcing.** The compiler (Task 003) must emit `'version' => DiscoveryCache::CACHE_VERSION`, never a literal, so a future bump invalidates stale caches loudly instead of producing fresh caches that fail their own version check. +- **Equivalence invariant.** The compiler `new`s the discovery classes while `Application` resolves `ObserverDiscovery` via the container; they agree only because no `#[Preference]` targets a discovery class. The compiler documents this bypass in a code comment, and Task 003's equivalence test guards it. +- **Write-failure surfacing.** `DiscoveryCache::write()` throws `DiscoveryCacheException::notWritable` (never returns false / never relies on a PHP warning) so `discovery:cache` can catch it and exit non-zero with a real message. From ec36afa3570e65e43935511a0f26ce33f991fc0d Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 12 Jun 2026 15:39:06 -0400 Subject: [PATCH 3/3] feat(discovery-cache): CLI-compiled discovery cache to skip per-request scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eliminates the per-request, full-codebase PHP-file scan for #[Preference]/ #[Plugin]/#[Observer]/#[Command] markers in Application::initialize() outside development, via a CLI-compiled cache. All 6 tasks, TDD, full-suite green (6735). - DiscoveryEnvironment: core-owned $_ENV reader (APP_ENV / DISCOVERY_CACHE_ENABLED / DISCOVERY_CACHE_PATH) — NO marko/config dependency (core requires only php + psr/container; config isn't bound until after discovery). - DiscoveryCache: write/read/exists/clear a generated storage/cache/discovery.php (var_export array literal, no closures/unserialize); hydrate back into the four result value objects; loud DiscoveryCacheException (names the path) on corrupt/ invalid/version-mismatched content; write() throws notWritable on failure. - DiscoveryCompiler: runs the four existing discovery passes over resolved modules and produces the plain-array payload (equivalence-tested vs a fresh scan). - discovery:cache (compile + write, reports counts + path) and discovery:clear (idempotent delete) commands. - Boot gate in initialize(): cache used only when enabled AND env != development AND a valid file exists (four independent forks preserve subsystem ordering; CommandRegistry/Runner wiring runs on both scan and cache paths); a missing file rescans, a corrupt file throws. core gains NO marko/config dependency. - Ships core's first config/discovery.php for marko/config consumers. Prerequisite recovered first (commit dc0ecc7): the tokenizer-based ClassFileParser (tier2 F3) whose edit was lost — a cache compiled against the buggy preg_match parser would have baked in silently-skipped files. NOTE: no mtime invalidation — discovery:cache must run on deploy (release note added). Devil's-advocate-reviewed, standards-enforced, docs updated, lint clean. Co-Authored-By: Claude Fable 5 --- .../discovery-cache/001-discovery-config.md | 23 +- .../discovery-cache/002-discovery-cache.md | 39 +- .../discovery-cache/003-discovery-compiler.md | 20 +- .../004-discovery-cache-command.md | 16 +- .../005-discovery-clear-command.md | 17 +- .../discovery-cache/006-boot-integration.md | 42 +- .claude/plans/discovery-cache/_plan.md | 14 +- .claude/release-process.md | 2 + packages/core/config/discovery.php | 14 + packages/core/src/Application.php | 115 ++- .../src/Commands/DiscoveryCacheCommand.php | 51 ++ .../src/Commands/DiscoveryClearCommand.php | 31 + .../core/src/Discovery/DiscoveryCache.php | 401 ++++++++++ .../core/src/Discovery/DiscoveryCompiler.php | 106 +++ .../src/Discovery/DiscoveryEnvironment.php | 29 + .../Exceptions/DiscoveryCacheException.php | 49 ++ .../Command/DiscoveryCacheCommandTest.php | 182 +++++ .../Command/DiscoveryClearCommandTest.php | 114 +++ .../Unit/ApplicationDiscoveryCacheTest.php | 752 ++++++++++++++++++ .../Unit/Discovery/DiscoveryCacheTest.php | 406 ++++++++++ .../Unit/Discovery/DiscoveryCompilerTest.php | 349 ++++++++ .../Discovery/DiscoveryEnvironmentTest.php | 164 ++++ packages/docs-markdown/docs/packages/cli.md | 10 +- packages/docs-markdown/docs/packages/core.md | 95 +++ 24 files changed, 2957 insertions(+), 84 deletions(-) create mode 100644 packages/core/config/discovery.php create mode 100644 packages/core/src/Commands/DiscoveryCacheCommand.php create mode 100644 packages/core/src/Commands/DiscoveryClearCommand.php create mode 100644 packages/core/src/Discovery/DiscoveryCache.php create mode 100644 packages/core/src/Discovery/DiscoveryCompiler.php create mode 100644 packages/core/src/Discovery/DiscoveryEnvironment.php create mode 100644 packages/core/src/Exceptions/DiscoveryCacheException.php create mode 100644 packages/core/tests/Command/DiscoveryCacheCommandTest.php create mode 100644 packages/core/tests/Command/DiscoveryClearCommandTest.php create mode 100644 packages/core/tests/Unit/ApplicationDiscoveryCacheTest.php create mode 100644 packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php create mode 100644 packages/core/tests/Unit/Discovery/DiscoveryCompilerTest.php create mode 100644 packages/core/tests/Unit/Discovery/DiscoveryEnvironmentTest.php diff --git a/.claude/plans/discovery-cache/001-discovery-config.md b/.claude/plans/discovery-cache/001-discovery-config.md index 119ac070..9ff9b407 100644 --- a/.claude/plans/discovery-cache/001-discovery-config.md +++ b/.claude/plans/discovery-cache/001-discovery-config.md @@ -1,6 +1,6 @@ # Task 001: DiscoveryEnvironment reader and shipped config defaults -**Status**: pending +**Status**: complete **Depends on**: [none] **Retry count**: 0 @@ -49,14 +49,14 @@ for the CLI commands (which run after full boot). Core's boot never reads this f - `$_ENV` is process-global and Pest runs in parallel per worker. Every test that sets `DISCOVERY_CACHE_ENABLED`, `APP_ENV`, or `DISCOVERY_CACHE_PATH` MUST snapshot those three keys in `beforeEach` and restore them exactly (including unsetting keys that were absent) in `afterEach` (or use `try/finally`). Leaking a mutated `$_ENV` would silently flip the ~40 existing `Application::initialize()` tests in `packages/core/tests/Unit/ApplicationTest.php` into "cache enabled, production" mode. Exercise the no-env-set default path explicitly to prove the defaults. ## Requirements (Test Descriptions) -- [ ] `it returns enabled() true by default (no env set) and treats DISCOVERY_CACHE_ENABLED values 0, false, no, off, and empty (case-insensitive) as disabled` -- [ ] `it returns enabled() true for any other present DISCOVERY_CACHE_ENABLED value (e.g. "1", "true", "yes")` -- [ ] `it returns environment() from APP_ENV and defaults to production when APP_ENV is unset` -- [ ] `it returns cachePath() from DISCOVERY_CACHE_PATH and defaults to storage/cache/discovery.php` -- [ ] `it reads $_ENV live at call time (a value set after construction is reflected)` -- [ ] `it reads no value from marko/config and has no Marko\Config import (boot-time reader is config-package-free)` -- [ ] `the shipped config/discovery.php returns an array with enabled, environment, and cache_path keys matching the DiscoveryEnvironment defaults when no env vars are set` -- [ ] `it snapshots and restores the three $_ENV keys around each test so no env state leaks (verify $_ENV unchanged after the suite for keys that were originally absent)` +- [x] `it returns enabled() true by default (no env set) and treats DISCOVERY_CACHE_ENABLED values 0, false, no, off, and empty (case-insensitive) as disabled` +- [x] `it returns enabled() true for any other present DISCOVERY_CACHE_ENABLED value (e.g. "1", "true", "yes")` +- [x] `it returns environment() from APP_ENV and defaults to production when APP_ENV is unset` +- [x] `it returns cachePath() from DISCOVERY_CACHE_PATH and defaults to storage/cache/discovery.php` +- [x] `it reads $_ENV live at call time (a value set after construction is reflected)` +- [x] `it reads no value from marko/config and has no Marko\Config import (boot-time reader is config-package-free)` +- [x] `the shipped config/discovery.php returns an array with enabled, environment, and cache_path keys matching the DiscoveryEnvironment defaults when no env vars are set` +- [x] `it snapshots and restores the three $_ENV keys around each test so no env state leaks (verify $_ENV unchanged after the suite for keys that were originally absent)` ## Acceptance Criteria - All requirements have passing tests @@ -65,4 +65,7 @@ for the CLI commands (which run after full boot). Core's boot never reads this f - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- `DiscoveryEnvironment` at `packages/core/src/Discovery/DiscoveryEnvironment.php` — reads `$_ENV` live at call time for all three keys; uses a typed `FALSE_VALUES` constant array for coercion; no marko/config imports +- `config/discovery.php` at `packages/core/config/discovery.php` — mirrors same coercion logic using a local `$falseValues` variable; returns `['enabled', 'environment', 'cache_path']` +- Test isolation: `beforeEach`/`afterEach` snapshot/restore all three `$_ENV` keys; unsets keys that were originally absent +- All 8 tests pass; phpcs and php-cs-fixer clean; 501 core tests pass diff --git a/.claude/plans/discovery-cache/002-discovery-cache.md b/.claude/plans/discovery-cache/002-discovery-cache.md index 34c304ad..a68074de 100644 --- a/.claude/plans/discovery-cache/002-discovery-cache.md +++ b/.claude/plans/discovery-cache/002-discovery-cache.md @@ -1,6 +1,6 @@ # Task 002: DiscoveryCache read/write/exists/clear with loud corruption errors -**Status**: pending +**Status**: complete **Depends on**: [001] **Retry count**: 0 @@ -30,20 +30,20 @@ Add `DiscoveryCache`, the component that owns the compiled cache file at `storag - Standards: strict types, constructor promotion (`ProjectPaths`, `DiscoveryEnvironment`), no final, no magic methods, full type declarations. NO reference to `Marko\Config`. ## Requirements (Test Descriptions) -- [ ] `it writes a discovery payload to a return-array PHP file at the configured cache path and creates the directory if missing` -- [ ] `it reports exists() true after a write and false before any write or after clear` -- [ ] `it clears the cache file and clear() is idempotent when the file is already absent` -- [ ] `it loads a written cache back into PreferenceRecord, PluginDefinition, ObserverDefinition, and CommandDefinition objects identical to the payload` -- [ ] `it round-trips PluginDefinition beforeMethods/afterMethods associative arrays preserving target-method keys and the pluginMethod/sortOrder shape` -- [ ] `it round-trips empty definition arrays and empty aliases/beforeMethods/afterMethods without turning associative arrays into lists` -- [ ] `it resolves a relative cache_path against the project base path and uses an absolute cache_path unchanged` -- [ ] `it throws DiscoveryCacheException when the cache file content is not a PHP array` -- [ ] `it throws DiscoveryCacheException when the cache file is missing required keys (version, preferences, plugins, observers, commands)` -- [ ] `it throws DiscoveryCacheException when the cache version key does not match the current cache schema version` -- [ ] `it throws DiscoveryCacheException when a record within a section is missing a required field (e.g. a plugin entry without targetClass)` -- [ ] `it throws DiscoveryCacheException when a record field has the wrong type (e.g. observer priority is a string, command aliases is not an array, plugin beforeMethods is not an array)` -- [ ] `it throws DiscoveryCacheException::notWritable when the target directory cannot be created or the file/rename cannot be written (never returns false silently)` -- [ ] `it throws DiscoveryCacheException whose suggestion names the cache file path to delete as the primary recovery, plus discovery:clear, when content is corrupt` +- [x] `it writes a discovery payload to a return-array PHP file at the configured cache path and creates the directory if missing` +- [x] `it reports exists() true after a write and false before any write or after clear` +- [x] `it clears the cache file and clear() is idempotent when the file is already absent` +- [x] `it loads a written cache back into PreferenceRecord, PluginDefinition, ObserverDefinition, and CommandDefinition objects identical to the payload` +- [x] `it round-trips PluginDefinition beforeMethods/afterMethods associative arrays preserving target-method keys and the pluginMethod/sortOrder shape` +- [x] `it round-trips empty definition arrays and empty aliases/beforeMethods/afterMethods without turning associative arrays into lists` +- [x] `it resolves a relative cache_path against the project base path and uses an absolute cache_path unchanged` +- [x] `it throws DiscoveryCacheException when the cache file content is not a PHP array` +- [x] `it throws DiscoveryCacheException when the cache file is missing required keys (version, preferences, plugins, observers, commands)` +- [x] `it throws DiscoveryCacheException when the cache version key does not match the current cache schema version` +- [x] `it throws DiscoveryCacheException when a record within a section is missing a required field (e.g. a plugin entry without targetClass)` +- [x] `it throws DiscoveryCacheException when a record field has the wrong type (e.g. observer priority is a string, command aliases is not an array, plugin beforeMethods is not an array)` +- [x] `it throws DiscoveryCacheException::notWritable when the target directory cannot be created or the file/rename cannot be written (never returns false silently)` +- [x] `it throws DiscoveryCacheException whose suggestion names the cache file path to delete as the primary recovery, plus discovery:clear, when content is corrupt` ## Acceptance Criteria - All requirements have passing tests @@ -51,4 +51,11 @@ Add `DiscoveryCache`, the component that owns the compiled cache file at `storag - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Added `DiscoveryCacheException` in `packages/core/src/Exceptions/` extending `MarkoException` with four static factories: `unreadable`, `malformed`, `versionMismatch`, `notWritable` +- Added `DiscoveryCache` in `packages/core/src/Discovery/` with `CACHE_VERSION = 1` constant; constructor takes `ProjectPaths` + `DiscoveryEnvironment` (both `private readonly`) +- Path resolution: absolute paths (leading `/` or Windows drive letter) used as-is; relative paths resolved against `$projectPaths->base` +- `write()` serializes via `var_export` in `container->get(ObserverDiscovery::class)`, autowired), while the compiler `new`s it directly. These are equivalent ONLY because no `#[Preference]` currently targets a discovery class. Add a code comment in `DiscoveryCompiler` documenting that it intentionally bypasses container/preference resolution for the discovery classes themselves, and why that is safe (discovery runs before preferences apply to discovery classes; no preference targets them). The equivalence test below must compare the compiled output against the SAME construction the reference uses so this invariant is guarded, not assumed. ## Requirements (Test Descriptions) -- [ ] `it compiles an empty payload (empty preference, plugin, observer, command arrays) for modules with no attribute-bearing classes` -- [ ] `it includes the current cache schema version key sourced from DiscoveryCache::CACHE_VERSION (not a hardcoded literal) in the compiled payload` -- [ ] `it compiles preferences discovered across all modules into the payload preferences array` -- [ ] `it compiles plugins, observers, and commands discovered across all modules into their respective payload arrays` -- [ ] `it produces a payload that, written and reloaded through DiscoveryCache, yields discovery objects identical to running the four discovery passes directly over the same modules` -- [ ] `it aggregates results from multiple modules preserving the module load order` +- [x] `it compiles an empty payload (empty preference, plugin, observer, command arrays) for modules with no attribute-bearing classes` +- [x] `it includes the current cache schema version key sourced from DiscoveryCache::CACHE_VERSION (not a hardcoded literal) in the compiled payload` +- [x] `it compiles preferences discovered across all modules into the payload preferences array` +- [x] `it compiles plugins, observers, and commands discovered across all modules into their respective payload arrays` +- [x] `it produces a payload that, written and reloaded through DiscoveryCache, yields discovery objects identical to running the four discovery passes directly over the same modules` +- [x] `it aggregates results from multiple modules preserving the module load order` ## Acceptance Criteria - All requirements have passing tests @@ -41,4 +41,8 @@ Add `DiscoveryCompiler`, which runs the four existing attribute-marker discovery - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- `DiscoveryCompiler` created at `packages/core/src/Discovery/DiscoveryCompiler.php` in `Marko\Core\Discovery` namespace. +- Mirrors `Application::discoverPreferences/Plugins/Observers/Commands` exactly: loops modules for `PreferenceDiscovery`/`PluginDiscovery` (each `new PreferenceDiscovery()`/`new PluginDiscovery()` internally create their own `ClassFileParser`), and passes full module list to `ObserverDiscovery`/`CommandDiscovery` constructed directly with `new ObserverDiscovery(new ClassFileParser())` / `new CommandDiscovery(new ClassFileParser())`. +- Uses `DiscoveryCache::CACHE_VERSION` constant (not a hardcoded literal) for the `version` key. +- Code comment documents why bypassing container/preference resolution is safe for discovery classes. +- All 6 test requirements pass. The 12 pre-existing failures in `ApplicationDiscoveryCacheTest.php` are unrelated to this task (they existed before these changes). diff --git a/.claude/plans/discovery-cache/004-discovery-cache-command.md b/.claude/plans/discovery-cache/004-discovery-cache-command.md index 935d7063..4db1957d 100644 --- a/.claude/plans/discovery-cache/004-discovery-cache-command.md +++ b/.claude/plans/discovery-cache/004-discovery-cache-command.md @@ -1,6 +1,6 @@ # Task 004: discovery:cache command -**Status**: pending +**Status**: complete **Depends on**: [002, 003] **Retry count**: 0 @@ -31,11 +31,11 @@ ad-hoc paths. - Standards: strict types, constructor promotion, no final, no magic methods, full type declarations. `DiscoveryCacheCommand` is in `marko/core` and MUST NOT reference `Marko\Config` (it injects `DiscoveryCache`/`DiscoveryCompiler`/`ModuleRepositoryInterface` only). ## Requirements (Test Descriptions) -- [ ] `it compiles discovery and writes the cache file, returning exit code 0` -- [ ] `it writes a cache file that DiscoveryCache reports as existing after the command runs` -- [ ] `it reports the cache file path and the counts of cached preferences, plugins, observers, and commands` -- [ ] `it returns a non-zero exit code and a helpful message (catching DiscoveryCacheException::notWritable) when the cache cannot be written` -- [ ] `it overwrites a pre-existing cache file with freshly compiled content` +- [x] `it compiles discovery and writes the cache file, returning exit code 0` +- [x] `it writes a cache file that DiscoveryCache reports as existing after the command runs` +- [x] `it reports the cache file path and the counts of cached preferences, plugins, observers, and commands` +- [x] `it returns a non-zero exit code and a helpful message (catching DiscoveryCacheException::notWritable) when the cache cannot be written` +- [x] `it overwrites a pre-existing cache file with freshly compiled content` ## Acceptance Criteria - All requirements have passing tests @@ -43,4 +43,6 @@ ad-hoc paths. - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Created `packages/core/src/Commands/DiscoveryCacheCommand.php`: `readonly` class, `#[Command(name: 'discovery:cache')]`, injects `DiscoveryCompiler`, `DiscoveryCache`, `ModuleRepositoryInterface`. Calls `compile(modules)`, then `write(payload)`, reports path via `$discoveryCache->path()` and per-section counts. Catches `DiscoveryCacheException` and returns exit code 1 with message + suggestion. +- Added `public path(): string` method to `DiscoveryCache` to expose the resolved cache path without duplicating resolution logic in the command. +- Test file at `packages/core/tests/Command/DiscoveryCacheCommandTest.php`. Uses unique temp dirs per test with cleanup. The non-writable test creates a `0555` (read-only) directory so `file_put_contents` fails and `DiscoveryCacheException::notWritable` is thrown. diff --git a/.claude/plans/discovery-cache/005-discovery-clear-command.md b/.claude/plans/discovery-cache/005-discovery-clear-command.md index 5049d845..6507c0de 100644 --- a/.claude/plans/discovery-cache/005-discovery-clear-command.md +++ b/.claude/plans/discovery-cache/005-discovery-clear-command.md @@ -1,6 +1,6 @@ # Task 005: discovery:clear command -**Status**: pending +**Status**: complete **Depends on**: [002] **Retry count**: 0 @@ -23,10 +23,10 @@ Add the `discovery:clear` CLI command. It removes the compiled discovery cache f - Standards: strict types, constructor promotion, no final, no magic methods, full type declarations. MUST NOT reference `Marko\Config`. ## Requirements (Test Descriptions) -- [ ] `it removes an existing cache file and returns exit code 0` -- [ ] `it reports the cache as cleared via writeLine` -- [ ] `it returns exit code 0 and reports success when no cache file exists` -- [ ] `it leaves DiscoveryCache exists() false after running` +- [x] `it removes an existing cache file and returns exit code 0` +- [x] `it reports the cache as cleared via writeLine` +- [x] `it returns exit code 0 and reports success when no cache file exists` +- [x] `it leaves DiscoveryCache exists() false after running` ## Acceptance Criteria - All requirements have passing tests @@ -34,4 +34,9 @@ Add the `discovery:clear` CLI command. It removes the compiled discovery cache f - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Created `DiscoveryClearCommand` at `packages/core/src/Commands/DiscoveryClearCommand.php` +- `readonly class` — all constructor properties are immutable +- Injects `DiscoveryCache` directly (concrete autowirable class) +- `DiscoveryCache::clear()` is already idempotent (handles absent file), so the command always succeeds with exit code 0 +- Output message: "Discovery cache cleared successfully." +- Tests at `packages/core/tests/Command/DiscoveryClearCommandTest.php` cover all 4 requirements diff --git a/.claude/plans/discovery-cache/006-boot-integration.md b/.claude/plans/discovery-cache/006-boot-integration.md index 83073bff..cda0ff2b 100644 --- a/.claude/plans/discovery-cache/006-boot-integration.md +++ b/.claude/plans/discovery-cache/006-boot-integration.md @@ -1,6 +1,6 @@ # Task 006: Boot integration — prefer cache outside development -**Status**: pending +**Status**: complete **Depends on**: [001, 002] **Retry count**: 0 @@ -82,19 +82,19 @@ All other boot wiring (module discovery, autoloaders, container/registry constru - Standards: strict types, constructor promotion, no final, no magic methods, full type declarations. The boot path MUST NOT reference `Marko\Config`. ## Requirements (Test Descriptions) -- [ ] `it hydrates preferences, plugins, observers, and commands from the cache when enabled and environment is not development and the cache exists` -- [ ] `it binds CommandRegistry in the container and creates the CommandRunner on a cache hit (commands are runnable without a scan)` -- [ ] `it registers each cached command exactly once and never throws duplicateCommandName on a cache hit (scan and cache halves are mutually exclusive)` -- [ ] `it preserves the existing boot ordering on a cache hit (observers registered before the event dispatcher is created; commands after the module repository is bound)` -- [ ] `it does not consult the filesystem for markers on a cache hit (a class present on disk but absent from the cache is not registered)` -- [ ] `it always rescans the filesystem in development even when a cache file exists (a class present on disk is registered regardless of cache contents)` -- [ ] `it rescans the filesystem when caching is enabled but no cache file exists` -- [ ] `it rescans the filesystem when caching is disabled by env (DISCOVERY_CACHE_ENABLED=false)` -- [ ] `it throws DiscoveryCacheException during boot when the cache exists but is corrupt and caching is enabled outside development` -- [ ] `it skips the corrupt-cache throw in development (rescans instead) so a corrupt cache never blocks dev boot` -- [ ] `it produces the same registered preferences, plugins, observers, and commands from a valid cache as from a fresh scan of the same modules` -- [ ] `it rescans (defaults to enabled production) when marko/env is not installed and no env vars are set` -- [ ] `it restores $_ENV and removes any temp cache file after each test so other Application::initialize() tests are unaffected` +- [x] `it hydrates preferences, plugins, observers, and commands from the cache when enabled and environment is not development and the cache exists` +- [x] `it binds CommandRegistry in the container and creates the CommandRunner on a cache hit (commands are runnable without a scan)` +- [x] `it registers each cached command exactly once and never throws duplicateCommandName on a cache hit (scan and cache halves are mutually exclusive)` +- [x] `it preserves the existing boot ordering on a cache hit (observers registered before the event dispatcher is created; commands after the module repository is bound)` +- [x] `it does not consult the filesystem for markers on a cache hit (a class present on disk but absent from the cache is not registered)` +- [x] `it always rescans the filesystem in development even when a cache file exists (a class present on disk is registered regardless of cache contents)` +- [x] `it rescans the filesystem when caching is enabled but no cache file exists` +- [x] `it rescans the filesystem when caching is disabled by env (DISCOVERY_CACHE_ENABLED=false)` +- [x] `it throws DiscoveryCacheException during boot when the cache exists but is corrupt and caching is enabled outside development` +- [x] `it skips the corrupt-cache throw in development (rescans instead) so a corrupt cache never blocks dev boot` +- [x] `it produces the same registered preferences, plugins, observers, and commands from a valid cache as from a fresh scan of the same modules` +- [x] `it rescans (defaults to enabled production) when marko/env is not installed and no env vars are set` +- [x] `it restores $_ENV and removes any temp cache file after each test so other Application::initialize() tests are unaffected` ## Acceptance Criteria - All requirements have passing tests @@ -102,4 +102,16 @@ All other boot wiring (module discovery, autoloaders, container/registry constru - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) + +### Application.php changes +- Added imports: `CommandDefinition`, `PreferenceRecord`, `ObserverDefinition`, `PluginDefinition`, `DiscoveryCache`, `DiscoveryEnvironment`, `DiscoveryCacheException` +- `initialize()` now extracts `$projectPaths` as a local variable (passed to `DiscoveryCache`), computes `$useCache` gate, loads `$cachePayload` once (shared across four forks) +- Four independent forks replace the four `discoverX()` calls: `registerPreferencesFromCache`, `registerPluginsFromCache`, `registerObserversFromCache`, `registerCommandsFromCache` +- `registerCommandsFromCache` mirrors `discoverCommands()` container wiring: constructs `CommandRegistry`, binds it in container, creates `CommandRunner` +- `registerObserversFromCache` mirrors `discoverObservers()`: constructs `ObserverRegistry` before registering +- Corrupt cache (file exists but invalid) throws `DiscoveryCacheException` loudly (no silent fallback) +- Development + disabled + no-file all fall through to existing scan path + +### Test isolation +- `cacheTestSnapshotEnv()` snapshots and restores `APP_ENV`, `DISCOVERY_CACHE_ENABLED`, `DISCOVERY_CACHE_PATH` via try/finally in every test +- Each test uses a unique `bin2hex(random_bytes(8))` temp base; cleanup via `cacheTestCleanupDirectory()` diff --git a/.claude/plans/discovery-cache/_plan.md b/.claude/plans/discovery-cache/_plan.md index 769b7dd7..2f09da54 100644 --- a/.claude/plans/discovery-cache/_plan.md +++ b/.claude/plans/discovery-cache/_plan.md @@ -4,7 +4,7 @@ 2026-06-10 ## Status -ready +completed ## Objective Eliminate the per-request, full-codebase PHP-file scan in `Application::initialize()` by adding a CLI-compiled discovery cache: a `discovery:cache` command compiles attribute-marker discovery (preferences, plugins, observers, commands) into a generated `return [...]` PHP file that boot loads instead of rescanning, outside development. @@ -99,12 +99,12 @@ This plan MUST land AFTER `tier2-high-correctness/003-classfileparser-tokenizer. ## Task Overview | Task | Description | Depends On | Status | |------|-------------|------------|--------| -| 001 | `DiscoveryEnvironment` env reader (core, NO marko/config) + shipped `config/discovery.php` defaults | - | pending | -| 002 | `DiscoveryCacheException` + `DiscoveryCache` read/write/exists/clear + hydrate/serialize | 001 | pending | -| 003 | `DiscoveryCompiler` — run the four scans, produce the plain-array payload | 002 | pending | -| 004 | `discovery:cache` command (compile + write) | 002, 003 | pending | -| 005 | `discovery:clear` command (remove cache file) | 002 | pending | -| 006 | Boot integration in `Application::initialize()` (cache hit skips scan; dev always rescans; corrupt throws) | 001, 002 | pending | +| 001 | `DiscoveryEnvironment` env reader (core, NO marko/config) + shipped `config/discovery.php` defaults | - | completed | +| 002 | `DiscoveryCacheException` + `DiscoveryCache` read/write/exists/clear + hydrate/serialize | 001 | completed | +| 003 | `DiscoveryCompiler` — run the four scans, produce the plain-array payload | 002 | completed | +| 004 | `discovery:cache` command (compile + write) | 002, 003 | completed | +| 005 | `discovery:clear` command (remove cache file) | 002 | completed | +| 006 | Boot integration in `Application::initialize()` (cache hit skips scan; dev always rescans; corrupt throws) | 001, 002 | completed | **Hard cross-plan prerequisite:** all tasks below depend on `tier2-high-correctness/003-classfileparser-tokenizer` having merged first (see Prerequisite note above). Do not begin this plan until that task is on `develop`. diff --git a/.claude/release-process.md b/.claude/release-process.md index 1a42a425..6e04e225 100644 --- a/.claude/release-process.md +++ b/.claude/release-process.md @@ -144,6 +144,8 @@ After the tag is pushed, everything else is automatic: ``` - Every release-worthy commit on `develop` has a `#NN` reference in its message (the default for both merge commits and squash merges via the GitHub UI) +**Application deploy note:** After deploying a new release to a server, regenerate the discovery cache: `marko discovery:cache`. There is no automatic invalidation --- serving stale discovery means missing preferences, plugins, observers, or commands until the cache is recompiled. + **If tests fail:** Fix the failing tests and re-run `./bin/release.sh`. Do not skip tests. The script aborts before touching `CHANGELOG.md` or creating any tag, so a failed run leaves no garbage commits. **If a PR is missing from the generated notes:** the only realistic cause is a commit on `develop` that doesn't reference its PR with `#NN`. Add a `(#NN)` to the commit message (or amend before tagging) and re-run. The script lists every PR it found at the start of generation so missing ones are visible immediately. diff --git a/packages/core/config/discovery.php b/packages/core/config/discovery.php new file mode 100644 index 00000000..ac1d1278 --- /dev/null +++ b/packages/core/config/discovery.php @@ -0,0 +1,14 @@ + $rawEnabled === null + ? true + : !in_array(strtolower((string) $rawEnabled), $falseValues, strict: true), + 'environment' => $_ENV['APP_ENV'] ?? 'production', + 'cache_path' => $_ENV['DISCOVERY_CACHE_PATH'] ?? 'storage/cache/discovery.php', +]; diff --git a/packages/core/src/Application.php b/packages/core/src/Application.php index af42de32..278466f8 100644 --- a/packages/core/src/Application.php +++ b/packages/core/src/Application.php @@ -4,6 +4,7 @@ namespace Marko\Core; +use Marko\Core\Command\CommandDefinition; use Marko\Core\Command\CommandDiscovery; use Marko\Core\Command\CommandRegistry; use Marko\Core\Command\CommandRunner; @@ -11,16 +12,21 @@ use Marko\Core\Container\Container; use Marko\Core\Container\ContainerInterface; use Marko\Core\Container\PreferenceDiscovery; +use Marko\Core\Container\PreferenceRecord; use Marko\Core\Container\PreferenceRegistry; use Marko\Core\Discovery\ClassFileParser; +use Marko\Core\Discovery\DiscoveryCache; +use Marko\Core\Discovery\DiscoveryEnvironment; use Marko\Core\Event\EventDispatcher; use Marko\Core\Event\EventDispatcherInterface; +use Marko\Core\Event\ObserverDefinition; use Marko\Core\Event\ObserverDiscovery; use Marko\Core\Event\ObserverRegistry; use Marko\Core\Exceptions\BindingConflictException; use Marko\Core\Exceptions\BindingException; use Marko\Core\Exceptions\CircularDependencyException; use Marko\Core\Exceptions\CommandException; +use Marko\Core\Exceptions\DiscoveryCacheException; use Marko\Core\Exceptions\EventException; use Marko\Core\Exceptions\ModuleException; use Marko\Core\Exceptions\PluginException; @@ -34,6 +40,7 @@ use Marko\Core\Module\ModuleRepositoryInterface; use Marko\Core\Path\ProjectPaths; use Marko\Core\Plugin\InterceptorClassGenerator; +use Marko\Core\Plugin\PluginDefinition; use Marko\Core\Plugin\PluginDiscovery; use Marko\Core\Plugin\PluginInterceptor; use Marko\Core\Plugin\PluginRegistry; @@ -86,7 +93,7 @@ public function __construct( ) {} /** - * @throws ModuleException|CircularDependencyException|BindingConflictException|BindingException|PluginException|PreferenceConflictException|EventException|ContainerExceptionInterface|RouteException|RouteConflictException|CommandException|ReflectionException|RuntimeException + * @throws ModuleException|CircularDependencyException|BindingConflictException|BindingException|PluginException|PreferenceConflictException|EventException|ContainerExceptionInterface|RouteException|RouteConflictException|CommandException|ReflectionException|RuntimeException|DiscoveryCacheException */ public static function boot(string $basePath): self { @@ -106,7 +113,7 @@ public static function boot(string $basePath): self } /** - * @throws ModuleException|CircularDependencyException|BindingConflictException|BindingException|PluginException|PreferenceConflictException|EventException|ContainerExceptionInterface|RouteException|RouteConflictException|CommandException|ReflectionException + * @throws ModuleException|CircularDependencyException|BindingConflictException|BindingException|PluginException|PreferenceConflictException|EventException|ContainerExceptionInterface|RouteException|RouteConflictException|CommandException|ReflectionException|DiscoveryCacheException */ public function initialize(): void { @@ -146,21 +153,45 @@ public function initialize(): void // Register ProjectPaths for dependency injection (base path derived from vendor path) $basePath = dirname($this->vendorPath); - $this->container->instance(ProjectPaths::class, new ProjectPaths($basePath)); + $projectPaths = new ProjectPaths($basePath); + $this->container->instance(ProjectPaths::class, $projectPaths); // Register bindings from all modules foreach ($this->modules as $module) { $bindingRegistry->registerModule($module); } - // Discover and register preferences - $this->discoverPreferences(); + // Determine whether to hydrate from cache or run live scans. + // The gate reads DiscoveryEnvironment (which reads $_ENV directly) — no marko/config dependency. + $env = new DiscoveryEnvironment(); + $cache = new DiscoveryCache($projectPaths, $env); + $useCache = $env->enabled() && $env->environment() !== 'development' && $cache->exists(); + + // Load cache payload once (shared across all four subsystem forks). + // A corrupt cache throws DiscoveryCacheException loudly — no silent fallback. + /** @var array{preferences: PreferenceRecord[], plugins: PluginDefinition[], observers: ObserverDefinition[], commands: CommandDefinition[]}|null $cachePayload */ + $cachePayload = $useCache ? $cache->load() : null; + + // Fork 1 — preferences (independent of the other three) + if ($cachePayload !== null) { + $this->registerPreferencesFromCache($cachePayload['preferences']); + } else { + $this->discoverPreferences(); + } - // Discover and register plugins - $this->discoverPlugins(); + // Fork 2 — plugins (independent of the other three) + if ($cachePayload !== null) { + $this->registerPluginsFromCache($cachePayload['plugins']); + } else { + $this->discoverPlugins(); + } - // Discover and register observers - $this->discoverObservers(); + // Fork 3 — observers (independent; MUST run before EventDispatcher is created below) + if ($cachePayload !== null) { + $this->registerObserversFromCache($cachePayload['observers']); + } else { + $this->discoverObservers(); + } // Create event dispatcher and register in container $this->eventDispatcher = new EventDispatcher($this->container, $this->observerRegistry); @@ -170,8 +201,12 @@ public function initialize(): void $moduleRepository = new ModuleRepository($this->modules); $this->container->instance(ModuleRepositoryInterface::class, $moduleRepository); - // Discover and register commands - $this->discoverCommands(); + // Fork 4 — commands (MUST run after module repository is bound above) + if ($cachePayload !== null) { + $this->registerCommandsFromCache($cachePayload['commands']); + } else { + $this->discoverCommands(); + } // Discover and register routes (if routing package is available) $this->discoverRoutes(); @@ -304,6 +339,64 @@ private function discoverCommands(): void $this->commandRunner = new CommandRunner($this->container, $this->commandRegistry); } + /** + * @param PreferenceRecord[] $records + * + * @throws PreferenceConflictException + */ + private function registerPreferencesFromCache(array $records): void + { + foreach ($records as $record) { + $this->preferenceRegistry->register( + $record->replaces, + $record->replacement, + ); + } + } + + /** + * @param PluginDefinition[] $definitions + * + * @throws PluginException|ReflectionException + */ + private function registerPluginsFromCache(array $definitions): void + { + foreach ($definitions as $definition) { + $this->pluginRegistry->register($definition); + } + } + + /** + * @param ObserverDefinition[] $definitions + */ + private function registerObserversFromCache(array $definitions): void + { + $this->observerRegistry = new ObserverRegistry(); + + foreach ($definitions as $definition) { + $this->observerRegistry->register($definition); + } + } + + /** + * @param CommandDefinition[] $definitions + * + * @throws CommandException + */ + private function registerCommandsFromCache(array $definitions): void + { + $this->commandRegistry = new CommandRegistry(); + + foreach ($definitions as $definition) { + $this->commandRegistry->register($definition); + } + + // Bind registry in container so commands can inject it + $this->container->instance(CommandRegistry::class, $this->commandRegistry); + + $this->commandRunner = new CommandRunner($this->container, $this->commandRegistry); + } + /** * @throws RouteException|RouteConflictException|ReflectionException */ diff --git a/packages/core/src/Commands/DiscoveryCacheCommand.php b/packages/core/src/Commands/DiscoveryCacheCommand.php new file mode 100644 index 00000000..981a30b6 --- /dev/null +++ b/packages/core/src/Commands/DiscoveryCacheCommand.php @@ -0,0 +1,51 @@ +moduleRepository->all(); + $payload = $this->discoveryCompiler->compile($modules); + + try { + $this->discoveryCache->write($payload); + } catch (DiscoveryCacheException $e) { + $output->writeLine($e->getMessage()); + $output->writeLine($e->getSuggestion()); + + return 1; + } + + $output->writeLine('Discovery cache compiled successfully.'); + $output->writeLine('Cache path: ' . $this->discoveryCache->path()); + $output->writeLine('preferences: ' . count($payload['preferences'])); + $output->writeLine('plugins: ' . count($payload['plugins'])); + $output->writeLine('observers: ' . count($payload['observers'])); + $output->writeLine('commands: ' . count($payload['commands'])); + + return 0; + } +} diff --git a/packages/core/src/Commands/DiscoveryClearCommand.php b/packages/core/src/Commands/DiscoveryClearCommand.php new file mode 100644 index 00000000..34c0e84a --- /dev/null +++ b/packages/core/src/Commands/DiscoveryClearCommand.php @@ -0,0 +1,31 @@ +discoveryCache->clear(); + + $output->writeLine('Discovery cache cleared successfully.'); + + return 0; + } +} diff --git a/packages/core/src/Discovery/DiscoveryCache.php b/packages/core/src/Discovery/DiscoveryCache.php new file mode 100644 index 00000000..b923a358 --- /dev/null +++ b/packages/core/src/Discovery/DiscoveryCache.php @@ -0,0 +1,401 @@ +discoveryEnvironment->cachePath(); + + if ($this->isAbsolutePath($path)) { + return $path; + } + + return $this->projectPaths->base . '/' . $path; + } + + private function isAbsolutePath(string $path): bool + { + return str_starts_with($path, '/') || (strlen($path) >= 3 && ctype_alpha($path[0]) && $path[1] === ':'); + } + + /** + * Returns the absolute path to the cache file (public accessor). + */ + public function path(): string + { + return $this->resolveCachePath(); + } + + /** + * Returns true if the cache file currently exists. + */ + public function exists(): bool + { + return file_exists($this->resolveCachePath()); + } + + /** + * Removes the cache file if it exists. Idempotent — safe to call when absent. + */ + public function clear(): void + { + $path = $this->resolveCachePath(); + if (file_exists($path)) { + unlink($path); + } + } + + /** + * Serializes the discovery payload to a PHP return-array file using var_export. + * + * Writes atomically via a temp file in the same directory, then rename(). + * + * @param array{preferences: PreferenceRecord[], plugins: PluginDefinition[], observers: ObserverDefinition[], commands: CommandDefinition[]} $payload + * + * @throws DiscoveryCacheException + */ + public function write(array $payload): void + { + $path = $this->resolveCachePath(); + $dir = dirname($path); + + if (!is_dir($dir) && !@mkdir($dir, 0755, true) && !is_dir($dir)) { + throw DiscoveryCacheException::notWritable($path); + } + + $data = [ + 'version' => self::CACHE_VERSION, + 'preferences' => array_map( + fn (PreferenceRecord $r) => [ + 'replacement' => $r->replacement, + 'replaces' => $r->replaces, + ], + $payload['preferences'], + ), + 'plugins' => array_map( + fn (PluginDefinition $d) => [ + 'pluginClass' => $d->pluginClass, + 'targetClass' => $d->targetClass, + 'beforeMethods' => $d->beforeMethods, + 'afterMethods' => $d->afterMethods, + ], + $payload['plugins'], + ), + 'observers' => array_map( + fn (ObserverDefinition $d) => [ + 'observerClass' => $d->observerClass, + 'eventClass' => $d->eventClass, + 'priority' => $d->priority, + 'async' => $d->async, + ], + $payload['observers'], + ), + 'commands' => array_map( + fn (CommandDefinition $d) => [ + 'commandClass' => $d->commandClass, + 'name' => $d->name, + 'description' => $d->description, + 'aliases' => $d->aliases, + ], + $payload['commands'], + ), + ]; + + $content = "resolveCachePath(); + + if (!file_exists($path)) { + throw DiscoveryCacheException::unreadable($path); + } + + /** @var mixed $data */ + $data = include $path; + + if (!is_array($data)) { + throw DiscoveryCacheException::malformed($path, 'cache file must return an array'); + } + + $requiredKeys = ['version', 'preferences', 'plugins', 'observers', 'commands']; + foreach ($requiredKeys as $key) { + if (!array_key_exists($key, $data)) { + throw DiscoveryCacheException::malformed($path, "missing required key '$key'"); + } + } + + if ($data['version'] !== self::CACHE_VERSION) { + throw DiscoveryCacheException::versionMismatch($path, (int) $data['version'], self::CACHE_VERSION); + } + + return [ + 'preferences' => $this->hydratePreferences($path, $data['preferences']), + 'plugins' => $this->hydratePlugins($path, $data['plugins']), + 'observers' => $this->hydrateObservers($path, $data['observers']), + 'commands' => $this->hydrateCommands($path, $data['commands']), + ]; + } + + /** + * @param mixed $records + * @return PreferenceRecord[] + * + * @throws DiscoveryCacheException + */ + private function hydratePreferences( + string $path, + mixed $records, + ): array { + if (!is_array($records)) { + throw DiscoveryCacheException::malformed($path, "'preferences' must be an array"); + } + + $result = []; + foreach ($records as $i => $record) { + if (!is_array($record)) { + throw DiscoveryCacheException::malformed($path, "preferences[$i] must be an array"); + } + $this->assertStringField($path, $record, 'preferences', $i, 'replacement'); + $this->assertStringField($path, $record, 'preferences', $i, 'replaces'); + + $result[] = new PreferenceRecord( + replacement: $record['replacement'], + replaces: $record['replaces'], + ); + } + + return $result; + } + + /** + * @param mixed $records + * @return PluginDefinition[] + * + * @throws DiscoveryCacheException + */ + private function hydratePlugins( + string $path, + mixed $records, + ): array { + if (!is_array($records)) { + throw DiscoveryCacheException::malformed($path, "'plugins' must be an array"); + } + + $result = []; + foreach ($records as $i => $record) { + if (!is_array($record)) { + throw DiscoveryCacheException::malformed($path, "plugins[$i] must be an array"); + } + $this->assertStringField($path, $record, 'plugins', $i, 'pluginClass'); + $this->assertStringField($path, $record, 'plugins', $i, 'targetClass'); + $this->assertArrayField($path, $record, 'plugins', $i, 'beforeMethods'); + $this->assertArrayField($path, $record, 'plugins', $i, 'afterMethods'); + + $result[] = new PluginDefinition( + pluginClass: $record['pluginClass'], + targetClass: $record['targetClass'], + beforeMethods: $record['beforeMethods'], + afterMethods: $record['afterMethods'], + ); + } + + return $result; + } + + /** + * @param mixed $records + * @return ObserverDefinition[] + * + * @throws DiscoveryCacheException + */ + private function hydrateObservers( + string $path, + mixed $records, + ): array { + if (!is_array($records)) { + throw DiscoveryCacheException::malformed($path, "'observers' must be an array"); + } + + $result = []; + foreach ($records as $i => $record) { + if (!is_array($record)) { + throw DiscoveryCacheException::malformed($path, "observers[$i] must be an array"); + } + $this->assertStringField($path, $record, 'observers', $i, 'observerClass'); + $this->assertStringField($path, $record, 'observers', $i, 'eventClass'); + $this->assertIntField($path, $record, 'observers', $i, 'priority'); + $this->assertBoolField($path, $record, 'observers', $i, 'async'); + + $result[] = new ObserverDefinition( + observerClass: $record['observerClass'], + eventClass: $record['eventClass'], + priority: $record['priority'], + async: $record['async'], + ); + } + + return $result; + } + + /** + * @param mixed $records + * @return CommandDefinition[] + * + * @throws DiscoveryCacheException + */ + private function hydrateCommands( + string $path, + mixed $records, + ): array { + if (!is_array($records)) { + throw DiscoveryCacheException::malformed($path, "'commands' must be an array"); + } + + $result = []; + foreach ($records as $i => $record) { + if (!is_array($record)) { + throw DiscoveryCacheException::malformed($path, "commands[$i] must be an array"); + } + $this->assertStringField($path, $record, 'commands', $i, 'commandClass'); + $this->assertStringField($path, $record, 'commands', $i, 'name'); + $this->assertStringField($path, $record, 'commands', $i, 'description'); + $this->assertArrayField($path, $record, 'commands', $i, 'aliases'); + + $result[] = new CommandDefinition( + commandClass: $record['commandClass'], + name: $record['name'], + description: $record['description'], + aliases: $record['aliases'], + ); + } + + return $result; + } + + /** + * @param array $record + * + * @throws DiscoveryCacheException + */ + private function assertStringField( + string $path, + array $record, + string $section, + int|string $index, + string $field, + ): void { + if (!array_key_exists($field, $record)) { + throw DiscoveryCacheException::malformed($path, "$section[$index] missing required field '$field'"); + } + if (!is_string($record[$field])) { + throw DiscoveryCacheException::malformed($path, "$section[$index].$field must be a string"); + } + } + + /** + * @param array $record + * + * @throws DiscoveryCacheException + */ + private function assertArrayField( + string $path, + array $record, + string $section, + int|string $index, + string $field, + ): void { + if (!array_key_exists($field, $record)) { + throw DiscoveryCacheException::malformed($path, "$section[$index] missing required field '$field'"); + } + if (!is_array($record[$field])) { + throw DiscoveryCacheException::malformed($path, "$section[$index].$field must be an array"); + } + } + + /** + * @param array $record + * + * @throws DiscoveryCacheException + */ + private function assertIntField( + string $path, + array $record, + string $section, + int|string $index, + string $field, + ): void { + if (!array_key_exists($field, $record)) { + throw DiscoveryCacheException::malformed($path, "$section[$index] missing required field '$field'"); + } + if (!is_int($record[$field])) { + throw DiscoveryCacheException::malformed($path, "$section[$index].$field must be an int"); + } + } + + /** + * @param array $record + * + * @throws DiscoveryCacheException + */ + private function assertBoolField( + string $path, + array $record, + string $section, + int|string $index, + string $field, + ): void { + if (!array_key_exists($field, $record)) { + throw DiscoveryCacheException::malformed($path, "$section[$index] missing required field '$field'"); + } + if (!is_bool($record[$field])) { + throw DiscoveryCacheException::malformed($path, "$section[$index].$field must be a bool"); + } + } +} diff --git a/packages/core/src/Discovery/DiscoveryCompiler.php b/packages/core/src/Discovery/DiscoveryCompiler.php new file mode 100644 index 00000000..6c61ac3d --- /dev/null +++ b/packages/core/src/Discovery/DiscoveryCompiler.php @@ -0,0 +1,106 @@ + $modules + * @return array{version: int, preferences: PreferenceRecord[], plugins: PluginDefinition[], observers: ObserverDefinition[], commands: CommandDefinition[]} + */ + public function compile(array $modules): array + { + $preferences = $this->runPreferenceDiscovery($modules); + $plugins = $this->runPluginDiscovery($modules); + $observers = $this->runObserverDiscovery($modules); + $commands = $this->runCommandDiscovery($modules); + + return [ + 'version' => DiscoveryCache::CACHE_VERSION, + 'preferences' => $preferences, + 'plugins' => $plugins, + 'observers' => $observers, + 'commands' => $commands, + ]; + } + + /** + * @param array $modules + * @return PreferenceRecord[] + */ + private function runPreferenceDiscovery(array $modules): array + { + $discovery = new PreferenceDiscovery(); + $records = []; + + foreach ($modules as $module) { + $records = array_merge($records, $discovery->discoverInModule($module)); + } + + return $records; + } + + /** + * @param array $modules + * @return PluginDefinition[] + */ + private function runPluginDiscovery(array $modules): array + { + $discovery = new PluginDiscovery(); + $definitions = []; + + foreach ($modules as $module) { + $definitions = array_merge($definitions, $discovery->discoverInModule($module)); + } + + return $definitions; + } + + /** + * @param array $modules + * @return ObserverDefinition[] + */ + private function runObserverDiscovery(array $modules): array + { + $discovery = new ObserverDiscovery(new ClassFileParser()); + + return $discovery->discover($modules); + } + + /** + * @param array $modules + * @return CommandDefinition[] + */ + private function runCommandDiscovery(array $modules): array + { + $discovery = new CommandDiscovery(new ClassFileParser()); + + return $discovery->discover($modules); + } +} diff --git a/packages/core/src/Discovery/DiscoveryEnvironment.php b/packages/core/src/Discovery/DiscoveryEnvironment.php new file mode 100644 index 00000000..047f1a22 --- /dev/null +++ b/packages/core/src/Discovery/DiscoveryEnvironment.php @@ -0,0 +1,29 @@ +originalCachePath = array_key_exists('DISCOVERY_CACHE_PATH', $_ENV) + ? $_ENV['DISCOVERY_CACHE_PATH'] + : null; +}); + +afterEach(function (): void { + if ($this->originalCachePath === null) { + unset($_ENV['DISCOVERY_CACHE_PATH']); + } else { + $_ENV['DISCOVERY_CACHE_PATH'] = $this->originalCachePath; + } +}); + +function makeDiscoveryCacheSetup(string $cacheDir): array +{ + $cachePath = $cacheDir . '/discovery.php'; + $_ENV['DISCOVERY_CACHE_PATH'] = $cachePath; + $paths = new ProjectPaths($cacheDir); + $env = new DiscoveryEnvironment(); + $cache = new DiscoveryCache($paths, $env); + $compiler = new DiscoveryCompiler(); + $moduleRepository = new class () implements ModuleRepositoryInterface + { + public function all(): array + { + return []; + } + }; + $command = new DiscoveryCacheCommand($compiler, $cache, $moduleRepository); + $stream = fopen('php://memory', 'r+'); + $input = new Input([]); + $output = new Output($stream); + + return [ + 'cache' => $cache, + 'command' => $command, + 'stream' => $stream, + 'input' => $input, + 'output' => $output, + 'path' => $cachePath, + 'dir' => $cacheDir, + ]; +} + +function discoveryCacheTestCleanup(string $dir): void +{ + if (!is_dir($dir)) { + return; + } + + $files = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($dir, RecursiveDirectoryIterator::SKIP_DOTS), + RecursiveIteratorIterator::CHILD_FIRST, + ); + foreach ($files as $file) { + if ($file->isDir()) { + rmdir($file->getRealPath()); + } else { + unlink($file->getRealPath()); + } + } + rmdir($dir); +} + +it('compiles discovery and writes the cache file, returning exit code 0', function (): void { + $dir = sys_get_temp_dir() . '/marko_cache_cmd_test_' . uniqid('', true); + $setup = makeDiscoveryCacheSetup($dir); + + $exitCode = $setup['command']->execute($setup['input'], $setup['output']); + + expect($exitCode)->toBe(0) + ->and(file_exists($setup['path']))->toBeTrue(); + + fclose($setup['stream']); + discoveryCacheTestCleanup($dir); +}); + +it('writes a cache file that DiscoveryCache reports as existing after the command runs', function (): void { + $dir = sys_get_temp_dir() . '/marko_cache_cmd_test_' . uniqid('', true); + $setup = makeDiscoveryCacheSetup($dir); + + $setup['command']->execute($setup['input'], $setup['output']); + + expect($setup['cache']->exists())->toBeTrue(); + + fclose($setup['stream']); + discoveryCacheTestCleanup($dir); +}); + +it( + 'reports the cache file path and the counts of cached preferences, plugins, observers, and commands', + function (): void { + $dir = sys_get_temp_dir() . '/marko_cache_cmd_test_' . uniqid('', true); + $setup = makeDiscoveryCacheSetup($dir); + + $setup['command']->execute($setup['input'], $setup['output']); + + rewind($setup['stream']); + $result = stream_get_contents($setup['stream']); + + expect($result)->toContain($setup['path']) + ->and($result)->toContain('preferences') + ->and($result)->toContain('plugins') + ->and($result)->toContain('observers') + ->and($result)->toContain('commands'); + + fclose($setup['stream']); + discoveryCacheTestCleanup($dir); + }, +); + +it( + 'returns a non-zero exit code and a helpful message (catching DiscoveryCacheException::notWritable) when the cache cannot be written', + function (): void { + $dir = sys_get_temp_dir() . '/marko_cache_cmd_test_' . uniqid('', true); + // Create a read-only directory so the write will fail + mkdir($dir, 0555, true); + $cachePath = $dir . '/discovery.php'; + $_ENV['DISCOVERY_CACHE_PATH'] = $cachePath; + $paths = new ProjectPaths($dir); + $env = new DiscoveryEnvironment(); + $cache = new DiscoveryCache($paths, $env); + $compiler = new DiscoveryCompiler(); + $moduleRepository = new class () implements ModuleRepositoryInterface + { + public function all(): array + { + return []; + } + }; + $command = new DiscoveryCacheCommand($compiler, $cache, $moduleRepository); + $stream = fopen('php://memory', 'r+'); + $input = new Input([]); + $output = new Output($stream); + + $exitCode = $command->execute($input, $output); + + rewind($stream); + $result = stream_get_contents($stream); + + expect($exitCode)->not->toBe(0) + ->and($result)->not->toBeEmpty(); + + fclose($stream); + chmod($dir, 0755); + discoveryCacheTestCleanup($dir); + }, +); + +it('overwrites a pre-existing cache file with freshly compiled content', function (): void { + $dir = sys_get_temp_dir() . '/marko_cache_cmd_test_' . uniqid('', true); + mkdir($dir, 0755, true); + $setup = makeDiscoveryCacheSetup($dir); + + // Write stale content + file_put_contents($setup['path'], ' true];'); + + $exitCode = $setup['command']->execute($setup['input'], $setup['output']); + + $content = (string) file_get_contents($setup['path']); + + expect($exitCode)->toBe(0) + ->and($content)->not->toContain('"stale"') + ->and($content)->toContain('return '); + + fclose($setup['stream']); + discoveryCacheTestCleanup($dir); +}); diff --git a/packages/core/tests/Command/DiscoveryClearCommandTest.php b/packages/core/tests/Command/DiscoveryClearCommandTest.php new file mode 100644 index 00000000..4fc9da8c --- /dev/null +++ b/packages/core/tests/Command/DiscoveryClearCommandTest.php @@ -0,0 +1,114 @@ +originalCachePath = array_key_exists('DISCOVERY_CACHE_PATH', $_ENV) + ? $_ENV['DISCOVERY_CACHE_PATH'] + : null; +}); + +afterEach(function (): void { + if ($this->originalCachePath === null) { + unset($_ENV['DISCOVERY_CACHE_PATH']); + } else { + $_ENV['DISCOVERY_CACHE_PATH'] = $this->originalCachePath; + } +}); + +function makeDiscoveryClearSetup(string $cacheDir): array +{ + $cachePath = $cacheDir . '/discovery.php'; + $_ENV['DISCOVERY_CACHE_PATH'] = $cachePath; + $paths = new ProjectPaths($cacheDir); + $env = new DiscoveryEnvironment(); + $cache = new DiscoveryCache($paths, $env); + $command = new DiscoveryClearCommand($cache); + $stream = fopen('php://memory', 'r+'); + $input = new Input([]); + $output = new Output($stream); + + return [ + 'cache' => $cache, + 'command' => $command, + 'stream' => $stream, + 'input' => $input, + 'output' => $output, + 'path' => $cachePath, + ]; +} + +it('removes an existing cache file and returns exit code 0', function (): void { + $dir = sys_get_temp_dir() . '/marko_clear_test_' . uniqid('', true); + mkdir($dir, 0755, true); + $setup = makeDiscoveryClearSetup($dir); + + // Create the cache file + file_put_contents($setup['path'], 'execute($setup['input'], $setup['output']); + + expect($exitCode)->toBe(0) + ->and(file_exists($setup['path']))->toBeFalse(); + + fclose($setup['stream']); + rmdir($dir); +}); + +it('reports the cache as cleared via writeLine', function (): void { + $dir = sys_get_temp_dir() . '/marko_clear_test_' . uniqid('', true); + mkdir($dir, 0755, true); + $setup = makeDiscoveryClearSetup($dir); + + file_put_contents($setup['path'], 'execute($setup['input'], $setup['output']); + + rewind($setup['stream']); + $result = stream_get_contents($setup['stream']); + + expect($result)->toContain('Discovery cache cleared'); + + fclose($setup['stream']); + rmdir($dir); +}); + +it('returns exit code 0 and reports success when no cache file exists', function (): void { + $dir = sys_get_temp_dir() . '/marko_clear_test_' . uniqid('', true); + mkdir($dir, 0755, true); + $setup = makeDiscoveryClearSetup($dir); + + // Do NOT create the cache file + $exitCode = $setup['command']->execute($setup['input'], $setup['output']); + + rewind($setup['stream']); + $result = stream_get_contents($setup['stream']); + + expect($exitCode)->toBe(0) + ->and($result)->toContain('Discovery cache cleared'); + + fclose($setup['stream']); + rmdir($dir); +}); + +it('leaves DiscoveryCache exists() false after running', function (): void { + $dir = sys_get_temp_dir() . '/marko_clear_test_' . uniqid('', true); + mkdir($dir, 0755, true); + $setup = makeDiscoveryClearSetup($dir); + + file_put_contents($setup['path'], 'execute($setup['input'], $setup['output']); + + expect($setup['cache']->exists())->toBeFalse(); + + fclose($setup['stream']); + rmdir($dir); +}); diff --git a/packages/core/tests/Unit/ApplicationDiscoveryCacheTest.php b/packages/core/tests/Unit/ApplicationDiscoveryCacheTest.php new file mode 100644 index 00000000..e1569d1a --- /dev/null +++ b/packages/core/tests/Unit/ApplicationDiscoveryCacheTest.php @@ -0,0 +1,752 @@ +write($payload); +} + +/** + * Write a corrupt (non-PHP-return-array) cache file so DiscoveryCache::load() throws. + */ +function cacheTestWriteCorruptCache(string $basePath): void +{ + $cachePath = $basePath . '/storage/cache/discovery.php'; + $dir = dirname($cachePath); + if (!is_dir($dir)) { + mkdir($dir, 0755, true); + } + file_put_contents($cachePath, ' $name, + 'version' => '1.0.0', + 'extra' => ['marko' => ['module' => true]], + ], JSON_PRETTY_PRINT)); +} + +/** + * Create a module with a command class in src/. + * Returns the FQCN of the command class and its CLI name attribute value. + * + * @return array{class: string, commandName: string} + */ +function cacheTestCreateCommandModule(string $modulePath, string $moduleName, string $uniqueId): array +{ + cacheTestCreateModule($modulePath, $moduleName); + mkdir($modulePath . '/src', 0755, true); + + $ns = "CacheTestCmd$uniqueId"; + $commandName = "cache:test-$uniqueId"; + + $code = << "$ns\\CachedCommand", 'commandName' => $commandName]; +} + +/** + * Build an empty valid payload (no definitions in any subsystem). + * + * @return array{preferences: PreferenceRecord[], plugins: PluginDefinition[], observers: ObserverDefinition[], commands: CommandDefinition[]} + */ +function cacheTestEmptyPayload(): array +{ + return [ + 'preferences' => [], + 'plugins' => [], + 'observers' => [], + 'commands' => [], + ]; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +it( + 'hydrates preferences, plugins, observers, and commands from the cache when enabled and environment is not development and the cache exists', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + cacheTestCreateModule($vendorDir . '/acme/core', 'acme/core'); + + // Use real Application class as a plugin target (it definitely exists) + $prefRecord = new PreferenceRecord( + replacement: 'Acme\\Replacement', + replaces: 'Acme\\Original', + ); + $observerDef = new ObserverDefinition( + observerClass: 'Acme\\Observer', + eventClass: 'Acme\\Event', + priority: 0, + async: false, + ); + $commandDef = new CommandDefinition( + commandClass: 'Acme\\Cmd', + name: 'acme:hydrate-cmd', + description: 'Test', + aliases: [], + ); + + // Use Application as a real target class for the plugin (so ReflectionClass succeeds) + $pluginDef = new PluginDefinition( + pluginClass: Application::class, + targetClass: Application::class, + beforeMethods: [], + afterMethods: [], + ); + + $payload = [ + 'preferences' => [$prefRecord], + 'plugins' => [$pluginDef], + 'observers' => [$observerDef], + 'commands' => [$commandDef], + ]; + + cacheTestWriteCache($baseDir, $payload); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + // Preferences hydrated + expect($app->preferenceRegistry->getPreference('Acme\\Original')) + ->toBe('Acme\\Replacement') + // Plugins hydrated — Application class is the target + ->and($app->pluginRegistry->hasPluginsFor(Application::class))->toBeTrue() + // Observers hydrated + ->and($app->observerRegistry->getObserversFor('Acme\\Event'))->toHaveCount(1) + // Commands hydrated + ->and($app->commandRegistry->has('acme:hydrate-cmd'))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'binds CommandRegistry in the container and creates the CommandRunner on a cache hit (commands are runnable without a scan)', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + cacheTestCreateModule($vendorDir . '/acme/core', 'acme/core'); + + // Write an empty cache — we only need to confirm the wiring + cacheTestWriteCache($baseDir, cacheTestEmptyPayload()); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + // CommandRegistry must be bound in the container + $resolved = $app->container->get(CommandRegistry::class); + + expect($app->commandRegistry)->toBeInstanceOf(CommandRegistry::class) + ->and($app->commandRunner)->toBeInstanceOf(CommandRunner::class) + ->and($resolved)->toBe($app->commandRegistry); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'registers each cached command exactly once and never throws duplicateCommandName on a cache hit (scan and cache halves are mutually exclusive)', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + // Build a cache that lists the same command that is also on disk + $payload = [ + 'preferences' => [], + 'plugins' => [], + 'observers' => [], + 'commands' => [ + new CommandDefinition( + commandClass: $info['class'], + name: $info['commandName'], + description: 'Cache test command', + aliases: [], + ), + ], + ]; + cacheTestWriteCache($baseDir, $payload); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + + // Must not throw CommandException::duplicateCommandName + $app->initialize(); + + $all = $app->commandRegistry->all(); + $count = count(array_filter($all, fn ($d) => $d->name === $info['commandName'])); + + expect($count)->toBe(1); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'preserves the existing boot ordering on a cache hit (observers registered before the event dispatcher is created; commands after the module repository is bound)', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + cacheTestCreateModule($vendorDir . '/acme/core', 'acme/core'); + + $observerDef = new ObserverDefinition( + observerClass: 'Acme\\Observer', + eventClass: 'Acme\\Event', + priority: 0, + async: false, + ); + $commandDef = new CommandDefinition( + commandClass: 'Acme\\Cmd', + name: 'acme:order-cmd', + description: 'Test', + aliases: [], + ); + + cacheTestWriteCache($baseDir, [ + 'preferences' => [], + 'plugins' => [], + 'observers' => [$observerDef], + 'commands' => [$commandDef], + ]); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + // EventDispatcher was created AFTER observers — so observers must be registered + $observers = $app->observerRegistry->getObserversFor('Acme\\Event'); + + // CommandRegistry must be populated (commands ran after module-repo wiring) + expect($observers)->toHaveCount(1) + ->and($app->commandRegistry->has('acme:order-cmd'))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'does not consult the filesystem for markers on a cache hit (a class present on disk but absent from the cache is not registered)', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + // Write an EMPTY cache — the command exists on disk but is intentionally absent + cacheTestWriteCache($baseDir, cacheTestEmptyPayload()); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + // The command is on disk but omitted from the cache → must NOT be registered + expect($app->commandRegistry->has($info['commandName']))->toBeFalse(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'always rescans the filesystem in development even when a cache file exists (a class present on disk is registered regardless of cache contents)', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + // Write an EMPTY cache — command is on disk but absent from cache + cacheTestWriteCache($baseDir, cacheTestEmptyPayload()); + + // In development the cache must be ignored → rescan → command is found + $_ENV['APP_ENV'] = 'development'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + expect($app->commandRegistry->has($info['commandName']))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'rescans the filesystem when caching is enabled but no cache file exists', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + // No cache file written — rescan must happen + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + expect($app->commandRegistry->has($info['commandName']))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'rescans the filesystem when caching is disabled by env (DISCOVERY_CACHE_ENABLED=false)', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + // Write a cache that omits the command + cacheTestWriteCache($baseDir, cacheTestEmptyPayload()); + + // Disabled → rescan must occur → command is found despite empty cache + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = 'false'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + expect($app->commandRegistry->has($info['commandName']))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'throws DiscoveryCacheException during boot when the cache exists but is corrupt and caching is enabled outside development', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + cacheTestCreateModule($vendorDir . '/acme/core', 'acme/core'); + + cacheTestWriteCorruptCache($baseDir); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + + expect(fn () => $app->initialize())->toThrow(DiscoveryCacheException::class); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'skips the corrupt-cache throw in development (rescans instead) so a corrupt cache never blocks dev boot', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + cacheTestWriteCorruptCache($baseDir); + + // Development mode: corrupt cache must be ignored, rescan proceeds + $_ENV['APP_ENV'] = 'development'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + expect($app->commandRegistry->has($info['commandName']))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'produces the same registered preferences, plugins, observers, and commands from a valid cache as from a fresh scan of the same modules', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + + // --- SCAN boot (development, no cache) --- + $scanBaseDir = sys_get_temp_dir() . '/marko-cache-scan-' . $uniqueId; + $scanVendorDir = $scanBaseDir . '/vendor'; + $scanInfo = cacheTestCreateCommandModule( + $scanVendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + $_ENV['APP_ENV'] = 'development'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $scanApp = new Application( + vendorPath: $scanVendorDir, + modulesPath: '', + appPath: '', + ); + $scanApp->initialize(); + + $scanCommands = $scanApp->commandRegistry->all(); + + // --- CACHE boot (production, pre-written cache matching the scan) --- + $cacheBaseDir = sys_get_temp_dir() . '/marko-cache-hit-' . $uniqueId; + $cacheVendorDir = $cacheBaseDir . '/vendor'; + cacheTestCreateModule($cacheVendorDir . '/acme/core', 'acme/core'); + + // Build cache payload mirroring what scan would produce + $commandDef = new CommandDefinition( + commandClass: $scanInfo['class'], + name: $scanInfo['commandName'], + description: 'Cache test command', + aliases: [], + ); + cacheTestWriteCache($cacheBaseDir, [ + 'preferences' => [], + 'plugins' => [], + 'observers' => [], + 'commands' => [$commandDef], + ]); + + $_ENV['APP_ENV'] = 'production'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = '1'; + + $cacheApp = new Application( + vendorPath: $cacheVendorDir, + modulesPath: '', + appPath: '', + ); + $cacheApp->initialize(); + + $cacheCommands = $cacheApp->commandRegistry->all(); + + // Both paths must produce the same command names + $scanNames = array_map(fn ($d) => $d->name, $scanCommands); + $cacheNames = array_map(fn ($d) => $d->name, $cacheCommands); + sort($scanNames); + sort($cacheNames); + + expect($cacheNames)->toBe($scanNames); + + cacheTestCleanupDirectory($scanBaseDir); + cacheTestCleanupDirectory($cacheBaseDir); + } finally { + $restore(); + } + }, +); + +it( + 'rescans (defaults to enabled production) when marko/env is not installed and no env vars are set', + function (): void { + $restore = cacheTestSnapshotEnv(); + + try { + $uniqueId = bin2hex(random_bytes(8)); + $baseDir = sys_get_temp_dir() . '/marko-cache-test-' . $uniqueId; + $vendorDir = $baseDir . '/vendor'; + + $info = cacheTestCreateCommandModule( + $vendorDir . '/acme/core', + 'acme/core', + $uniqueId, + ); + + // No env vars set, no cache file → defaults to enabled+production → $cache->exists() is false → rescan + unset($_ENV['APP_ENV'], $_ENV['DISCOVERY_CACHE_ENABLED'], $_ENV['DISCOVERY_CACHE_PATH']); + + $app = new Application( + vendorPath: $vendorDir, + modulesPath: '', + appPath: '', + ); + $app->initialize(); + + // Rescan ran, command is found + expect($app->commandRegistry->has($info['commandName']))->toBeTrue(); + + cacheTestCleanupDirectory($baseDir); + } finally { + $restore(); + } + }, +); + +it( + 'restores $_ENV and removes any temp cache file after each test so other Application::initialize() tests are unaffected', + function (): void { + // This is a meta-test confirming the restore helper works correctly. + $restore = cacheTestSnapshotEnv(); + + $originalAppEnv = $_ENV['APP_ENV'] ?? null; + $originalEnabled = $_ENV['DISCOVERY_CACHE_ENABLED'] ?? null; + + try { + $_ENV['APP_ENV'] = 'staging'; + $_ENV['DISCOVERY_CACHE_ENABLED'] = 'false'; + + // Verify mutation happened + expect($_ENV['APP_ENV'])->toBe('staging') + ->and($_ENV['DISCOVERY_CACHE_ENABLED'])->toBe('false'); + } finally { + $restore(); + } + + // After restore the env must match what was there before + $restoredAppEnv = $_ENV['APP_ENV'] ?? null; + $restoredEnabled = $_ENV['DISCOVERY_CACHE_ENABLED'] ?? null; + + expect($restoredAppEnv)->toBe($originalAppEnv) + ->and($restoredEnabled)->toBe($originalEnabled); + }, +); diff --git a/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php b/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php new file mode 100644 index 00000000..2e773a93 --- /dev/null +++ b/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php @@ -0,0 +1,406 @@ + new DiscoveryCache($paths, $env), + 'path' => $cachePath, + 'dir' => $cacheDir, + ]; +} + +function emptyPayload(): array +{ + return [ + 'preferences' => [], + 'plugins' => [], + 'observers' => [], + 'commands' => [], + ]; +} + +function samplePayload(): array +{ + return [ + 'preferences' => [ + new PreferenceRecord( + replacement: 'App\MyLogger', + replaces: 'Marko\Core\Logger', + ), + ], + 'plugins' => [ + new PluginDefinition( + pluginClass: 'App\PricePlugin', + targetClass: 'App\Product', + beforeMethods: [ + 'getPrice' => ['pluginMethod' => 'beforeGetPrice', 'sortOrder' => 10], + ], + afterMethods: [ + 'getPrice' => ['pluginMethod' => 'afterGetPrice', 'sortOrder' => 20], + ], + ), + ], + 'observers' => [ + new ObserverDefinition( + observerClass: 'App\OrderObserver', + eventClass: 'App\OrderPlaced', + priority: 5, + async: true, + ), + ], + 'commands' => [ + new CommandDefinition( + commandClass: 'App\CacheCommand', + name: 'cache:clear', + description: 'Clears the cache', + aliases: ['cc'], + ), + ], + ]; +} + +describe('DiscoveryCache', function (): void { + beforeEach(function (): void { + $this->tmpDir = sys_get_temp_dir() . '/marko_discovery_cache_test_' . uniqid('', true); + $this->originalCachePath = array_key_exists('DISCOVERY_CACHE_PATH', $_ENV) + ? $_ENV['DISCOVERY_CACHE_PATH'] + : null; + }); + + afterEach(function (): void { + // Restore env + if ($this->originalCachePath === null) { + unset($_ENV['DISCOVERY_CACHE_PATH']); + } else { + $_ENV['DISCOVERY_CACHE_PATH'] = $this->originalCachePath; + } + + // Clean up temp directory + if (is_dir($this->tmpDir)) { + $files = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($this->tmpDir, RecursiveDirectoryIterator::SKIP_DOTS), + RecursiveIteratorIterator::CHILD_FIRST, + ); + foreach ($files as $file) { + if ($file->isDir()) { + rmdir($file->getRealPath()); + } else { + unlink($file->getRealPath()); + } + } + rmdir($this->tmpDir); + } + }); + + it( + 'writes a discovery payload to a return-array PHP file at the configured cache path and creates the directory if missing', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $cache = $setup['cache']; + $path = $setup['path']; + + expect(is_dir($this->tmpDir))->toBeFalse(); + + $cache->write(emptyPayload()); + + expect(file_exists($path))->toBeTrue() + ->and(is_dir($this->tmpDir))->toBeTrue(); + + $content = (string) file_get_contents($path); + expect($content)->toStartWith('and($content)->toContain('return '); + }, + ); + + it( + 'reports exists() true after a write and false before any write or after clear', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $cache = $setup['cache']; + + expect($cache->exists())->toBeFalse(); + + $cache->write(emptyPayload()); + expect($cache->exists())->toBeTrue(); + + $cache->clear(); + expect($cache->exists())->toBeFalse(); + }, + ); + + it( + 'clears the cache file and clear() is idempotent when the file is already absent', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $cache = $setup['cache']; + + // Should not throw when file doesn't exist + $cache->clear(); + expect($cache->exists())->toBeFalse(); + + // Write, clear, clear again — still no exception + $cache->write(emptyPayload()); + $cache->clear(); + $cache->clear(); + expect($cache->exists())->toBeFalse(); + }, + ); + + it( + 'loads a written cache back into PreferenceRecord, PluginDefinition, ObserverDefinition, and CommandDefinition objects identical to the payload', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $cache = $setup['cache']; + $payload = samplePayload(); + + $cache->write($payload); + $loaded = $cache->load(); + + expect($loaded['preferences'])->toHaveCount(1) + ->and($loaded['preferences'][0])->toBeInstanceOf(PreferenceRecord::class) + ->and($loaded['preferences'][0]->replacement)->toBe('App\MyLogger') + ->and($loaded['preferences'][0]->replaces)->toBe('Marko\Core\Logger') + ->and($loaded['plugins'])->toHaveCount(1) + ->and($loaded['plugins'][0])->toBeInstanceOf(PluginDefinition::class) + ->and($loaded['plugins'][0]->pluginClass)->toBe('App\PricePlugin') + ->and($loaded['plugins'][0]->targetClass)->toBe('App\Product') + ->and($loaded['observers'])->toHaveCount(1) + ->and($loaded['observers'][0])->toBeInstanceOf(ObserverDefinition::class) + ->and($loaded['observers'][0]->observerClass)->toBe('App\OrderObserver') + ->and($loaded['observers'][0]->eventClass)->toBe('App\OrderPlaced') + ->and($loaded['observers'][0]->priority)->toBe(5) + ->and($loaded['observers'][0]->async)->toBeTrue() + ->and($loaded['commands'])->toHaveCount(1) + ->and($loaded['commands'][0])->toBeInstanceOf(CommandDefinition::class) + ->and($loaded['commands'][0]->commandClass)->toBe('App\CacheCommand') + ->and($loaded['commands'][0]->name)->toBe('cache:clear') + ->and($loaded['commands'][0]->description)->toBe('Clears the cache') + ->and($loaded['commands'][0]->aliases)->toBe(['cc']); + }, + ); + + it( + 'round-trips PluginDefinition beforeMethods/afterMethods associative arrays preserving target-method keys and the pluginMethod/sortOrder shape', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $cache = $setup['cache']; + $before = [ + 'getPrice' => ['pluginMethod' => 'beforeGetPrice', 'sortOrder' => 10], + 'getTitle' => ['pluginMethod' => 'beforeGetTitle', 'sortOrder' => 5], + ]; + $after = [ + 'getPrice' => ['pluginMethod' => 'afterGetPrice', 'sortOrder' => 20], + ]; + $payload = [ + 'preferences' => [], + 'plugins' => [ + new PluginDefinition( + pluginClass: 'App\Plugin', + targetClass: 'App\Target', + beforeMethods: $before, + afterMethods: $after, + ), + ], + 'observers' => [], + 'commands' => [], + ]; + + $cache->write($payload); + $loaded = $cache->load(); + + expect($loaded['plugins'][0]->beforeMethods)->toBe($before) + ->and($loaded['plugins'][0]->afterMethods)->toBe($after); + }, + ); + + it( + 'round-trips empty definition arrays and empty aliases/beforeMethods/afterMethods without turning associative arrays into lists', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $cache = $setup['cache']; + + $cache->write(emptyPayload()); + $loaded = $cache->load(); + + expect($loaded['preferences'])->toBeEmpty() + ->and($loaded['plugins'])->toBeEmpty() + ->and($loaded['observers'])->toBeEmpty() + ->and($loaded['commands'])->toBeEmpty(); + }, + ); + + it( + 'resolves a relative cache_path against the project base path and uses an absolute cache_path unchanged', + function (): void { + // Relative path: resolve against base + $relPath = 'storage/cache/discovery.php'; + $_ENV['DISCOVERY_CACHE_PATH'] = $relPath; + $paths = new ProjectPaths($this->tmpDir); + $env = new DiscoveryEnvironment(); + $cacheRelative = new DiscoveryCache($paths, $env); + + $cacheRelative->write(emptyPayload()); + $expectedAbsPath = $this->tmpDir . '/' . $relPath; + expect(file_exists($expectedAbsPath))->toBeTrue(); + + // Absolute path: use as-is + $absPath = $this->tmpDir . '/abs/discovery.php'; + $_ENV['DISCOVERY_CACHE_PATH'] = $absPath; + $env2 = new DiscoveryEnvironment(); + $cacheAbsolute = new DiscoveryCache($paths, $env2); + + $cacheAbsolute->write(emptyPayload()); + expect(file_exists($absPath))->toBeTrue(); + }, + ); + + it( + 'throws DiscoveryCacheException when the cache file content is not a PHP array', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $path = $setup['path']; + $cache = $setup['cache']; + + mkdir($this->tmpDir, 0755, true); + file_put_contents($path, ' $cache->load())->toThrow(DiscoveryCacheException::class); + }, + ); + + it( + 'throws DiscoveryCacheException when the cache file is missing required keys (version, preferences, plugins, observers, commands)', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $path = $setup['path']; + $cache = $setup['cache']; + + mkdir($this->tmpDir, 0755, true); + file_put_contents($path, ' 1];'); + + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + }, + ); + + it( + 'throws DiscoveryCacheException when the cache version key does not match the current cache schema version', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $path = $setup['path']; + $cache = $setup['cache']; + + mkdir($this->tmpDir, 0755, true); + file_put_contents( + $path, + ' 9999, "preferences" => [], "plugins" => [], "observers" => [], "commands" => []];', + ); + + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + }, + ); + + it( + 'throws DiscoveryCacheException when a record within a section is missing a required field (e.g. a plugin entry without targetClass)', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $path = $setup['path']; + $cache = $setup['cache']; + $version = DiscoveryCache::CACHE_VERSION; + + mkdir($this->tmpDir, 0755, true); + file_put_contents( + $path, + " $version, 'preferences' => [], 'plugins' => [['pluginClass' => 'App\\\\Plugin']], 'observers' => [], 'commands' => []];", + ); + + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + }, + ); + + it( + 'throws DiscoveryCacheException when a record field has the wrong type (e.g. observer priority is a string, command aliases is not an array, plugin beforeMethods is not an array)', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $path = $setup['path']; + $cache = $setup['cache']; + $version = DiscoveryCache::CACHE_VERSION; + + mkdir($this->tmpDir, 0755, true); + + // Observer with priority as string + file_put_contents( + $path, + " $version, 'preferences' => [], 'plugins' => [], 'observers' => [['observerClass' => 'App\\\\Obs', 'eventClass' => 'App\\\\Evt', 'priority' => 'high', 'async' => false]], 'commands' => []];", + ); + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + + // Command aliases as string instead of array + file_put_contents( + $path, + " $version, 'preferences' => [], 'plugins' => [], 'observers' => [], 'commands' => [['commandClass' => 'App\\\\Cmd', 'name' => 'cmd', 'description' => '', 'aliases' => 'not-array']]];", + ); + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + + // Plugin beforeMethods as string + file_put_contents( + $path, + " $version, 'preferences' => [], 'plugins' => [['pluginClass' => 'App\\\\P', 'targetClass' => 'App\\\\T', 'beforeMethods' => 'wrong', 'afterMethods' => []]], 'observers' => [], 'commands' => []];", + ); + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + }, + ); + + it( + 'throws DiscoveryCacheException::notWritable when the target directory cannot be created or the file/rename cannot be written (never returns false silently)', + function (): void { + // Point to a path under a non-writable location + $unwritable = '/root/no_access_' . uniqid('', true) . '/discovery.php'; + $_ENV['DISCOVERY_CACHE_PATH'] = $unwritable; + $paths = new ProjectPaths($this->tmpDir); + $env = new DiscoveryEnvironment(); + $cache = new DiscoveryCache($paths, $env); + + expect(fn () => $cache->write(emptyPayload()))->toThrow(DiscoveryCacheException::class); + }, + ); + + it( + 'throws DiscoveryCacheException whose suggestion names the cache file path to delete as the primary recovery, plus discovery:clear, when content is corrupt', + function (): void { + $setup = makeCacheSetup($this->tmpDir); + $path = $setup['path']; + $cache = $setup['cache']; + + mkdir($this->tmpDir, 0755, true); + file_put_contents($path, 'load(); + } catch (DiscoveryCacheException $e) { + $exception = $e; + } + + expect($exception)->not->toBeNull() + ->and($exception->getSuggestion())->toContain($path) + ->and($exception->getSuggestion())->toContain('discovery:clear'); + }, + ); +}); diff --git a/packages/core/tests/Unit/Discovery/DiscoveryCompilerTest.php b/packages/core/tests/Unit/Discovery/DiscoveryCompilerTest.php new file mode 100644 index 00000000..506669ea --- /dev/null +++ b/packages/core/tests/Unit/Discovery/DiscoveryCompilerTest.php @@ -0,0 +1,349 @@ + $name, + 'extra' => ['marko' => ['module' => true]], + ])); + + return [ + 'dir' => $tempDir, + 'manifest' => new ModuleManifest( + name: $name, + version: '1.0.0', + path: $tempDir, + ), + ]; +} + +// Recursively remove a directory +function compilerTestCleanup(string $dir): void +{ + if (!is_dir($dir)) { + return; + } + + $items = scandir($dir); + foreach ($items as $item) { + if ($item === '.' || $item === '..') { + continue; + } + $path = $dir . '/' . $item; + if (is_dir($path)) { + compilerTestCleanup($path); + } else { + unlink($path); + } + } + rmdir($dir); +} + +describe('DiscoveryCompiler', function (): void { + beforeEach(function (): void { + $this->originalCachePath = array_key_exists('DISCOVERY_CACHE_PATH', $_ENV) + ? $_ENV['DISCOVERY_CACHE_PATH'] + : null; + }); + + afterEach(function (): void { + if ($this->originalCachePath === null) { + unset($_ENV['DISCOVERY_CACHE_PATH']); + } else { + $_ENV['DISCOVERY_CACHE_PATH'] = $this->originalCachePath; + } + }); + + it( + 'compiles an empty payload (empty preference, plugin, observer, command arrays) for modules with no attribute-bearing classes', + function (): void { + $module = makeCompilerTestModule('test/empty-module'); + $compiler = new DiscoveryCompiler(); + + $payload = $compiler->compile([$module['manifest']]); + + expect($payload)->toBeArray() + ->and($payload['preferences'])->toBeEmpty() + ->and($payload['plugins'])->toBeEmpty() + ->and($payload['observers'])->toBeEmpty() + ->and($payload['commands'])->toBeEmpty(); + + compilerTestCleanup($module['dir']); + }, + ); + + it( + 'includes the current cache schema version key sourced from DiscoveryCache::CACHE_VERSION (not a hardcoded literal) in the compiled payload', + function (): void { + $module = makeCompilerTestModule('test/version-module'); + $compiler = new DiscoveryCompiler(); + + $payload = $compiler->compile([$module['manifest']]); + + expect($payload)->toHaveKey('version') + ->and($payload['version'])->toBe(DiscoveryCache::CACHE_VERSION); + + compilerTestCleanup($module['dir']); + }, + ); + + it( + 'compiles preferences discovered across all modules into the payload preferences array', + function (): void { + $module = makeCompilerTestModule('test/pref-module'); + + // Write a PHP file with a #[Preference] class into the module's src/ + $preferenceCode = <<<'PHP' +compile([$module['manifest']]); + + expect($payload['preferences'])->toHaveCount(1) + ->and($payload['preferences'][0]->replacement)->toBe('DiscoveryCompilerTest\\Pref\\ReplacementService') + ->and($payload['preferences'][0]->replaces)->toBe('DiscoveryCompilerTest\\Pref\\OriginalService'); + + compilerTestCleanup($module['dir']); + }, + ); + + it( + 'compiles plugins, observers, and commands discovered across all modules into their respective payload arrays', + function (): void { + $module = makeCompilerTestModule('test/all-types-module'); + + // Plugin class + $pluginCode = <<<'PHP' +compile([$module['manifest']]); + + expect($payload['plugins'])->toHaveCount(1) + ->and($payload['plugins'][0]->pluginClass)->toBe('DiscoveryCompilerTest\\AllTypes\\ServicePlugin') + ->and($payload['plugins'][0]->targetClass)->toBe('DiscoveryCompilerTest\\AllTypes\\TargetService') + ->and($payload['observers'])->toHaveCount(1) + ->and($payload['observers'][0]->observerClass)->toBe('DiscoveryCompilerTest\\AllTypes\\TestObserver') + ->and($payload['observers'][0]->eventClass)->toBe('DiscoveryCompilerTest\\AllTypes\\TestEvent') + ->and($payload['observers'][0]->priority)->toBe(5) + ->and($payload['commands'])->toHaveCount(1) + ->and($payload['commands'][0]->commandClass)->toBe('DiscoveryCompilerTest\\AllTypes\\TestCommand') + ->and($payload['commands'][0]->name)->toBe('test:run') + ->and($payload['commands'][0]->aliases)->toBe(['tr']); + + compilerTestCleanup($module['dir']); + }, + ); + + it( + 'produces a payload that, written and reloaded through DiscoveryCache, yields discovery objects identical to running the four discovery passes directly over the same modules', + function (): void { + $module = makeCompilerTestModule('test/equivalence-module'); + $cacheDir = sys_get_temp_dir() . '/marko_compiler_cache_' . bin2hex(random_bytes(8)); + + // Write a preference class + $preferenceCode = <<<'PHP' +compile([$module['manifest']]); + $cache->write($payload); + $loaded = $cache->load(); + + // Run the four discovery passes directly (same construction as Application) + $preferenceDiscovery = new PreferenceDiscovery(); + $directPreferences = $preferenceDiscovery->discoverInModule($module['manifest']); + + $pluginDiscovery = new PluginDiscovery(); + $directPlugins = $pluginDiscovery->discoverInModule($module['manifest']); + + $observerDiscovery = new ObserverDiscovery(new ClassFileParser()); + $directObservers = $observerDiscovery->discover([$module['manifest']]); + + $commandDiscovery = new CommandDiscovery(new ClassFileParser()); + $directCommands = $commandDiscovery->discover([$module['manifest']]); + + expect($loaded['preferences'])->toHaveCount(count($directPreferences)) + ->and($loaded['plugins'])->toHaveCount(count($directPlugins)) + ->and($loaded['observers'])->toHaveCount(count($directObservers)) + ->and($loaded['commands'])->toHaveCount(count($directCommands)); + + // Verify preference round-trips correctly + expect($loaded['preferences'][0]->replacement)->toBe($directPreferences[0]->replacement) + ->and($loaded['preferences'][0]->replaces)->toBe($directPreferences[0]->replaces); + + compilerTestCleanup($module['dir']); + compilerTestCleanup($cacheDir); + + unset($_ENV['DISCOVERY_CACHE_PATH']); + }, + ); + + it( + 'aggregates results from multiple modules preserving the module load order', + function (): void { + $moduleA = makeCompilerTestModule('test/module-a'); + $moduleB = makeCompilerTestModule('test/module-b'); + + // Module A: preference + $prefA = <<<'PHP' +compile([$moduleA['manifest'], $moduleB['manifest']]); + + expect($payload['preferences'])->toHaveCount(2) + ->and($payload['preferences'][0]->replacement)->toBe('DiscoveryCompilerTest\\MultiA\\ReplacementA') + ->and($payload['preferences'][1]->replacement)->toBe('DiscoveryCompilerTest\\MultiB\\ReplacementB'); + + compilerTestCleanup($moduleA['dir']); + compilerTestCleanup($moduleB['dir']); + }, + ); +}); diff --git a/packages/core/tests/Unit/Discovery/DiscoveryEnvironmentTest.php b/packages/core/tests/Unit/Discovery/DiscoveryEnvironmentTest.php new file mode 100644 index 00000000..ca4a9175 --- /dev/null +++ b/packages/core/tests/Unit/Discovery/DiscoveryEnvironmentTest.php @@ -0,0 +1,164 @@ +originalEnabled = array_key_exists( + 'DISCOVERY_CACHE_ENABLED', + $_ENV, + ) ? $_ENV['DISCOVERY_CACHE_ENABLED'] : null; + $this->originalAppEnv = array_key_exists('APP_ENV', $_ENV) ? $_ENV['APP_ENV'] : null; + $this->originalCachePath = array_key_exists( + 'DISCOVERY_CACHE_PATH', + $_ENV, + ) ? $_ENV['DISCOVERY_CACHE_PATH'] : null; + }); + + afterEach(function (): void { + if ($this->originalEnabled === null) { + unset($_ENV['DISCOVERY_CACHE_ENABLED']); + } else { + $_ENV['DISCOVERY_CACHE_ENABLED'] = $this->originalEnabled; + } + + if ($this->originalAppEnv === null) { + unset($_ENV['APP_ENV']); + } else { + $_ENV['APP_ENV'] = $this->originalAppEnv; + } + + if ($this->originalCachePath === null) { + unset($_ENV['DISCOVERY_CACHE_PATH']); + } else { + $_ENV['DISCOVERY_CACHE_PATH'] = $this->originalCachePath; + } + }); + + it( + 'returns enabled() true by default (no env set) and treats DISCOVERY_CACHE_ENABLED values 0, false, no, off, and empty (case-insensitive) as disabled', + function (): void { + $env = new DiscoveryEnvironment(); + + // Default: no env var set → true + unset($_ENV['DISCOVERY_CACHE_ENABLED']); + expect($env->enabled())->toBeTrue(); + + // Falsy string values that should all map to false + foreach (['0', 'false', 'FALSE', 'False', 'no', 'NO', 'No', 'off', 'OFF', 'Off', ''] as $falsy) { + $_ENV['DISCOVERY_CACHE_ENABLED'] = $falsy; + expect($env->enabled())->toBeFalse(); + } + }, + ); + + it( + 'returns enabled() true for any other present DISCOVERY_CACHE_ENABLED value (e.g. "1", "true", "yes")', + function (): void { + $env = new DiscoveryEnvironment(); + + foreach (['1', 'true', 'TRUE', 'True', 'yes', 'YES', 'Yes', 'on', 'ON', 'enabled'] as $truthy) { + $_ENV['DISCOVERY_CACHE_ENABLED'] = $truthy; + expect($env->enabled())->toBeTrue(); + } + }, + ); + + it('returns environment() from APP_ENV and defaults to production when APP_ENV is unset', function (): void { + $env = new DiscoveryEnvironment(); + + unset($_ENV['APP_ENV']); + expect($env->environment())->toBe('production'); + + $_ENV['APP_ENV'] = 'staging'; + expect($env->environment())->toBe('staging'); + + $_ENV['APP_ENV'] = 'local'; + expect($env->environment())->toBe('local'); + }); + + it('returns cachePath() from DISCOVERY_CACHE_PATH and defaults to storage/cache/discovery.php', function (): void { + $env = new DiscoveryEnvironment(); + + unset($_ENV['DISCOVERY_CACHE_PATH']); + expect($env->cachePath())->toBe('storage/cache/discovery.php'); + + $_ENV['DISCOVERY_CACHE_PATH'] = '/var/cache/marko/discovery.php'; + expect($env->cachePath())->toBe('/var/cache/marko/discovery.php'); + }); + + it('reads $_ENV live at call time (a value set after construction is reflected)', function (): void { + unset($_ENV['DISCOVERY_CACHE_ENABLED']); + unset($_ENV['APP_ENV']); + unset($_ENV['DISCOVERY_CACHE_PATH']); + + $env = new DiscoveryEnvironment(); + + // Defaults before setting env vars + expect($env->enabled())->toBeTrue() + ->and($env->environment())->toBe('production') + ->and($env->cachePath())->toBe('storage/cache/discovery.php'); + + // Set env vars AFTER construction — must be reflected + $_ENV['DISCOVERY_CACHE_ENABLED'] = '0'; + $_ENV['APP_ENV'] = 'testing'; + $_ENV['DISCOVERY_CACHE_PATH'] = '/tmp/discovery.php'; + + expect($env->enabled())->toBeFalse() + ->and($env->environment())->toBe('testing') + ->and($env->cachePath())->toBe('/tmp/discovery.php'); + }); + + it( + 'reads no value from marko/config and has no Marko\Config import (boot-time reader is config-package-free)', + function (): void { + $source = (string) file_get_contents( + dirname(__DIR__, 3) . '/src/Discovery/DiscoveryEnvironment.php', + ); + + expect(str_contains($source, 'Marko\\Config'))->toBeFalse() + ->and(str_contains($source, 'ConfigRepository'))->toBeFalse() + ->and(str_contains($source, 'ConfigNotFoundException'))->toBeFalse(); + }, + ); + + it( + 'the shipped config/discovery.php returns an array with enabled, environment, and cache_path keys matching the DiscoveryEnvironment defaults when no env vars are set', + function (): void { + unset($_ENV['DISCOVERY_CACHE_ENABLED']); + unset($_ENV['APP_ENV']); + unset($_ENV['DISCOVERY_CACHE_PATH']); + + $config = require dirname(__DIR__, 3) . '/config/discovery.php'; + + expect($config)->toHaveKey('enabled') + ->and($config)->toHaveKey('environment') + ->and($config)->toHaveKey('cache_path') + ->and($config['enabled'])->toBeTrue() + ->and($config['environment'])->toBe('production') + ->and($config['cache_path'])->toBe('storage/cache/discovery.php'); + }, + ); + + it( + 'snapshots and restores the three $_ENV keys around each test so no env state leaks (verify $_ENV unchanged after the suite for keys that were originally absent)', + function (): void { + // This test verifies that our beforeEach/afterEach isolation works correctly. + // If the original state had these keys absent, they should not be present here + // (since afterEach restores the snapshot from beforeEach). + // We set them and they will be cleaned up by afterEach. + $_ENV['DISCOVERY_CACHE_ENABLED'] = 'no'; + $_ENV['APP_ENV'] = 'test-isolation'; + $_ENV['DISCOVERY_CACHE_PATH'] = '/tmp/test-isolation.php'; + + $env = new DiscoveryEnvironment(); + + expect($env->enabled())->toBeFalse() + ->and($env->environment())->toBe('test-isolation') + ->and($env->cachePath())->toBe('/tmp/test-isolation.php'); + // afterEach will restore the original state + }, + ); +}); diff --git a/packages/docs-markdown/docs/packages/cli.md b/packages/docs-markdown/docs/packages/cli.md index c0bc9b6f..809bf36d 100644 --- a/packages/docs-markdown/docs/packages/cli.md +++ b/packages/docs-markdown/docs/packages/cli.md @@ -20,10 +20,12 @@ Ensure Composer's global bin directory is in your PATH. From any directory within a Marko project: ```bash -marko list # Show all available commands -marko module:list # List installed modules -marko cache:clear # Clear cache (if cache module installed) -marko db:migrate # Run migrations (if database module installed) +marko list # Show all available commands +marko module:list # List installed modules +marko discovery:cache # Compile the discovery cache (run on deploy) +marko discovery:clear # Remove the discovery cache +marko cache:clear # Clear cache (if cache module installed) +marko db:migrate # Run migrations (if database module installed) ``` ### Creating Commands diff --git a/packages/docs-markdown/docs/packages/core.md b/packages/docs-markdown/docs/packages/core.md index a9d3e912..c28268c5 100644 --- a/packages/docs-markdown/docs/packages/core.md +++ b/packages/docs-markdown/docs/packages/core.md @@ -166,6 +166,82 @@ return [ ]; ``` +### Discovery Cache + +On every boot, Marko scans all module PHP files to discover `#[Preference]`, `#[Plugin]`, `#[Observer]`, and `#[Command]` attributes. In production this scan can be eliminated by compiling its results into a single PHP file --- the discovery cache. + +#### Compiling the cache + +```bash +marko discovery:cache +``` + +Scans all modules, writes the compiled cache, and reports per-section counts: + +``` +Discovery cache compiled successfully. +Cache path: /var/www/html/storage/cache/discovery.php +preferences: 4 +plugins: 12 +observers: 7 +commands: 9 +``` + +#### Clearing the cache + +```bash +marko discovery:clear +``` + +Deletes the compiled cache file. Idempotent --- safe to run when no cache exists. + +#### How boot uses the cache + +At boot, `Application::initialize()` reads three environment variables directly (not via `marko/config`, so the gate works before any config package is loaded): + +| Variable | Default | Description | +|---|---|---| +| `APP_ENV` | `production` | Application environment. Set to `development` to disable the cache. | +| `DISCOVERY_CACHE_ENABLED` | `true` | Set to `0`, `false`, `no`, `off`, or empty to disable. | +| `DISCOVERY_CACHE_PATH` | `storage/cache/discovery.php` | Path to the cache file. Relative paths resolve from the project root; absolute paths are used as-is. | + +The cache is used when **all three conditions** are true: + +1. `DISCOVERY_CACHE_ENABLED` is truthy +2. `APP_ENV` is not `development` +3. The cache file exists at `DISCOVERY_CACHE_PATH` + +If the cache file is **missing**, boot falls back to a normal full rescan --- no error. + +If the cache file is **corrupt, malformed, or version-mismatched**, boot throws `DiscoveryCacheException` immediately. There is no silent fallback. Run `marko discovery:clear` then `marko discovery:cache` to rebuild. + +In **`development`** environment the cache is always bypassed, so adding or editing a `#[Plugin]`, `#[Observer]`, `#[Preference]`, or `#[Command]` takes effect on the next request without any manual step. + +#### Configuration via `marko/config` + +When `marko/config` is installed, the same keys are available as a config file: + +```php title="config/discovery.php" +return [ + 'enabled' => true, // mirrors DISCOVERY_CACHE_ENABLED + 'environment' => 'production', // mirrors APP_ENV + 'cache_path' => 'storage/cache/discovery.php', // mirrors DISCOVERY_CACHE_PATH +]; +``` + +The core-owned `config/discovery.php` is shipped with `marko/core` and populates these values from `$_ENV` automatically. The boot gate reads `DiscoveryEnvironment` directly and does not depend on `marko/config`. + +#### Deploy requirement + +There is no file-modification-time invalidation. Whenever code changes (new modules, updated attributes), regenerate the cache as part of your deploy: + +```bash +composer install --no-dev --optimize-autoloader +marko discovery:cache +``` + +Serving stale discovery results in missing preferences, plugins, observers, or commands until the cache is recompiled. + ### Throwing Rich Exceptions Include context and fix suggestions: @@ -241,3 +317,22 @@ use Marko\Core\Exceptions\CircularDependencyException; Thrown by the container when a mutual constructor dependency cycle is detected (e.g. class A requires class B which requires class A). Rather than exhausting the call stack, the container detects the cycle and throws immediately with a human-readable chain (`A -> B -> A`) and a suggestion to remove the circular reference. Implements `Psr\Container\ContainerExceptionInterface`. + +### DiscoveryCacheException + +```php +use Marko\Core\Exceptions\DiscoveryCacheException; +``` + +Thrown by `Application::initialize()` when the discovery cache file is corrupt, structurally invalid, or carries a version number that does not match the running core version. There is no silent fallback --- a bad cache is a loud error. + +Named constructors: + +| Method | When thrown | +|---|---| +| `DiscoveryCacheException::unreadable($path)` | Cache file exists but cannot be read | +| `DiscoveryCacheException::malformed($path, $reason)` | Cache file structure is invalid or missing required keys | +| `DiscoveryCacheException::versionMismatch($path, $found, $expected)` | Cache was compiled by a different core version | +| `DiscoveryCacheException::notWritable($path)` | Cache directory or file is not writable (thrown by `discovery:cache`) | + +Fix in all cases: `marko discovery:clear && marko discovery:cache`.