[RFC]: Named exception constructors and complete per-component exception namespaces #189

Proposed Version

0.6.0

Basic Information

Stop building exception messages at the throw site. Every throw goes through a static named constructor on a component-owned exception class, with the message template beside it as a final public const string.

src/ has 119 throw new sites. Nearly all of them build the message on the spot, with concatenation, sprintf() or interpolation. Several build the same message in more than one class, and where they do the wording has already drifted apart.

The throw site should name the problem and nothing else:

namespace PhpDb\TableGateway\Exception;

use PhpDb\Exception;

use function sprintf;

class RuntimeException extends Exception\RuntimeException implements ExceptionInterface
{
    final public const string NO_PRIMARY_KEY = 'No information was provided to the RowGatewayFeature'
        . ' and/or no MetadataFeature could be consulted to find the primary key'
        . ' necessary for RowGateway object creation.';

    final public const string INVALID_MAGIC_CALL = 'Invalid method (%s) called, caught by %s::__call()';

    public static function forNoPrimaryKey(): self
    {
        return new self(self::NO_PRIMARY_KEY);
    }

    public static function forInvalidMagicCall(string $method, string $class): self
    {
        return new self(sprintf(self::INVALID_MAGIC_CALL, $method, $class));
    }
}

• Named constructors are static, return self and start with for.
• Each message is a single final public const string.
• Use new self(self::CONST) when the template has no placeholder, new self(sprintf(self::CONST, <args>)) when it does.

Convention

Naming a condition in a class instead of a method breaks the convention rather than following it. A condition earns its own class only when it carries something a named constructor can't:

1. Extra state, with accessors. Adapter\Exception\InvalidConnectionParametersException holds $parameters. Exception\ErrorException wraps a driver errno and message as a $previous cause, which is what the adapters build it for.
2. An external interface contract. Exception\ContainerException implements PSR-11's ContainerExceptionInterface.
3. A condition a consumer can act on. Adapter\Exception\InvalidQueryException is thrown by all three adapters and caught to log the SQL or retry.

Everything else is a named constructor on the component's SPL-shaped class. Adapter\Exception\VunerablePlatformQuoteException meets none of the three.

Strategy

1. Retype TableGateway\Exception\RuntimeException. It extends PhpDb\Exception\InvalidArgumentException today, so it isn't a \RuntimeException at all. It will extend Exception\RuntimeException. This breaks anyone catching it by the SPL type, and that's accepted. Exceptions need to be correct, and shipping a RuntimeException that is an InvalidArgumentException into 1.0 is worse.
2. Add PhpDb\Metadata\Exception. Metadata is the only component with no exception namespace, and its 7 bare \Exception throws are invisible to catch (PhpDb\Exception\ExceptionInterface). It gets ExceptionInterface, InvalidArgumentException and RuntimeException, matching every other component.
3. Converge the drifted messages, quoting caller-supplied identifiers. 'The PDO param "%s" contains invalid characters.' wins over the unquoted variant, and 'Table "%s" does not exist' wins over '"%s" does not exist'. The placeholder holds a name the caller passed in, so quoting shows where the name ends. Same rule everywhere: quote a caller-supplied identifier, leave a class name, method name or count unquoted.
4. Remove VunerablePlatformQuoteException rather than rename it. It fails all three tests above, so the condition becomes a method. Details under Considerations.

Background

Counts and line references are from 8bc38ea on 0.6.x.

Consolidation

• 'Identifier must be specified' appears four times. Sql\Predicate\In:43, IsNull:34 and Like:41 throw it as InvalidArgumentException; Between:50 throws it as a bare \LogicException. Same precondition, same words, two unrelated types.
• '$prefix must be a valid table prefix or null, empty string given' and '$separator must be a valid table separator, empty string given' are each written twice, in Sql\TableIdentifier:34,40 and Sql\TableIdentifierFactory:28,34.
• '%s expects parameter to be numeric, "%s" given' is written twice in one class, Sql\Select:318 and :335.
• 'Invalid magic property access in ' is concatenated twice, in TableGateway\AbstractTableGateway:451 and :466.
• '"%s" does not exist' (Metadata\Source\AbstractSource:173) and 'Table "%s" does not exist' (:380) report the same missing table two different ways.

Gaps

• PhpDb\Metadata has no Exception namespace. Metadata\Source\AbstractSource throws bare \Exception at lines 131, 173, 220, 380, 394, 474 and 554, two of them with an interpolated table, trigger or view name.
• Sql\AbstractSql:290 throws \ValueError('Invalid Argument type').
• Sql\Predicate\Between:50,54,58 throws \LogicException.

None of them implement PhpDb\Exception\ExceptionInterface, so a consumer catching the package's own marker interface misses them.

Defects

• src/TableGateway/Exception/RuntimeException.php extends PhpDb\Exception\InvalidArgumentException. catch (\RuntimeException) misses it and catch (\InvalidArgumentException) catches it. All 27 TableGateway throws that use it are mis-typed as a result.
• Exception\ContainerException implements PSR-11's ContainerExceptionInterface but not PhpDb\Exception\ExceptionInterface, unlike every other exception in the package.

Other considerations

Why the constants are final. The component base classes stay non-final, because the adapter packages extend Adapter\Exception\RuntimeException and InvalidConnectionParametersException and InvalidQueryException already do. Without final, a subclass could quietly redefine a published message template. Typed constants need PHP 8.3 and final const needs 8.1; the floor is ~8.3.0, so nothing moves.

The constants become public API. Once published, RuntimeException::NO_PRIMARY_KEY is something a consumer can reference and a downstream package can assert on. That's what lets tests and adapters stop hard-coding literals, but it also puts the constant names under BC policy. Settle the naming in PR 1, not in PR 5.

