Skip to content

refactor: align Type and EngineExtension exceptions with the named-constructor convention - #201

Merged
lisachenko merged 4 commits into
8.4from
claude/php-repo-modernization-6pfu7y-exception-factories
Aug 13, 2026
Merged

refactor: align Type and EngineExtension exceptions with the named-constructor convention#201
lisachenko merged 4 commits into
8.4from
claude/php-repo-modernization-6pfu7y-exception-factories

Conversation

@lisachenko

Copy link
Copy Markdown
Owner

AGENTS.md is explicit: "Domain exceptions are never thrown with a hand-written message at the call site. Each failure mode is a public static factory on the exception class." Two layers had never been brought in line — src/Type had no domain exception class at all, and EngineExtension had one (ExtensionNotRegisteredException) that covered a single failure mode while five others were hand-written inline, two of them character-for-character identical.

New exception classes and their factories

ZEngine\Type\TypeOperationException extends \RuntimeException

The Type layer's first domain exception class. Nine failure modes moved onto it; six were near-duplicates of each other spread over three files (Can not add/store/delete an item with key/index …).

Factory Replaced site
cannotAddKey($key) Type/HashTable.php add()
cannotAddIndex($key) Type/HashTable.php addIndex()
cannotDeleteKey($key) Type/HashTable.php delete()
cannotDeleteIndex($key) Type/HashTable.php deleteIndex()
functionNotPublished($key) Type/HashTable.php addFunctionEntry()
cannotStoreKey($key) Type/PersistentHashTable.php addInterned()
cannotStoreIndex($key) Type/PersistentHashTable.php addIndex()
danglingObjectEntry() Type/ObjectEntry.php assertObjectAlive()
referenceCountUnderflow() Type/ReferenceCountedTrait.php decrementReferenceCount()

On the class name and scope. I read every message before naming it. EngineTableException would have been accurate for seven of the nine but wrong for the dangling-weak-entry and refcount-underflow guards, which are not table failures. Rather than mint a second one-method class, the name generalises to what all nine actually are: an operation on a Type-layer wrapper that the engine state refused. HashTable::deleteIndex() (the "Can not delete an item with index" twin of delete()) and ReferenceCountedTrait's underflow guard were not in the original brief; they surfaced in the sweep and belong to the same family, so leaving them inline would have re-fragmented the wording the class exists to unify.

StringEntry::setCachedClassEntry()'s "This string does not carry an engine class-entry cache slot" stays a bare \LogicException: it is the only LogicException among the Type-layer sites, so folding it in would have forced either a base-type change (forbidden, see below) or a second class for one call site — and it is a plain programmer guard whose predicate (hasClassEntryCache()) is public, so the caller is told to check first.

ZEngine\EngineExtension\ModuleRegistrationException extends \RuntimeException

Shaped like its neighbour ExtensionNotRegisteredException. AbstractModule previously had no module exception class whatsoever.

Factory Replaced site
alreadyRegistered($moduleName) AbstractModule::register()
registrationRefused($moduleName) AbstractModule::register(), after zend_register_module_ex
startupFailed($moduleName) AbstractModule::startup()

ZEngine\Memory\HeapAnchorMissingException extends PersistentHeapException

ZEngineModule::onHeapDestroyed() and ZEngineModule::recoverHeapRegistry() hand-wrote "The zengine module has no globals block" twice, both as a PersistentHeapException.

