Conversation
Adds a minimal phpstan.neon.dist (level 5, paths: src, treatPhpDocTypesAsCertain: false) and a `composer phpstan` script. PHPStan is deliberately not added to require-dev: it needs PHP 7.4+ while this package supports PHP 5.6+, and the lint, test and quicktest workflows run `composer install` on PHP 5.6 and 7.0, where the dependency cannot resolve. The script therefore expects `phpstan` on the PATH (setup-php's `tools:` in CI, phive or a global Composer install locally), the same way the docs workflow provides phpdoc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fixes reported at level 5: - Iri::get_authority() is documented as returning string but returns null when the IRI has no authority, matching get_iauthority(). The docblock and the `authority` / `iauthority` magic properties now say string|null. - Iri::$port holds an int (set_port() casts, tests assert 80), not the string its @var and the `port` magic property claimed. - Iri::set_authority() compared the substr() result with false, which can only happen on PHP < 8.0; cast to string so the empty-port check works the same on every supported version. - Requests::parse_multiple() documented its by-reference $response as string, but overwrites it with a Response or the parsing Exception. The remaining 35 findings are docblock drift, PHP-version stub mismatches for cURL resources on PHP < 8.0, and undefined-variable false positives in Iri's UTF-8 decoding loop. They are recorded in phpstan-baseline.neon so the run is clean and new findings surface. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds a `phpstan` job alongside PHPCS. PHPStan itself comes from setup-php's `tools:` input, since it cannot be a Composer dev dependency of a PHP 5.6+ package; Composer dependencies are still installed so PHPStan can use the project autoloader. The github error format annotates findings inline on pull requests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1288654 to
e753e78
Compare
|
Force-pushed to drop |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@whyisjake Thank you for your interest in contributing.
You couldn't know this, but this was already being worked on behind the scenes and what you've paid for now to let Claude do, is not the way to go.
As things are this PR is:
- Incomplete.
- Incorrect.
- Clearly created without the "human in the loop", while AI-assisted PRs require a "human in the loop" per the WP guidelines.
- And adding an extra maintenance burden on the maintainers instead of lessening it.
I appreciate your efforts, but I'm not keen (at all) on merging this PR in its current form.
|
|
||
| # Run static analysis. The github error format annotates findings inline in PRs. | ||
| - name: Run PHPStan | ||
| run: composer phpstan -- --no-progress --error-format=github |
There was a problem hiding this comment.
| run: composer phpstan -- --no-progress --error-format=github | |
| run: phpstan analyse --error-format=github |
There was a problem hiding this comment.
Done, the job now runs phpstan analyse --error-format=github directly.
There was a problem hiding this comment.
These changes do not belong here. PHPStan is not being installed via Composer, so these scripts will not work out of the box and only cause errors and confusion.
Instead, the CONTRIBUTING guide needs to be updated to explain to download the PHPStan PHAR file and run that locally.
There was a problem hiding this comment.
Reverted. I added a Static Analysis section to CONTRIBUTING.md with the PHAR download and run instructions instead.
There was a problem hiding this comment.
This file needs to be listed in the .gitattributes export-ignores.
Along the same lines, phpstan.neon (local override) needs to be added to .gitignore.
There was a problem hiding this comment.
Added phpstan.neon.dist (and the new phpstan-bootstrap.php) to the export-ignore list, and phpstan.neon to .gitignore.
| includes: | ||
| - phpstan-baseline.neon | ||
|
|
There was a problem hiding this comment.
There should be no baseline file.
Either fix issues or if they are false positives (which PHPStan is prone to throw), list them explicitly in this file under ignoreErrors.
There was a problem hiding this comment.
Removed the baseline. Each remaining finding is now either fixed or listed under ignoreErrors with an explanation of why it's ignored. None of them are PHPStan bugs I could link to: they're PHP < 8 stub differences (cURL resources), constants defined at runtime that PHPStan resolves as fixed, the PHPUnit version shim, and tests that pass invalid input on purpose. Happy to reword or fix any entry you'd rather not see ignored.
There was a problem hiding this comment.
Please be aware that - as explicitly noted in the class docblock - this is an externally (un)maintained file and does not comply with our coding standards etc.
There was a problem hiding this comment.
Understood. I reverted my changes to this file and excluded it from analysis, with a comment in the config saying why.
There was a problem hiding this comment.
There should be no baseline file.
Either fix issues, or if they are false positives (which PHPStan is prone to throw), list them explicitly in the phpstan.neon.dist file under ignoreErrors with a link to the bug report. In some cases, PHPStan will refuse to fix their bugs, in that case, still list in ignoreErrors with an explanation.
There was a problem hiding this comment.
Removed. See the reply on phpstan.neon.dist for how the remaining findings are handled.
| paths: | ||
| - src |
There was a problem hiding this comment.
Analysing src is all nice and dandy, but why not analyze all the code ?
| paths: | |
| - src | |
| bootstrapFiles: | |
| - tests/bootstrap.php | |
| paths: | |
| - build/ghpages | |
| - examples | |
| - library | |
| - src | |
| - tests | |
| excludePaths: | |
| analyse: | |
| - build/ghpages/vendor |
There was a problem hiding this comment.
Applied your suggestion as-is. The only additions are the (?) marker on build/ghpages/vendor, since that directory only exists after a docs build, and phpstan-bootstrap.php as a first bootstrap file to silence the PSR-0 deprecation notice during analysis (the commit message has the details).
| * `$response` is either set to a \WpOrg\Requests\Response instance, or a \WpOrg\Requests\Exception object | ||
| * | ||
| * @param string $response Full response text including headers and body (will be overwritten with Response instance) | ||
| * @param string|\WpOrg\Requests\Response|\WpOrg\Requests\Exception $response Full response text including headers and body (will be overwritten with a Response instance, or with the Exception thrown while parsing) |
There was a problem hiding this comment.
AFAICS, this is wrong. The input should always be the string response, never an exception.
There was a problem hiding this comment.
You're right. I kept @param string and documented the by-reference overwrite with @param-out instead.
Addresses the review feedback on the initial setup: - Remove phpstan-baseline.neon. Every remaining finding is either fixed (following commits) or listed under `ignoreErrors` in phpstan.neon.dist with an explanation of why it is ignored. - Analyse build/ghpages, examples, library, src and tests with tests/bootstrap.php as a bootstrap file, excluding build/ghpages/vendor. - Exclude src/Iri.php from analysis: it is an externally maintained file, as its class docblock notes. The earlier docblock and comparison changes to it are reverted. - Remove the `composer phpstan` script: PHPStan is not installed via Composer, so the script would only fail. CONTRIBUTING.md now documents downloading the PHAR and running it locally instead. - Run `phpstan analyse` directly in the CI job. - Add phpstan.neon.dist to the .gitattributes export-ignore list and the phpstan.neon local override to .gitignore. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Transport\Curl: pass bool and int values to curl_setopt() for options which expect them (CURLOPT_RETURNTRANSFER, CURLOPT_SSL_VERIFYPEER, and the timeout options, where ceil()/round() return float). - Response: drop the array defaults on $headers and $cookies; the constructor always assigns a Headers and a Jar object, which is what the docblocks declare. - CaseInsensitiveDictionary::offsetSet(): the value is mixed, not string (Cookie stores boolean flag attributes in it). - Cookie: declare @phpstan-consistent-constructor, the contract that `new static()` in Cookie::parse() relies on. - StatusUnknown: the constructor casts the code to int, so document it as int. - IdnaEncoder: @internal takes no parenthesised value; move the note to the description. - Requests::parse_multiple(): document the by-reference overwrite with @param-out instead of widening the input type; the input is always the response string. - library/Requests.php: @var requires a variable name; the constant is described without the tag. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Document the lazily initialised static resource properties in TypeProviderHelper and IsCurlHandleTest as nullable, which is what the isset() guards rely on. - FsockopenTest: query the current locale with the documented '0' string argument. - BaseTestCase: remove the `return` statements after markTestSkipped(), which always throws. - StatusCodeTest: fix a `$` missing from a @PARAM tag. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
30562bd to
377aa34
Compare
…imeouts The Composer autoloader registers the autoloader for the deprecated PSR-0 `Requests_*` class names. When PHPStan reflects on those names, which the autoloader tests reference, it triggers an E_USER_DEPRECATED notice that PHPStan reports as an internal error, so the CI job exited non-zero on a fresh result cache. A PHPStan-only bootstrap file now defines REQUESTS_SILENCE_PSR0_DEPRECATIONS, the documented way to silence that notice, before tests/bootstrap.php runs. Also add a transport test with fractional `timeout` and `connect_timeout` values, which exercises the CURLOPT_TIMEOUT_MS and CURLOPT_CONNECTTIMEOUT_MS branches touched in the previous commit and which had no coverage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the feedback on the PR. Appreciate the time that you took. I've addressed your feedback and attemped to clean up a few things. |
This branch has not been deployed
2 participants
Pull Request Type
This is a:
Context
Requests currently has parse-error linting, PHPCS with WPCS and PHPCompatibility, and PHPUnit, but no static analysis. This adds PHPStan so type-level regressions surface in CI.
Detailed Description
Following @jrfnl's review, the second round (last three commits) replaces the initial approach:
build/ghpages,examples,library,srcandtests(withtests/bootstrap.phpas a bootstrap file) is either fixed or listed underignoreErrorsinphpstan.neon.distwith an explanation. The ignore groups are: cURL resource handles on PHP < 8.0 (PHPStan analyses against PHP 8 stubs); constants defined at runtime that PHPStan resolves as fixed (REQUESTS_SILENCE_PSR0_DEPRECATIONS,REQUESTS_TEST_SERVER_*_AVAILABLE,OPENSSL_TLSEXT_SERVER_NAME); thespl_autoload_register()stub declaring a void callback; the PHPUnitsetMethods()/addMethods()version shim; tests that deliberately pass invalid input; tests of magic property access; assertions on analysis-time constants; and two tests disabled at the top withmarkTestSkipped()(Investigate failing test for revoked certificate #966, testSNISupport no longer verifies SNI #1077). None of these are PHPStan bug reports; each entry says why it is ignored.reportUnmatchedIgnoredErrorsstays on, so a stale entry fails the run.src/Iri.phpis excluded from analysis as the externally maintained file, and the earlier changes to it are reverted.src/:curl_setopt()values typed as the option expects (true/falsefor booleans,(int)for the ceil/round timeouts);Response::$headers/$cookiesno longer carry array defaults the constructor immediately replaces;CaseInsensitiveDictionary::offsetSet()documents its value asmixed;Cookiedeclares@phpstan-consistent-constructorfornew static();StatusUnknown::$codeisint(the constructor casts); the@internaltag inIdnaEncodertakes no value;Requests::parse_multiple()uses@param-outfor the by-reference overwrite instead of widening the input type; a stray@varinlibrary/Requests.php.tests/: nullable docblocks on the lazily initialised static resource properties;setlocale(LC_NUMERIC, '0'); removedreturnaftermarkTestSkipped(); a@paramtypo.composer installon 5.6 and 7.0). CONTRIBUTING.md now documents downloading the PHAR and running it locally.phpstanjob incs.ymlrunsphpstan analyse --error-format=github, with PHPStan from setup-php'stools:; Composer dependencies are still installed for the autoloader.phpstan-bootstrap.phpdefinesREQUESTS_SILENCE_PSR0_DEPRECATIONSfor the analysis run only: the Composer autoloader registers the deprecated PSR-0 autoloader, and reflecting on theRequests_*names inAutoloadTestotherwise triggers itsE_USER_DEPRECATEDnotice, which PHPStan reports as an internal error (that was the first CI failure of round 2).timeout/connect_timeoutvalues now exercises theCURLOPT_*_MSbranches thecurl_setopt()fix touched, which had no coverage before (codecov patch check).phpstan.neon.distis export-ignored;phpstan.neon(local override) is git-ignored.Level 5 was kept: on
develop, level 6 adds ~190 missing-type findings that would need a typing effort rather than fixes.One thing surfaced by the analysis that I left alone:
Requests::set_defaults()documentscookies => falseas accepted, but theempty()check replacesfalsewith an emptyJar, so the later!== falseguard can never seefalse. Whetherfalseshould disable cookie handling is a separate question; the finding is ignored with that note.Quality assurance
Tests executed (PHP 8.5.10, PHPStan 2.2.16 PHAR):
phpstan analysereports no errors, including with a cleared result cache anderror_reporting=-1;phpcsandparallel-lintclean on all changed PHP files; the full PHPUnit suite viaphpunit10.xml.disthas the same 72 failing tests before and after the change, all in the test-server-backedCurlTest,FsockopenTest,HttpTest,BasicTest,SessionTest,RequestMultipleTestandCookieTestclasses, because the test server was not running locally for that run. The newtestFloatTimeoutOptionstest was run against the bundled test server locally on both the cURL and fsockopen transports and passes.AI disclosure
AI assistance: Yes. Tool(s): Claude Code. Used for: the initial PHPStan level sweep, drafting the configuration, the code and docblock changes, the ignore list and its explanations, the CI job, and this description.