Resolve PHP version checks in src/Type/Php extensions from Scope::getPhpVersion() - #6551
Conversation
b9c1900 to
c805f01
Compare
|
This pull request has been marked as ready for review. |
…PhpVersion() Return-type, throw-type and type-specifying extensions under src/Type/Php asked a DI-injected PhpVersion which PHP version is analysed. That ignores both PHP_VERSION_ID narrowing in the analysed code and a configured phpVersion min/max range. They now read the range-aware PhpVersions from the call's Scope, along with the helpers they share (ArrayColumnHelper, ArrayFilterFunctionReturnTypeHelper, FilterFunctionReturnTypeHelper, MbFunctionsReturnTypeExtensionTrait). A new build rule reports scope-aware extensions that inject PhpVersion. The three reflection extensions converted in a separate pull request are baselined here until that lands. Split out of phpstan#6529. The Regex* classes are left for a dedicated change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c805f01 to
5ce825a
Compare
SanderMuller
left a comment
There was a problem hiding this comment.
Reviewed at 5ce825acc. The conversions hold up. Three small things to clean up, and one behaviour change that the integration tests already show.
Cleanup
- The description is stale. It still mentions the build rule, the baselined reflection extensions and
MbFunctionsReturnTypeExtensionTrait, and all three were taken out in4b21fe5b1,d7dfbbb05and5ce825acc. The body becomes the squash commit. - Eight new
PhpVersionsmethods have no caller. Themb*,str_splitand PDO changes they served were reverted:hasPDOSubclasses(),isEmptyStringValidAliasForNoneInMbSubstituteCharacter(),isNullValidArgInMbSubstituteCharacter(),isNumericStringValidArgInMbSubstituteCharacter(),strSplitReturnsEmptyArray(),supportsAllUnicodeScalarCodePointsInMbSubstituteCharacter(),supportsPassNoneEncodings()andthrowsOnInvalidMbStringEncoding(). Each one is only called on the injectedPhpVersion. PDOConnectReturnTypeExtensionis only a line-wrap change now.
The slevomat integration red
The Integration - slevomat/coding-standard job that analyses slevomat's sources reports 3 new errors. The first is:
Property SlevomatCodingStandard\Sniffs\Commenting\AnnotationNameSniff::$normalizedAnnotations (array<string, string>|null) does not accept array<string, string>|false.
They come from two array_combine() calls and one substr($requiredName, 0, -1) at slevomat's pinned 6eff0ed5. slevomat requires "php": "^7.4 || ^8.0" and sets no phpVersion or platform pin. The base therefore answers for the 8.4 runtime, and this PR answers for the range, where both functions can return false on 7.4. That is what this PR is for. It also means every library with a 7.x lower bound and no phpVersion config gets these errors on upgrade. Is that intended for 2.3.x as it is, or should it wait for bleedingEdge?
What I checked
- All four new test files fail with
src/reset to the merge base4c63e37e5. The range test fails 2 of 4, the nsrt file 19 assertions, and both throw-type tests fail. - I dumped 17 expressions at fixed 7.2, 7.4 and 8.5 and at two ranges. At a fixed version, base and PR agree on all of them. Under a range, only
substr('', 5)widens, to''|false, and nothing narrows. version_compare($a, $b, '<')also changes at a fixed version:bool|nullon base at 7.4,boolhere. That is a fix, but it is worth a line in the description too.- Self-analysis reports the same error set as the merge base. phpcs is clean, and no touched class is turbo-shadowed.
- I could not run PHP 8.2, so the host-independent
DateTime::modify()detection is only covered by CI.
Performance
I counted MutatingScope::getPhpVersion() calls in a single-process self-analysis: 214,761 on the merge base and 220,476 here, so 2.7% more. For them to cost even 0.1% of a self-analysis of about 150s CPU, each added call would need about 26µs.
CI
The other reds match #6485 or the 2.3.x tip. Integration - shopsys/shopsys failed on Phing PHAR could not be downloaded, and its second job passed.
|
thank you! |
Return-type, throw-type and type-specifying extensions under src/Type/Php asked a DI-injected PhpVersion which PHP version is analysed. That ignores both PHP_VERSION_ID narrowing in the analysed code and a configured phpVersion min/max range. They now read the range-aware PhpVersions from the call's Scope, along with the helpers they share
A new build rule reports scope-aware extensions that inject PhpVersion. The three reflection extensions converted in a separate pull request are baselined here until that lands.
Split out of #6529. The Regex* classes are left for a dedicated change.
Extracted the no brainer parts into this separate PR so its easier to fiddle focused with the more complex ones