Deviation from the brief, deliberate. The brief suggested folding these two into the new EngineExtension module class extending \RuntimeException. That would have changed the thrown type from PersistentHeapException to something outside the heap hierarchy and silently broken every catch (PersistentHeapException) around a heap lookup — including ZEngineModule::getDisplayInfo() in the very same file, which converts heap failures into an "inert" row rather than throwing across the info_func FFI boundary (issue #50). The base-type preservation guarantee wins over class placement, so the message moved onto a new subclass of what was already being thrown, following the house style of HeapInertException / MissingClassException (final subclass + one static factory) and living beside its siblings in ZEngine\Memory.

ZEngine\System\Hook\OpCodeHookException extends \RuntimeException

Factory Replaced site
handlerInstallFailed() OpCodeHook::install()
handlerRestoreFailed() OpCodeHook::uninstall()

Base-type preservation guarantee

Every new exception class extends exactly the type the throw it replaces was throwing, and every replaced message is carried over verbatim. No catch anywhere can stop matching as a result of this PR:

New class Extends Because the replaced throws were
TypeOperationException \RuntimeException all nine were \RuntimeException
ModuleRegistrationException \RuntimeException all three were \RuntimeException
HeapAnchorMissingException PersistentHeapException both were PersistentHeapException
OpCodeHookException \RuntimeException both were \RuntimeException

Catchers verified by grep before choosing each parent:

  • Core::shutdown()'s find() pre-check exists precisely because HashTable::delete() throws on an absent key — still a \RuntimeException, still caught.
  • tests/Type/PersistentHashTableTest.php expects \RuntimeException and asserts /index 2/ on the message — the factory emits the identical string.
  • tests/Type/StringEntryOwnershipTest.php expects \RuntimeException from the refcount underflow, \LogicException from the immutable-increment guard (untouched).
  • tests/EngineExtension/fixture/module-lifecycle-order.php catches bare RuntimeException around a rejected registration.
  • ZEngineModule::getDisplayInfo() catches PersistentHeapException.
  • No test or doc asserts any of the other replaced message strings, and docs/ mentions none of these types.

Latent deprecation fixed: AbstractModule::detectModuleName()

strtolower(preg_replace_callback(...)) fed strtolower() a ?string. preg_replace_callback() returns null on a PCRE failure (backtrack/recursion limit), and passing null to strtolower() has been deprecated since PHP 8.1 — a real runtime deprecation on that path, not just a static-analysis artifact. The error path now falls back to the unsplit class name ($snakeCased ?? $className), which is what the camelCase→snake_case conversion degrades to anyway. The freed phpstan-baseline.neon entry is pruned by hand (Parameter #1 $string of function strtolower expects string, string|null given); the second AbstractModule baseline entry is unrelated and stays.

Sweep: sites kept as-is, with reasoning

src/ was swept for throw new \?(RuntimeException|LogicException|InvalidArgumentException|ReflectionException)( with inline interpolated messages. Excluded from consideration: HotSwap.php, Core.php, ReflectionMethod.php, PersistentHeap.php, the ClassExtension/Hook/* proceed()/getOriginalCallable() throws, AbstractMethodResolutionHook.php, and AbstractSyntaxTree/Node.php's OutOfBoundsException guards — all owned by concurrent PRs. Remaining hits, grouped:

  • OpCodeHook.php, Cannot install an engine hook after Core::shutdown() — kept native \LogicException. It is a genuine shared-vocabulary duplicate, but the same string is raised verbatim by Hook/AbstractHook.php and ClassExtension/Hook/IteratorBridge.php too. Converting only OpCodeHook's copy would leave the message split between a factory and two inline twins — strictly worse than the status quo. It wants one factory for all three sites, and AbstractHook is owned by the in-flight hook-consolidation PR, so this belongs there.
  • OpCodeHook.php, out-of-order uninstall guard — kept. Programmer-misuse guard on a public API, single site, stable message; OpCodeHookTest and HookLifecycleTest both assert only the \LogicException type.
  • OpCodeHook.php, handler-signature check — kept \InvalidArgumentException. Pure argument validation, and OpCodeHookTest asserts the message verbatim; a factory adds no clarity and only adds a way to drift.
  • EngineExtension/ExtensionManager.php:62 (Module … is already registered; use get()) — kept \LogicException. Different layer and different base type from ModuleRegistrationException::alreadyRegistered(): this is the framework-side registry rejecting a double register() call (programmer error), not the engine module registry refusing an entry.
  • EngineExtension/ModuleDependency.php:57,61,64 — kept. Constructor argument validation (\InvalidArgumentException); AbstractModuleTest asserts the type.
  • Type/StringEntry.php:308, Type/ReleasableTrait.php:98, Type/ReferenceCountedTrait.php:54,71, Type/StructArray.php, Type/ResourceEntry.php, Type/OpLine.php, Type/ObjectEntry.php:254, Type/ClosureEntry.php:120 — kept. All either \LogicException/\OutOfBoundsException programmer guards with stable single-site messages, or native-reflection-parity \ReflectionExceptions.
  • Reflection/* \ReflectionExceptions (Class X should be in the engine., trait/alias/precedence errors) — kept. These are deliberate native-Reflection* parity errors; consumers catch \ReflectionException by contract.
  • AbstractSyntaxTree/{DeclarationNode,ListNode,NodeFactory,NodeKind}.php — kept. Node-kind argument validation and "not yet supported" guards, adjacent to the AST files a concurrent dedup PR is restructuring.

Three clusters are real remaining duplication but sit outside this PR's stated scope, and each needs its own exception class in a layer this PR does not otherwise touch — flagged as follow-ups rather than smuggled in here: Reflection/FunctionLikeTrait.php's eight "… are available only for user-defined functions" variants (three of them byte-identical), System/ObjectStore.php's twice-duplicated "Object store is read-only structure" plus its thrice-duplicated out-of-bounds message, and the "Unknown code … New version of PHP?" trio spanning NodeKind, OpCode and ReflectionValue.

Adjacency with in-flight PRs

  • perf(system): resolve the executing frame from execute_data instead of debug_backtrace in OpCodeHook #199 (OpCodeHook::handle() frame resolution) rewrites handle() and the two Closure::fromCallable([$this, 'handle']) call sites in this same file. I inspected its branch: it does not touch either throw, and only shifts them by one line via an edited const docblock. This PR stays rooted at origin/8.4 (70d8d4f); expect a trivial, non-conflicting merge.
  • fix: exception-safe engine-state restoration (fake scope, pDestructor, class-table guards) #197 (exception-safety / deleteWithoutDestructor) touches HotSwap.php, Core.php, ReflectionMethod.php — untouched here. HashTable::deleteWithoutDestructor() sits between two converted throws in HashTable.php but its own body is unchanged.
  • The memory-ownership PR owns PersistentHeap.php:614,663 — excluded, not touched.
  • The hook-consolidation PR owns AbstractHook::getOriginalCallable() and the ClassExtension/Hook/* proceed() prologues — untouched, and the shutdown-guard message above is left to it on purpose.

Validation

vendor/bin/phpstan analyse (level max) and PHP_CS_FIXER_IGNORE_ENV=1 vendor/bin/php-cs-fixer fix --dry-run are both clean, and were clean on origin/8.4 beforehand. All four new files pass php -l.

Local tests were not run, by AGENTS.md's non-negotiable version rule: this branch targets PHP 8.4 and the container interpreter is 8.5.9. Running the suite means Core::init() reading zend_class_entry at 8.4 offsets under an 8.5 engine — silent memory corruption, not a clean failure. Validation here is static-only; CI on the 8.4 runners is the gate. The change is a pure message-relocation refactor with types and strings preserved, which is exactly the shape that survives that constraint.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG


Generated by Claude Code

claude added 4 commits August 13, 2026 19:29
The Type layer had no domain exception class: nine failure modes threw a bare
\RuntimeException with a hand-written message at the call site, and six of them
were near-duplicates of each other ("Can not add/store/delete an item with
key/index ..."), so the same wording lived in three files at once.

TypeOperationException now owns every one of those messages behind a named
static constructor, per the convention in AGENTS.md. It extends \RuntimeException
- exactly what each replaced throw threw - so every existing catch keeps
matching: Core::shutdown()'s find() pre-check around HashTable::delete(), the
\RuntimeException expectations in PersistentHashTableTest and
StringEntryOwnershipTest, and any consumer catching the base type.

Messages are carried over verbatim; only their home changed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
…nstructors

AbstractModule had no module exception class at all: register() and startup()
threw three bare \RuntimeExceptions with inline messages, while ZEngineModule
hand-wrote the very same 'The zengine module has no globals block' text twice.

Two classes now own those messages behind named constructors (AGENTS.md):

- ModuleRegistrationException (EngineExtension, shaped like the neighbouring
  ExtensionNotRegisteredException) extends \RuntimeException, exactly what the
  three replaced throws threw - the module-lifecycle-order fixture's
  catch (RuntimeException) around a rejected registration keeps matching.
- HeapAnchorMissingException extends PersistentHeapException, which is what
  ZEngineModule already threw there. The anchor slot IS heap state, so the class
  joins the existing heap hierarchy in ZEngine\Memory next to its siblings rather
  than moving the failure to an unrelated base type and breaking every
  catch (PersistentHeapException) around a heap lookup.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
…e_callback

preg_replace_callback() returns null when PCRE fails (backtrack or recursion
limit), and that null went straight into strtolower() - a deprecation since
PHP 8.1 that the baseline had been carrying as "expects string, string|null
given". On the error path the class name is now used unsplit instead, which is
what the camelCase-to-snake_case conversion degrades to anyway.

The freed baseline entry is pruned.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
…onstructors

install() and uninstall() reported the two zend_set_user_opcode_handler()
failures with inline messages that share their whole vocabulary ("Can not
install user opcode handler" / "Can not restore original opcode handler").
Both are engine-domain failures of the same operation, so OpCodeHookException
now owns them behind named constructors, extending \RuntimeException exactly
like the throws it replaces.

The three remaining SPL throws in this file stay native on purpose:

- the Core::isShutdown() guard repeats a message that AbstractHook and
  IteratorBridge raise verbatim; unifying it means one factory for all three
  sites, which belongs with the hook-consolidation work that owns those files,
  not to a partial conversion here.
- the out-of-order uninstall guard and the handler-signature check are
  programmer-misuse guards on a public API with stable messages (the latter is
  asserted verbatim by OpCodeHookTest); a factory adds no clarity there.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
@lisachenko
lisachenko marked this pull request as ready for review August 13, 2026 20:20
@lisachenko
lisachenko merged commit aa1141b into 8.4 Aug 13, 2026
19 checks passed
@lisachenko
lisachenko deleted the claude/php-repo-modernization-6pfu7y-exception-factories branch August 13, 2026 20:20
lisachenko pushed a commit that referenced this pull request Aug 13, 2026
Re-bases the CI signal on the true merged state: 8.4 gained the
exception-factory classes (#201) that also touch OpCodeHook's
install-time guards.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
lisachenko pushed a commit that referenced this pull request Aug 13, 2026
Makes the CI signal concrete on the exact combination the merge preview
tests: this branch's dedup plus the merged wave (frame-scope resolution,
sizeOfType migration, exception factories).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants