chore/review-fixes-1#4
Merged
Merged
Conversation
…face subscriber failures
Closes review items §7, §8.2, §3, §6, §13.5, §8.5 (see runtime/review/report.md):
- @api markers added to event and connection port interfaces; EventAction
scoped @psalm-internal (used only inside internal\action\Workflow).
- Command and Factory moved to src/internal/ (namespaces
dbschemix\core\internal\{command,template}) to align with the existing
internal/ convention; @psalm-internal scope tightened from `dbschemix`
to `dbschemix\core` on Command.
- Public package helpers moved from src/internal/functions.php
(namespace dbschemix\core\internal) to src/package/functions.php
(namespace dbschemix\core\package); renamed get_package_version ->
version and get_package_path -> path (prefix becomes redundant in
package\ namespace). composer.json "files" autoload entry updated.
- Context and InputOptions enforce non-empty-string and non-negative-int
invariants at runtime via InvalidArgumentException, replacing assert()
which is disabled in production with zend.assertions=-1.
- EventDispatcher surfaces subscriber failures through
trigger_error(E_USER_WARNING) instead of silent swallow; subscriber
policy documented on EventDispatcher::trigger() and
EventSubscriberInterface::subscriptions(). Isolation preserved so a
broken subscriber cannot corrupt the migration's success/error event
contract.
- EventPublisherInterface removed (dead contract: declared on()/off()
but never implemented by EventDispatcher and never consumed).
All checks green: Psalm (100% inferred, no errors), PHPStan (no errors),
PHPUnit (86 tests / 234 assertions), PHPCS (57 files).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes review item §design-patterns "subscriptions() keyed by raw string" (see runtime/review/report.md). EventSubscriberInterface::subscriptions() returned array<string, callable>, where the string was Event::value. This forced consumers to hardcode strings or use `Event::X->value` at every subscription site, removed static verification of enum coverage, and conflated the dispatcher's internal string-keyed indexing with the public subscription contract. Enum-as-array-key is not supported by PHP (TypeError: Cannot access offset of type E), so the natural alternative — array<Event, callable> — is not viable. Replaced with a typed list of Subscription DTOs instead: - New @api class src/event/Subscription.php — final readonly, holds an Event and a Closure(Event, EventInterface): void. - EventSubscriberInterface::subscriptions() now returns list<Subscription>. - EventDispatcher::__construct() converts Event -> string at the boundary (subscription->event->value) so internal storage stays string-keyed, but no string crosses the public API. - tests/stub/TestSubscriber updated; @PHPStan-Ignore varTag.nativeType hack no longer needed. Smoke-test confirms multiple Subscription entries on one subscriber dispatch correctly to matching events and ignore non-subscribed events. All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86 tests), PHPCS. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…d sealed-root roles Closes: - §7 «Mark API and internal» applied to the exception hierarchy (six classes) - §12 «PrepareException dead code» — N/A (used downstream) - §5 «InitializationException asymmetric constructor» — WONT-FIX (intentional semantic shape, not asymmetry) See runtime/review/report.md for the full rationale on each. Context: the reviewer (which only sees core) flagged PrepareException as dead because nothing in core throws it. In fact it is a public extension point for driver implementations in downstream packages (dbschemix/pdo, dbschemix/pgsql, dbschemix/clickhouse) — they throw it from their ConnectionInterface / StatementInterface implementations when statement preparation fails, and Workflow::run() catches it through the generic Throwable handler and wraps it into ActionException. Changes: - All six exception classes (MigratorException, ActionException, ConnectionException, InitializationException, ConfigurationException, PrepareException) now carry @api markers, making the stability tier of the exception contract explicit — both for consumers that catch them and for driver implementors that throw them. - MigratorException gains a class-level docblock describing the sealed hierarchy: catch the base for generic handling or a concrete subtype for targeted handling. - PrepareException gains a class-level docblock describing its extension-point role for downstream driver packages and the workflow that wraps it into ActionException. - The constructor signature asymmetry the reviewer flagged is intentional: each constructor takes the domain data of its case plus the originating cause (filename for ActionException, driver for ConnectionException, description for InitializationException with no domain object). The two classes that inherit the parent signature (ConfigurationException, PrepareException) do so by design — to keep text flexible for user input validation and to give downstream driver implementors freedom in how they construct PrepareException. All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86 tests), PHPCS. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes review item §4 (Methods and functions / Typing): src/internal/action/Workflow.php. Workflow::run() and Workflow::iteratorHandler() accepted `callable` as their native type with a PHPDoc-typed signature. The reviewer noted that the richest static typing for first-class callables — `Closure` with a typed signature — was not used, so Psalm could only verify the callable's shape through PHPDoc and not at runtime. All existing call sites already produce Closure instances: - `$fsHandler = static fn(filesystem\Action $fs): Iterator => $fs->up(...)` (arrow functions are Closures) - `$command->up(...)`, `$command->down(...)`, `$command->exec(...)` (first-class callable syntax produces Closures) Replaced `callable` with `Closure` in both the native type and the PHPDoc shape on Workflow::run and Workflow::iteratorHandler; added `use Closure;`. No call-site changes needed. Effect: PHP now rejects non-Closure callables (string function names, [$obj, 'method'] arrays, invokables) at the method boundary before they enter the body. Psalm and PHPStan now have a real native class type plus the typed Closure signature to reason about. All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…xplicit exception policy Closes review item §5 (@throws Throwable on CommandInterface::fetchApplied) as WONT-FIX with documentation; see runtime/review/report.md. The reviewer flagged `@throws Throwable` on the four CommandInterface methods as collapsing the "three tiers of failure" discipline. The framing confuses the contract's audience: CommandInterface is a driver-extension API, not a consumer API. - Consumers interact with Migrator, whose exception contract is fully typed (ActionException, ConfigurationException, ConnectionException, InitializationException). They never see Throwable. - Driver implementors (internal\command\Command, plus downstream packages dbschemix/pdo, dbschemix/pgsql, dbschemix/clickhouse) implement CommandInterface and may surface whatever their underlying DB client throws (\PDOException, mysqli_sql_exception, ClickHouse client exceptions, ...) plus library-typed exceptions such as PrepareException. - Workflow catches Throwable at its boundary (Workflow::run, Workflow::getAppliedMigrations) and wraps each into the appropriate typed MigratorException subtype before it propagates out. The three tiers of failure are restored at the boundary. Forcing driver implementors to pre-wrap into a typed exception would add boilerplate without typing benefit (Workflow's broad catch is unchanged either way) and would break existing downstream implementations. Change: CommandInterface now carries a class-level docblock that records audience (driver implementors), exception policy (Throwable on the port, typed exceptions at the workflow boundary), and the explicit invariant that driver implementors are not required to pre-wrap. All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tring Closes review item §3 (Typing — PHPDoc completes the contract): src/template/FactoryInterface.php:14. FactoryInterface::makeBody() previously had no @return annotation and a native string return type. The body is written verbatim to disk as a migration file (via Workflow::create -> filesystem\Action::create), so an empty string silently produces a no-op migration template. Added `@return non-empty-string` to make the contract explicit. The in-tree internal\template\Factory::makeBody implementation returns a heredoc with fixed -- @up / -- @down headers and is statically verified as non-empty. Downstream implementations that return empty now surface on static analysis instead of producing a useless file. All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tInterface role
Closes review item §design-patterns "Anaemic EventInterface forces instanceof"
(see runtime/review/report.md).
The reviewer's framing was partially valid (subscribers that need structured
data must instanceof onto the concrete event class) and partially wrong (the
concrete classes already expose payload as public readonly properties — it's
type narrowing, not "reach-back into internals"). All structural
alternatives (fat EventInterface, generic Subscription<T>, per-event
callback signatures) trade away more than they gain.
The actual hidden problem: the Event enum case -> concrete event class
mapping was nowhere documented. A subscriber had to read Workflow.php and
Migrator.php to know what to instanceof onto, turning an implicit @api
contract into a guessing game.
This commit makes that mapping explicit:
- src/event/Event.php: each of the seven cases now carries a docblock with
a one-line description of when it fires and an explicit
"Dispatched payload: {@see ConcreteEvent} (properties)" line. The
class-level docblock declares that the mapping is part of the @api
contract.
- src/event/EventInterface.php: class-level docblock explains the role
split — thin surface (getName/getMessage) for generic logging,
structured data via instanceof onto MigrateSuccessEvent /
MigrateErrorEvent / ExceptionEvent, with the mapping in {@see Event}.
The mapping itself:
MigrateSuccess -> MigrateSuccessEvent (action, context)
MigrateError -> MigrateErrorEvent (action, context, exception)
InitializationError -> ExceptionEvent (dbName, exception)
ConnectionError -> ExceptionEvent (dbName, exception)
ConfigurationError -> ExceptionEvent (dbName, exception)
FilesystemError -> ExceptionEvent (dbName, exception)
FilesystemNotice -> ExceptionEvent (dbName, exception)
No code changes; no type changes; no breaking changes. Subscribers may now
instanceof with documented certainty instead of guessing.
All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes review item coding-style §2 ("functions in namespace for a library")
on src/internal/filesystem/functions.php as WONT-FIX; see
runtime/review/report.md.
Rationale: precedent for keeping function-in-namespace form is already set
in src/package/functions.php (public package helpers) — see review item §3.
Wrapping three internal one-line helpers in a final class for lazy autoload
adds churn for negligible benefit. All three remain `@infection-ignore-all`
deterministic transformers; the cost of eager autoload through composer.json
"files" entry is negligible for a handful of pure functions.
Same commit fixes a misnamed helper:
- `joinBasename($path, $postfix)` → `joinSuffix($path, $suffix)`.
The original name was misleading: the function does not join a basename,
it appends a suffix to a directory path to produce a sibling directory
(e.g. /path/to/dir + '-fixture' → /path/to/dir-fixture/). The new name
matches the action and stays in the existing join* family alongside
joinFilename. Parameter renamed for consistency. Two call sites in
Action.php (fixture / repeatable directory construction) updated.
All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes review item coding-style §8 ("ActionType has SKIP case never matched
in prepareCommand").
Confirmed:
- prepareCommand() callers pass only ActionType::UP and ActionType::DOWN.
- ActionType carries @psalm-internal dbschemix\core\internal\filesystem
(the narrowest scope in the codebase), so downstream packages
(dbschemix/pdo, dbschemix/pgsql, dbschemix/clickhouse) have no
legitimate path to it.
- "skip" semantics in the project exist, but on the filename level
(Action::makeIterator regex /.+(?<!skip)\.sql/i rejects *skip.sql files),
not as a block-level marker inside a single migration file. Block-level
-- @Skip support is not implemented and not planned.
Removed the SKIP case as dead code. No callers to update.
All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…command\Options @api + invariants) Closes review nits §7 (Command.php inline @var), §8 (\r\n vs no separator), §9.7 (command\Options namespace), and reopens the §6 VO-invariant pattern for command\Options that the reviewer missed. 1. src/internal/command/Command.php:41 - str_replace-based SQL assembly with /** @var non-empty-string $query */ cast replaced by string concatenation. First segment is a non-empty literal and $this->config->table is `non-empty-string` (from Config's PHPDoc), so Psalm infers `non-empty-string` without the manual cast. - Smoke-tested against the four (WHERE present/absent) × (LIMIT present/absent) cases: SQL is functionally equivalent to the old form, and the new form drops the redundant double-spaces that used to land around elided placeholders. 2. src/event/MigrateErrorEvent.php - getMessage() line separator changed from `\r\n` (Windows-style) to `\n` (Unix-style, idiomatic for a PHP library). - MigrateSuccessEvent stays as-is — it has no multi-line content and no separator is needed. 3. src/command/Options.php - The reviewer's §9.7 framing was incorrect: command\Options is used in CommandInterface::fetchApplied's signature and is therefore part of the public driver-extension API. The reviewer offered "move to internal\ OR add explicit stability annotation"; the second path is correct. Added @api marker. - Bonus the reviewer missed: command\Options used assert() for its non-negative-int invariants on $limit and $version — the same pattern §6 already fixed for Context and InputOptions. assert() is disabled in production with zend.assertions=-1, so the invariants were silently dropped. Replaced with throwing \InvalidArgumentException guards. Wither methods (withVersion, makeFromInput) carry @psalm-suppress MissingThrowsDocblock ("Inputs come from already-validated sources"). Cascading @psalm-suppress MissingThrowsDocblock added to Workflow::up and Workflow::getLastVersion where `new Options()` / `new Options(limit: 1)` are constructed with literal non-negative arguments that cannot trigger the throw. §11 (sealed-set documentation on MigratorException) was closed in ece1751 — the class-level docblock already enumerates all five concrete subtypes as a closed set and documents the catch-base-or-subtype pattern. All checks green: Psalm (100% inferred), PHPStan, PHPUnit (86), PHPCS (58). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…philosophy Re-classifies review item §6 (VO assert vs throwing constructor) from DONE to WONT-FIX after discovering the original assert() was an intentional static-first design choice, not an oversight; see runtime/review/report.md. Background: The reviewer flagged assert() on VO invariants in Context, InputOptions, command\Options, and filesystem\Options as unsafe (assert disabled in production by zend.assertions=-1). The previous closure replaced assert with throwing \InvalidArgumentException guards plus full non-empty-string validation. This required @psalm-suppress TypeDoesNotContainType on the constructors (Psalm trusts non-empty-string PHPDoc as a static fact and sees the runtime '' check as dead) and @psalm-suppress MissingThrowsDocblock cascading through wither methods, Migrator, Workflow, and Action (all transitively construct one of these VOs from already-validated state). User pushback on @psalm-suppress MissingThrowsDocblock prompted converting that cascade to explicit @throws \InvalidArgumentException on Workflow, filesystem\Action, and MigratorInterface. That worked, but exposed @psalm-suppress TypeDoesNotContainType as still required for the constructors themselves. The only way to drop that suppress without losing runtime safety is to abandon constructor promotion for the narrow-typed fields (declare properties separately, accept loose string/int parameters, validate-and-assign in the body). That structural refactor would have been a significant departure from the original code. The original author chose assert() deliberately: - The project requires PHPStan + Psalm + Infection (minCoveredMsi: 99) + strict PHPCS. Target audience is consumers using static analysis. - In a static-first design, assert() is a development-time tripwire that catches misuse in CI and local development; production safety is provided by static analysis rejecting violations before they reach prod. - This is a legitimate design school for libraries, distinct from "always-valid VO at runtime". The reviewer applied the latter to code written in the former. Revert: - src/Context.php: removed throw-based guards + use InvalidArgumentException; restored single assert($this->version >= 0). - src/InputOptions.php: removed throws + use; restored assert($this->limit >= 0); assert($this->version >= 0);. - src/command/Options.php: removed throws + use; restored two asserts. Retained @api marker (separate §9.7 closure, not part of this revert). - src/internal/filesystem/Options.php: removed throw + use; restored assert($this->limit >= 0). - src/internal/action/Workflow.php: removed @throws InvalidArgumentException from up/down/fixture/initialization/repeatable/getLastVersion; removed @psalm-suppress MissingThrowsDocblock from up/getLastVersion; removed use InvalidArgumentException. - src/internal/filesystem/Action.php: removed @throws InvalidArgumentException from up/down/fixture; removed use InvalidArgumentException. - src/MigratorInterface.php: removed @throws InvalidArgumentException from all 7 methods; removed use InvalidArgumentException. - src/Migrator.php: removed @psalm-suppress MissingThrowsDocblock from all 6 methods; removed use InvalidArgumentException. Verification: - grep -rn "psalm-suppress" /app/src returns nothing. - Psalm: 100% inferred, no errors. - PHPStan: no errors. - PHPUnit: 86 tests pass. - PHPCS: 58 files pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ers, drop one message-text assertion
Closes 4 of the 5 "test coverage" findings from the review (option A —
quick wins, see runtime/review/report.md):
1. tests/workflow/ConfigurationTest.php — dropped
$this->expectExceptionMessage('Connection error') from testConnectionException.
ConnectionException::class is enough specificity for that test
(only one realistic path through TestDriverError → init). The
broader pattern (~17 other expectExceptionMessage* sites in the
suite) is documented as an open tail — fixing them properly would
require typed sub-exceptions, not test-only refactor.
2. tests/package/FunctionsTest.php (new) — 5 tests covering
src/package/functions.php (version, path):
- happy path against phpunit/phpunit (guaranteed installed)
- OutOfBoundsException branch for unknown package
- static cache invariant on version()
3. tests/InputOptionsTest.php (new) — 14 tests covering wither
semantics and derived predicates:
- withResetLimit: only limit dropped, other 7 fields preserved
- withVersion: minimal carry (version+dryRun+dbName+migrationName);
drops limit, exactlyAll, hasRepeatable, applyLatestVersion
- withExactlyAll: sets exactlyAll, preserves all except
applyLatestVersion
- immutability check on all three withers
- 5 data-driven cases for hasApplyLatestVersion (covers
applyLatestVersion × version × limit matrix)
- 4 data-driven cases for hasRepeatable (hasRepeatable × dryRun)
4. tests/internal/PathFunctionsTest.php (new) — 18 tests covering
src/internal/filesystem/functions.php:
- normalizePath: 8 cases (absolute/relative, single/multiple
trailing slashes, leading/trailing whitespace, root)
- joinSuffix: 5 cases (path/suffix slash variants)
- joinFilename: 5 cases (path slash variants + timestamp-prefixed
filename)
All tests follow project conventions: testFoo method naming, iterable
yield-style data providers, assertSame for typed equality, hand-written
test bodies (no mocking frameworks).
Coverage delta: 86 tests / 234 assertions → 123 tests / 304 assertions
(+37 tests, +70 assertions). All 4 checks green: Psalm (100% inferred),
PHPStan, PHPUnit (123), PHPCS (58).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.