Type DOMDocument load/save/validate string arguments as non-empty-string on PHP 8 - #6075
Conversation
…string` on PHP 8
- Since PHP 8.0 the `$source`/`$filename` arguments of DOMDocument's
load/save/validate methods throw a `ValueError` ("must not be empty")
when given an empty string; the stub now types them as `non-empty-string`
so passing `''` is reported.
- Moved the `DOMDocument` class out of `stubs/dom.stub` into two version-gated
stub files selected by a new `DomDocumentStubFilesExtension`:
`stubs/DOMDocument.stub` (< PHP 8, original behaviour) and
`stubs/DOMDocument_php8.stub` (>= PHP 8, `non-empty-string` arguments),
mirroring the existing `socket_select` version-gated stub pattern.
- Swept the whole sibling family, not just the reported `loadHTML`:
`load`, `loadXML`, `loadHTML`, `loadHTMLFile`, `save`, `saveHTMLFile`,
`schemaValidate`, `schemaValidateSource`, `relaxNGValidate`,
`relaxNGValidateSource` — all verified to throw `ValueError` on empty input.
- Adjusted `JunitErrorFormatterTest` to assert non-empty output before
`DOMDocument::loadXML()`, matching the stricter stub.
VincentLanglet
left a comment
There was a problem hiding this comment.
Can't we use functionMap_php80delta instead ?
Replace the version-gated stub-file approach with entries in functionMap_php80delta.php, the idiomatic mechanism for version-specific signatures. The base functionMap.php keeps the pre-PHP-8 plain-string signatures; the php80delta 'new' section overrides the ten load/save/validate methods with non-empty-string arguments on PHP 8.0+. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed VincentLanglet's review. Here's what I did: Response to the reviewYes — ChangesReverted the stub-file approach:
Used the function map delta instead:
The parameter names in Verification
Note: because The commit is pushed to the PR branch. |
staabm
left a comment
There was a problem hiding this comment.
I think we should move the new signatures into functionMap_php80delta_bleedingEdge because narrowing of parameter types is a BC break
…a_bleedingEdge Narrowing the load/save/validate string arguments from string to non-empty-string is a BC break, so gate it behind the stricter function map (bleeding edge) rather than applying it unconditionally on PHP 8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed staabm's review. What I didMoved the ten DOMDocument Changes
This file is only loaded when Why no test/other changes were needed
Verification
Committed as |
Summary
Since PHP 8.0,
DOMDocument::loadHTML()(and its sibling load/save/validatemethods) throw a
ValueErrorwhen passed an empty string(
Argument #1 ($source) must not be empty). PHPStan previously typed thesearguments as plain
string, so calling e.g.$doc->loadHTML('')was notreported. This change types those arguments as
non-empty-stringon PHP 8+so the mistake is caught.
Changes
src/PhpDoc/DomDocumentStubFilesExtension.php, a version-gatedStubFilesExtensionthat loadsstubs/DOMDocument_php8.stubon PHP >= 8.0and
stubs/DOMDocument.stubon older versions (same pattern asSocketSelectStubFilesExtension).DOMDocumentclass definition out ofstubs/dom.stub(which keepsthe other DOM classes) into the two new stub files. A stub class may only be
declared in one loaded stub file, so the class had to be extracted to be
version-gated.
stubs/DOMDocument_php8.stubtypes the string arguments of the followingmethods as
non-empty-string:load,loadXML,loadHTML,loadHTMLFile,save,saveHTMLFile,schemaValidate,schemaValidateSource,relaxNGValidate,relaxNGValidateSource.stubs/DOMDocument.stubpreserves the original pre-PHP-8 signatures(arguments stay plain
string), so no false positives for code targetingPHP 7.
tests/PHPStan/Command/ErrorFormatter/JunitErrorFormatterTest.phpto assert the captured output is non-empty before passing it to
DOMDocument::loadXML(), since the stub is now stricter.Root cause
The DOM stub described the load/save/validate arguments as
string, but onPHP 8+ these methods reject the empty string with a
ValueError. The reportedloadHTMLbug is one instance of a whole family: every DOMDocument method thatconsumes a source string or a file path got the same "must not be empty"
ValueErrorin the PHP 8.0 ext/dom cleanup. All ten were verified against theruntime and fixed together. The fix is version-gated because on PHP 7 these
methods return
falsefor an empty string instead of throwing, sonon-empty-stringmust not be enforced there.Test
tests/PHPStan/Rules/Methods/data/bug-14979.php+CallMethodsRuleTest::testBug14979()(guarded with#[RequiresPhp('>= 8.0.0')]) assert that passing''to all ten DOMDocumentmethods is reported as
expects non-empty-string, '' given., while non-emptyliterals (
loadHTML('<html></html>'),loadXML('<root/>')) are accepted.The test fails without the stub change (no errors reported) and passes with it.
Fixes phpstan/phpstan#14979