[simon-mundy]

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• QA improvement (unit/integration tests, CS fixes, etc.) that does not change code behavior

Checklist:

• An issue has been created for this pull request.
• I have added tests for my changes.
• All new and existing tests passed.
• Documentation has been updated accordingly.
• This pull request targets the correct branch.

Description

PR 3 of 6 from #189, and the largest: all 55 throw sites in src/Sql.

Stacked on #190. GitHub cannot base a cross-fork PR on a fork branch, so this targets 0.6.x and shows both commits. Only the second, 84e15389, belongs to this PR. Merge #190 first; I will rebase after. It does not depend on #191.

Message text is byte-identical to what src/Sql produced before, verified template by template against the original literals rather than by eye.

New namespace. Sql\Predicate\Exception holds the 10 templates the predicate classes raise, so they sit with the classes that raise them. This is purely additive: Sql\Predicate\Exception\InvalidArgumentException extends Sql\Exception\InvalidArgumentException, so anything catching the parent is unaffected.

The namespace deliberately has no ExceptionInterface of its own. An earlier revision of this PR added one for symmetry with the other components, and it earned nothing: because both classes extend their Sql\Exception counterparts, they are already caught by Sql\Exception\ExceptionInterface, by Sql\Exception\InvalidArgumentException and by the root marker. A narrower marker would only have collapsed the same InvalidArgumentException / RuntimeException split a second time, over a strict subset of what Sql\Exception\ExceptionInterface already covers — four classes against two. It was unreleased, so dropping it costs nothing. The split leaves 24 factories on Sql\Exception\InvalidArgumentException and 6 on Sql\Exception\RuntimeException, against 9 and 1 on the new classes. Four of those 24 cover the Sql\Ddl column and check-constraint messages added by #183, #184 and #185, which landed on 0.6.x after the audit on #189 was taken.

Nine duplicated messages collapse onto one template each, five of which the audit on #189 had missed:

Message	Copies	Where
This Sql object is intended to work with only the table "%s"...	4	Sql
Identifier must be specified	4	Predicate\Between, In, IsNull, Like
A number of parameters (%d) was found...	2	AbstractSql
The key %s was not found in this objects column list	2	Insert
Since this object was created with a table and/or schema...	2	Select
The subject does not appear to implement %s, thus calling %s...	2	Platform, AbstractPlatform
$prefix must be a valid table prefix or null...	2	TableIdentifier, TableIdentifierFactory
$separator must be a valid table separator...	2	TableIdentifier, TableIdentifierFactory
%s expects parameter to be numeric, "%s" given	2	Select

A copy-paste bug, fixed. Platform::getSqlString() reported '...thus calling prepareStatement() has no effect'. Parameterising the subject template exposed it. Nothing asserted the string, so that path had no coverage; the two platform tests now assert the full rendered message.

Two retypes. Predicate\Between's \LogicException and AbstractSql's \ValueError become package types, so both reach PhpDb\Exception\ExceptionInterface.

Between is not a BC break, contrary to what #189 says: \InvalidArgumentException extends \LogicException, so every existing catch (\LogicException) still matches. BetweenTest passed untouched while still expecting \LogicException, which is how this surfaced; it now asserts the precise type. AbstractSql's \ValueError is a real break, since \Error and \Exception are unrelated branches. That leaves three genuine catch-breaking changes across the whole RFC rather than five.

Also in passing: Predicate::unnest() and Predicate::__get() never declared what they throw, and PredicateSet imported Exception from inside its own namespace.

Motivation and Context

Refs #189. Depends on #190 for the convention, the constant naming rule and the structural guards. src/Sql is 51 of the 119 throw sites in the package and the worst concentration of duplicated message text.

On the target branch. Targets 0.6.x although it carries a breaking change, for the reason agreed on #189: 0.5.0 is the current tag and 0.6.0 is unreleased. The only genuine break here is AbstractSql's \ValueError, on a default => match arm that no test reaches.

How has this been tested?

mago format --check, mago lint and mago analyze all exit 0. Full unit suite passes: 1743 tests, 3642 assertions.

Every factory is covered: 24/24, 6/6, 9/9 and 1/1. Templates are verified byte-identical against the original literals by a throwaway script asserting all 36 before the call sites were touched, and the four generated template tests assert each rendered message thereafter. The 46 src/Sql message assertions now derive from the templates rather than restating them.

One thing worth knowing, because it will recur in any PR that touches a method its test class does not declare: before the per-factory tests existed, clover reported only 3 of 36 factories covered, yet deliberately breaking a factory failed 5 tests, proving the code was exercised. The cause is #[CoversMethod]. Once a test class declares any, PHPUnit records coverage only for the methods listed, so code executed outside that list is not attributed. The factories were reached through the classes under test, which do not declare them. The per-factory tests declare their own CoversMethod and resolve the number. #194 hit the same thing on Adapter::executeQuery(), where four existing tests exercised a method the class had never declared.

Baseline and configuration changes

• The two baselined sprintf argument-type entries in analyzer-baseline.toml are re-attributed, not added to: the same unsoundness now reports against forInvalidJoinName and forForeignTable, with counts matching exactly (1 and 4). Underlying cause, worth its own issue: TableIdentifier has no __toString(), so Sql::forForeignTable($this->table) fatals when $this->table is a TableIdentifier.
• Three halstead readings shift, in AbstractSql, Select and Ddl\Column\AbstractPrecisionColumn. Removing long string literals reduces distinct operands, which raises halstead difficulty, so simplifying these methods moved the metric the wrong way.
• too-many-methods is excluded for src/Sql/Exception/ in mago.toml rather than baselined. The threshold is 10, verified empirically against mago 1.50.0. A catalogue holds one template per condition a component can report, so the count tracks failure modes; the remaining 20 are a long tail of 9 consumers, 7 of them with one or two factories, so there is no cohesive further split to make. phpdb-qa-tools is untouched. This will conflict with Route the TableGateway and RowGateway throw sites through named constructors #193 on the same line, which adds src/TableGateway/Exception/; whichever merges second needs the two paths merged into one array.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The merge-base changed after approval.