feat!: 2.0 - stateless exception handling, PHP ^8.3, revived toolchain - #139
Merged
Conversation
Revives the php8 branch, untouched since May 2024, and lands it as 2.0. Three defects were confirmed by running the branch, not just read out of the diff: throwError() could not be used. The typed $exception property had no default, so calling it raised "Typed property must not be accessed before initialization" when nothing had failed. It also stayed uninitialised after a failed isXMLStringValid(), because that path caught InvalidXml and returned false without recording anything. Five parameters were implicitly nullable (string $xsdPath = null), which PHP 8.4 and newer report as deprecated on every call. libxml_use_internal_errors(true) was never restored, leaving libxml error handling switched on for the rest of the consumer's process. All three shared a cause: the error lived on the validator object. A stale error survived the next successful call, and two validations on a shared instance overwrote each other. The API now follows what the ecosystem settled on, a predicate plus a throwing variant: isXMLFileValid() / isXMLStringValid() bool, never throw validateXMLFile() / validateXMLString() void, throw with detail The predicates are implemented by catching from the throwing methods, so the two forms cannot drift apart. throwError() is gone. Every exception now implements the XmlValidatorException marker interface and extends RuntimeException, so one catch block replaces a union that would silently stop covering everything as exceptions get added. InvalidXml carries the LibXMLError objects, so line and column survive instead of being flattened into one imploded string. A missing XSD reports as FileDoesNotExist rather than surfacing as a libxml parse error about the document. PHP floor moves to ^8.3, replacing ^8.1|^8.2|^8.3, which excluded the 8.4 and 8.5 the CI matrix should be testing. phan and churn-php are dropped: phan duplicates phpstan at level max and needs ext-ast, which was in neither the local nor the CI environment, and churn reports a metric rather than gating anything. Dependency count falls from 67 to 42. phpstan now analyses against the whole 8.3 to 8.5 range so a version-specific slip is caught here rather than in a consumer's runtime. Tests were rewritten and split by concern. The old suite wrapped assertions in "if (is_string($contents))", so an unreadable fixture made the test pass having asserted nothing; fixtures now load through a helper that fails loudly. Coverage is 100% of lines, methods and classes, with PHPUnit set to fail on risky, warning, deprecation and notice. CI had rotted. actions/checkout was still v1, composer self-update ran on every job, --no-suggest has not existed since Composer 2, and the matrix listed a psalm job with no matching script or dependency, so it could only ever fail. Mergify gated Dependabot merges on a Travis check retired years ago, which is part of why this repo drifted. A Composer audit job now guards the committed lock, and dependabot.yml gained the github-actions ecosystem that would have kept checkout current. The Scrutinizer coverage job is not carried over: ocular.phar now sits behind a Cloudflare challenge and returns a 403 HTML page rather than the phar, so the job could not work. Its badge is removed with it. BREAKING CHANGE: throwError() is removed, exceptions extend RuntimeException instead of Exception, and PHP 8.3 is the minimum. See the migration table in README.md.
Contributor
Tick the box to add this pull request to the merge queue (same as
|
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Revives the
php8branch, untouched since 2024-05-15, and lands it as 2.0.0.This PR carries two commits: the parked 2024 modernisation, and the 2.0 work on top. Reviewing the second commit alone is enough — the first is already on
php8.Why now
Dependabot has been reporting advisories on this repo while its own security-update jobs failed. It could not resolve
masterat all, becausemasterstill declares"php": ">=7.2"with 2024-era tooling:Patching
masterin place would have meant rewritingrequire-devanyway, which is most of whatphp8already did.Defects fixed
Found by running the branch, not by reading the diff.
1.
throwError()was unusable. The typed$exceptionproperty had no default:Raised both when nothing had failed and after a failed
isXMLStringValid()— that path caughtInvalidXmland returnedfalsewithout recording anything, so string validation could never report a reason.2. Five implicitly nullable parameters.
string $xsdPath = nullin threeValidatormethods and bothValidatorInterfacemethods, which PHP 8.4+ reports as deprecated on every call. The package declared^8.1|^8.2|^8.3, so this was invisible to its own CI.3. Global state leak.
libxml_use_internal_errors(true)was never restored, leaving libxml error handling switched on for the rest of the consumer's process.Design
All three shared one cause: the error lived on the validator object, so a stale error survived the next successful call and two validations on a shared instance overwrote each other.
The API now follows the shape the ecosystem settled on (
league/flysystem'sFilesystemException, PSR-18'sClientExceptionInterface, Respect/Validation'svalidate()/assert()split) — a predicate plus a throwing variant:isXMLFileValid()/isXMLStringValid()bool, never throwvalidateXMLFile()/validateXMLString()void, throw with detailThe predicates are implemented by catching from the throwing methods, so the two forms cannot drift apart.
throwError()is removed.Every exception implements the
XmlValidatorExceptionmarker interface and extendsRuntimeException, so one catch block replaces a union that silently stops covering everything as exceptions are added.InvalidXmlkeeps theLibXMLErrorobjects, soline 2 column 23: xmlParseEntityRef: no namesurvives where before you got one imploded string. A missing XSD now reports asFileDoesNotExistinstead of surfacing as a libxml parse error about the document.Closes the gap #138 was aiming at, from a different direction — worth a reply to @duncan412 either way, since that report identified a real bug.
Toolchain
^8.3, replacing^8.1|^8.2|^8.3which excluded the 8.4/8.5 the matrix should testext-ast, present in neither the local nor the CI environment, so it had been running through the slow polyfill. Churn reports a metric rather than gating anything. Dependencies fall 67 → 42. Easy to reverse if you disagree.phpcs.xml.distandphpstan.neon.distreplace inline CLI flags; both coversrc,testsandexample.phpTests
Rewritten and split by concern. The old suite wrapped assertions in
if (is_string($contents)), so an unreadable fixture made the test pass having asserted nothing; fixtures now load through a helper that fails loudly.PHPUnit runs with
failOnRisky,failOnWarning,failOnDeprecation,failOnNoticeandrequireCoverageMetadata.CI and automation
actions/checkout@v1→@v7composer self-updateand--no-suggest, which has not existed since Composer 2psalmmatrix entry: no matching script and novimeo/psalmdependency, so it could only ever failComposer auditjob guarding the committed lock, deliberately outside the matrix because--prefer-lowestresolves to the oldest permitted versions and will always carry advisories.mergify.ymlgated Dependabot merges oncontinuous-integration/travis-ci/pr, retired years ago — part of why this repo drifted. Now gates on the test and audit checks.dependabot.ymlgained thegithub-actionsecosystem that would have keptcheckoutcurrentNot carried over: the Scrutinizer coverage job.
ocular.pharnow sits behind a Cloudflare challenge and returns a 403 HTML page rather than the phar, so the job could not work. Its badge is removed with it; the quality badge stays. Say the word if you want coverage reporting back and I'll wire Codecov.Verification
example.phpruns deprecation-free on real PHP 8.4.24 and 8.5.9 — the version that exhibited defect 2. Also checked untrusted input: external entities do not resolve and an entity-expansion bomb is rejected in 1ms, so there was nothing to fix there.Breaking changes
throwError()removed, exceptions extendRuntimeExceptioninstead ofException, PHP 8.3 minimum. Migration table is inREADME.md.Tag as 2.0.0 after merge.