Repository navigation
Introduce the named exception constructor convention - #190
Merged
simon-mundy merged 4 commits intoSep 29, 2026
Merged
simon-mundy merged 4 commits into
simon-mundy merged 4 commits into
Conversation
8 tasks done
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
Author
This was referenced Sep 28, 2026
- Document the convention, the constant naming rule and the three tests for when a condition earns its own class in docs/book/exceptions.md - Point TableGateway\Exception\RuntimeException at Exception\RuntimeException rather than Exception\InvalidArgumentException, so it is a \RuntimeException - Declare the @throws the retype exposed on AbstractTableGateway, TableGateway::__construct and GlobalAdapterFeature::preInitialize, which the wrong parent had been covering - Extend ContainerException from Exception\RuntimeException so it carries the package marker interface, and move its message to a SERVICE template - Guard the marker interface, the SPL parents and the constant naming rule in ExceptionHierarchyTest, scanning src so new classes are covered on sight - Assert ContainerException's message against its template and drop the file's stale lint baseline entries Refs php-db#189 Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
simon-mundy
force-pushed
the
exception/pr-1-convention-and-hierarchy
branch
from
September 28, 2026 06:38
08c2230 to
28c25a9
Compare
This was referenced Sep 28, 2026
tyrsson
self-requested a review
September 28, 2026 08:58
tyrsson
requested changes
Sep 28, 2026
tyrsson
left a comment
tyrsson
left a comment
Member
There was a problem hiding this comment.
Lets keep it simple and just glob the exception classes if we are going to approach it from this direction.
- List every concrete exception in ExceptionHierarchyTest::EXCEPTION_CLASSES, mapped to the SPL class it must extend, in place of scanning src - Assert the SPL parent for every class rather than only those named after an SPL exception, and drop the name-based SPL_PARENTS map - Move the named constructor helpers into the test and delete the ExceptionClassLocator test asset Refs php-db#189 Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
simon-mundy
force-pushed
the
exception/pr-1-convention-and-hierarchy
branch
from
September 28, 2026 10:29
1cc8804 to
fc72ca4
Compare
This was referenced Sep 28, 2026
tyrsson
previously approved these changes
Sep 29, 2026
…rowable Two changes to the marker interface, both surfaced while reviewing php-db#189's convention against the code. The root interface did not extend Throwable, which was unintentional. It left the interface usable only in a catch clause: a parameter typed against it was not known to be throwable and had no getMessage(), so `mago analyze` reported non-existent-method and invalid-throw on any code that tried. It now extends Throwable, and ExceptionHierarchyTest pins that so it cannot regress. The interface carries a one-line docblock saying what it marks, and nothing about the change itself. Each component also declared its own ExceptionInterface extending the root one. Those are removed: Adapter, ResultSet, RowGateway, Sql and TableGateway. Every concrete exception class extends its counterpart in PhpDb\Exception, which is where the marker is declared, so all 20 still implement it by inheritance and the 15 redundant `implements` clauses are gone. docs/book/exceptions.md records the single-interface rule and shows how to catch a whole component by naming its classes. BC: the five component interfaces ship in 0.5.0, so `catch (PhpDb\Sql\Exception\ExceptionInterface)` and the equivalents stop resolving. Nothing in this repository or the adapter packages catches one; phpdb-mysql names them in @throws docblocks only, which its own RFC covers. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- ContainerException::forService() passed 0 explicitly as $code, which is the RuntimeException default. Infection reported two escaped mutants on it, IncrementInteger and DecrementInteger, because nothing asserts the code. Removing the argument removes the literal rather than adding an assertion for a value PHP already supplies; getCode() is unchanged and Covered Code MSI for the file goes from 33% to 100% - ExceptionHierarchyTest's naming-guard exemption named the pull request that would remove the class. The reason for the exemption stays; the reference to work that will be merged and forgotten does not Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
simon-mundy
force-pushed
the
exception/pr-1-convention-and-hierarchy
branch
from
September 29, 2026 03:32
9c7bb07 to
2886bc7
Compare
tyrsson
approved these changes
Sep 29, 2026
simon-mundy
added a commit
that referenced
this pull request
Sep 29, 2026
Drops 18 entries for issues that no longer exist: the seven per-component ExceptionInterface files deleted in PR #190, four exception classes that are now final, three resolved unhandled-thrown-type and mixed-argument sites, and the unfulfilled-expect and prefer-test-attribute entries settled by the mutation coverage work in PR #188. Suppressed issue totals are unchanged at 3948 lint and 1215 analyzer, so no new findings are masked.
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
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.
Types of changes
Checklist:
Description
PR 1 of 6 from #189. Establishes the named exception constructor convention and fixes the two hierarchy defects the audit found. No throw sites move in this PR.
docs/book/exceptions.mddocuments the convention, the constant naming rule (forFooBar()usesFOO_BAR) and the three tests for when a condition earns its own class rather than a named constructor.TableGateway\Exception\RuntimeExceptionextendedPhpDb\Exception\InvalidArgumentException, so it was not a\RuntimeExceptionat all. It now extendsException\RuntimeException.@throwsannotations onAbstractTableGateway,TableGateway::__constructandGlobalAdapterFeature::preInitialize. The wrong parent had been letting@throws Exception\InvalidArgumentExceptioncover a thrownRuntimeException, so the analyzer accepted them. Declared.ContainerExceptionimplemented PSR-11'sContainerExceptionInterfacebut notPhpDb\Exception\ExceptionInterface, unlike every other exception in the package. It now extendsException\RuntimeException, picking the marker up through its parent, and its message moved to aSERVICEtemplate.ExceptionHierarchyTestguards all three rules by scanningsrc/, so a new exception class is covered as soon as it is written: every concrete exception implements the marker interface, a class named after an SPL exception extends that SPL exception, and everyfor*factory has the template constant its name implies.VunerablePlatformQuoteExceptionis exempt from the naming guard, with the reason in code: its factory is named for its arguments rather than the fault and holds its message inline. PR 5 removes the class, which retires the exemption.ErrorExceptionis deliberately absent from the SPL-parent map, also documented in code. BothPhpDb\Exception\ErrorExceptionandPhpDb\Adapter\Exception\ErrorExceptionextend\Exceptionrather than\ErrorException, and\ErrorException's third constructor argument is$severitywhere\Exception's is$previous. Correcting the parent would change a released constructor contract that all three adapters construct against, so it needs its own decision.Motivation and Context
Refs #189.
src/has 119throw newsites that build their messages inline, several of them building the same message in more than one class. This PR lands the rule and the guards so PRs 2 to 6 have something to conform to, and fixes the two defects that make the current hierarchy wrong.On the target branch. This targets
0.6.xrather than the next major, against the template's guidance for backwards incompatible changes. RetypingTableGateway\Exception\RuntimeExceptionbreakscatch (\InvalidArgumentException)around any of the 27 TableGateway throws that use it. The reasoning, agreed on #189, is that 0.5.0 is the current tag and 0.6.0 is unreleased, so this is the window, and shipping aRuntimeExceptionthat is anInvalidArgumentExceptioninto 1.0 is the worse outcome. Happy to retarget if the TSC would rather hold it for the next major.How has this been tested?
mago format --check,mago lintandmago analyzeall exit 0. Full unit suite passes: 1561 tests, 3437 assertions, verified on this branch alone rather than only as part of the stack.Each of the three guards was verified to fail on the defect it exists for, by reintroducing that defect and confirming the failure.
ContainerException::forService()is at 1/1 methods and 9/9 lines covered.Baseline changes are limited to removing three
test/unit/Exception/ContainerExceptionTest.phpentries fromlint-baseline.tomlthat this PR makes stale.analyzer-baseline.tomlis untouched.