chore: raise PHPStan to level 9 - #27
Merged
Merged
Conversation
Level 5 left the whole public surface untyped: every getter returned a bare array, so nothing described the shape of a placemark or a style to an IDE, and no analysis could catch a caller reading a key that is never set. It also hid three real defects. loadFromFile() passed the result of file_get_contents() straight on. An unreadable file, a permissions problem rather than a missing one, made that false, which became the empty string and surfaced as a parse error about the content instead of about the file. It now throws KmlParserException::failedToRead() with the path. Every xpath() call was iterated without checking for the false it returns on a malformed expression, and preg_split() the same. Both now fall back to an empty array. The KMZ limits were read from config with a blind (int) cast, so a non-numeric value silently became 0, which turns the limit off. That is the opposite of what someone setting a limit wants, so a value that is not a number now falls back to the documented default. Fixed at the source: the baseline is still empty, and there are no phpstan-ignore comments, no inline @var overrides and no casts added to quiet the analyser.
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.
Why
Level 5 was letting the entire public surface through untyped:
A bare
arraytells an IDE nothing and tells the analyser nothing. There is no way to catch a caller reading$placemark['coordinates']on a MultiGeometry, which is exactly the mistake the README now warns about in prose.Level 9 also found three real defects, which is the actual argument for the bump.
Defects found
1. An unreadable file reported the wrong problem
file_get_contents()returnsfalsewhen the file exists but cannot be read: a permissions problem, a file being written, a broken symlink.falsewas passed straight intoloadFromString(), where it became'', and the user gotXML parsing error: String could not be parsed as XML.That points at the content. The content was fine. The file could not be opened.
Now throws
KmlParserException::failedToRead():Unable to read KML file: <path>.loadFromFile()already had afile_exists()check for the missing case, so the two are now distinguishable.2.
xpath()andpreg_split()failures were iteratedSimpleXMLElement::xpath()returnsarray|false|null, andpreg_split()returnsarray|false. Both were fed straight toforeach. Five call sites across the parser, the validator and the coordinate trait. All now fall back to an empty array.3. A non-numeric archive limit turned the limit off
(int) 'plenty'is0, and0is the documented way to disable the limit. So a typo in the config silently removed the protection added in #24, which is the worst possible failure direction for that particular setting. A value that is not a number now falls back to the documented default.Typing
ParsesCoordinatesdeclaresPositionandPolygonBoundariesas@phpstan-type, imported byKmlParser, so the precise shapes are written once.The getters are typed as precisely as they honestly can be:
list<Placemark>,array<string, Style>,array<string, array{id: string, pairs: array<string, string>}>,array{type: string, features: list<array<string, mixed>>}. A placemark really is a heterogeneousarray<string, mixed>, and pretending otherwise would be a fiction the analyser then enforces on everyone. Typed DTOs are the real answer there and remain a separate, breaking piece of work.The GeoJSON conversion is narrowed with
is_array()andis_numeric()rather than by widening the signatures. As a side effect, a hand-built or partially malformed geometry array now returnsnullor0.0for the bad part instead of raising a TypeError, so one broken coordinate cannot take a whole document's conversion down.Fixed at the source
phpstan-baseline.neonis still 0 bytes@phpstan-ignorecomments@varoverrides (the two@varinKmlValidatorare pre-existing property declarations, not suppressions)Two
@suppressions, deliberate@file_get_contents()and, from #24,@mkdir(). In both cases the return value is what is acted on, and the exception thrown carries the path while PHP's warning does not. Without the@, an application converting warnings to exceptions getsfile_get_contents(): Failed to open stream: Permission deniedinstead ofUnable to read KML file: <path>. Both carry a comment saying so, and both are covered by a test that asserts the package's message, not PHP's.Tests
Unable to read KML file(skipped on Windows, wherechmoddoes not deny reads the same way)KML file not found, so the two cases stay distinctmax_archive_entriesfalls back to the default instead of disabling the limitSuite 88 to 91.
phpstan analysereports[OK] No errorsat level 9, Pint clean.