Message text only moves where it had drifted. Every existing message stays byte-identical except the drifted pairs, which converge on one wording each: the PDO-param message on the quoted form, and Metadata\Source\AbstractSource's two "table does not exist" messages on 'Table "%s" does not exist', matching the Trigger "%s" and View "%s" wording already in that class. Everything else keeps the 128 message assertions green and keeps each PR reviewable as a mechanical change.

Five changes break a catch, all on purpose. Metadata's bare \Exception becomes typed. Between's \LogicException and AbstractSql's \ValueError become PhpDb types. TableGateway\Exception\RuntimeException changes parent. VunerablePlatformQuoteException goes. catch (\LogicException) and catch (\ValueError) around those call sites stop matching. 0.5.0 is the current tag and 0.6.0 is unreleased, so this is the window. If the TSC would rather keep the SPL parents, Sql\Exception\LogicException extends \LogicException and Sql\Exception\ValueError extends \ValueError both work, at the cost of two more classes.

VunerablePlatformQuoteException is removed, not renamed. It holds no state, implements no external contract, and reports a setup error a caller can't recover from: quoting without extension or driver support. It fails all three tests, so its two throw sites (Adapter\Platform\Sql92:43 and AbstractPlatform:147) move to Adapter\Exception\RuntimeException::forVulnerablePlatformQuote(string $platformName, string $methodName). The misspelling goes with the class.

There's no BC shim for this one. The class is released (0.4.1, 0.5.0) and final, so nobody can have subclassed it, but a class_alias() to Adapter\Exception\RuntimeException would be worse than the break: catch (VunerablePlatformQuoteException) would quietly start catching every adapter runtime failure. So remove it and note it in the changelog. It already extends Adapter\Exception\RuntimeException, so anyone catching that is unaffected. In-org cost is 15 test references: 10 in core, 3 in phpdb-pgsql, 2 in phpdb-sqlite, none in phpdb-mysql.

Adapter packages. phpdb-mysql, phpdb-pgsql and phpdb-sqlite can use core's new named constructors straight away, but adapter-specific messages don't belong in core constants. Each adapter needs its own Exception namespace and its own issue. This RFC covers core, and should land first so the adapters have a pattern to copy.

Tests. Moving 128 message assertions from literals to Exception::CONST is mechanical but large, and it's what makes the constants worth having. It can go in its own PR.

Coverage. The repo targets 100%. Every named constructor is a new public method and needs a test for both the type and the rendered message. A #[DataProvider] over [factory args, expected message] per exception class stops that becoming 119 near-identical test methods.

Baselines. analyzer-baseline.toml and lint-baseline.toml both move, so each PR regenerates them. #182's typing work touches some of the same files.

Proposal(s)

One PR per item, each against this issue. Items 2 to 6 are independent of each other and depend only on item 1.

• PR 1: convention and hierarchy fixes. Document the convention and the three tests for when a condition earns its own class, in docs/book/. Point TableGateway\Exception\RuntimeException at Exception\RuntimeException. Make Exception\ContainerException implement Exception\ExceptionInterface. No throw sites change.
• PR 2: add PhpDb\Metadata\Exception. New ExceptionInterface, InvalidArgumentException and RuntimeException matching the other components. Replace the 7 bare \Exception throws in Metadata\Source\AbstractSource with forUnknownColumn(), forUnknownTable(), forUnknownConstraint(), forUnknownTrigger(), forUnknownView() and forUnsupportedTableType(), converging the two "table does not exist" messages on one template.
• PR 3: Sql, 51 sites. Named constructors across Sql, Sql\Predicate and Sql\Ddl. Hoist 'Identifier must be specified' to one constant shared by In, IsNull, Like and Between. Move Between onto the same InvalidArgumentException as its siblings. Give AbstractSql:290 a typed exception. Dedupe the two Select numeric messages and the four TableIdentifier and TableIdentifierFactory messages.
• PR 4: TableGateway and RowGateway, 34 sites. Named constructors. Collapse the two 'Invalid magic property access' concatenations and the RowGatewayFeature four-line concatenation into templates.
• PR 5: Adapter, 24 sites. Named constructors. Converge the two PDO-param messages on the quoted template. Keep InvalidConnectionParametersException's $parameters argument by giving it a named constructor that takes it. Remove VunerablePlatformQuoteException, move its two throw sites to RuntimeException::forVulnerablePlatformQuote(), and update the 15 test references across core, phpdb-pgsql and phpdb-sqlite.
• PR 6: ResultSet, Container and the test migration. The remaining 3 sites, then move the 128 message assertions in test/ onto the constants.

Test plan, per PR

• Every named constructor gets a unit test for the returned type and the rendered message, driven by a #[DataProvider] per exception class.
• Existing tests pass unchanged, since message text doesn't move. The exceptions are the PDO-param and Between tests, which change in their own PRs with the reason in the PR body.
• Each PR runs mago lint and format and the analyzer, then regenerates analyzer-baseline.toml and lint-baseline.toml.
• One new test asserts that every class in src/**/Exception/ implements PhpDb\Exception\ExceptionInterface and extends the SPL class its name claims, so the TableGateway\Exception\RuntimeException defect can't come back.

Appendix/Additional Info

Already in the tree: Adapter\Exception\VunerablePlatformQuoteException::forPlatformAndMethod() and Exception\ContainerException::forService().

Related issues: #182 typing inconsistency, which touches some of the same files; #22 1.0.0 scope of work; #8 document all members.

Out of scope: an Exception namespace and named constructors for phpdb-mysql, phpdb-pgsql and phpdb-sqlite. One issue each, after this lands.

Audit basis: 8bc38ea on 0.6.x.