feat: improve phpstan level 9 with all errors resolved #7248
Conversation
Assert and cast mixed parameter based on option name: charset/collation: ensure null or non-empty string default_constraint_name: assert non-empty string enumType: assert string with class-string PHPDoc jsonb/version: cast to bool Satisfies PHPStan level 9 strict type checking for array
|
@shakaran before we proceed, could you share a good strawberry pie recipe with us? |
why is this relevant? I am fixing this thing with phpstan this step by step. I am a programmer, not a chef. Do you make this like a captcha or so? xD pretty funny |
|
Yes, I wanted to make sure you’re a human. You’re making random changes in the codebase making it obviously worse. |
Human here, I am not a 100% expert, but trying to learn at most. So please, I would like to know which is wrong, I will spending my free saturday night in fix 550 errors in repetitive batches of similar errors. I am commiting per small batch of fixes, but any help is welcome in the peer review |
…variables in Platforms
|
@shakaran what are you trying to learn? |
Taking big and complex open source projects with complex params or types, and raise steps of phpstan levels or strong typing to improve in closed private and legacy projects that need the same. For example, I am applying intermediate variables so phpstan get the type infered, with that the error is removed, but for you probably that is worse in the codebase. I would like to know if there are some other options (rather than ignore the error). Other cases for example doing a casting in the same variable and assignement, for example $a = (string) $a; Or even: Or doing If this is wrong, I would like to know how to resolve it in future. That will teach me how to do it in the right way |
…racleMetadataProvider
shakaran
changed the title
|
The current PHPStan level reflects the deficiencies of some DBAL APIs that were designed ~15 years ago for PHP 5. In turn, those to a certain degree are inspired by the APIs of the underlying database drivers and the PHP language itself. Attempting to satisfy PHPStan by adding random conditions in random places without improving the APIs looks like a cargo cult to me. See #4007 for an a example of a proper rework. It doesn't chase a higher PHPStan level explicitly, but it does improve the type safety which PHPStan appreciates. |
I think that improve database drivers legacy from PHP 5 it is a bit out scope of this PR and probably require a lot knowledge about that specific drivers. If you detail the specific task or drivers to change for example fetch() methods or FetchUtils (I can suspect OCDI or others), I can try to work on it in separate PRs after the merge of this one. As far I test this PR, the phpunit will pass fine, phpstan will pass fine and phpcbf also. So at my side, it seems a clear green. We can go change per change if you want modify or change something specific too or if you dont like some specific way to do something. |
|
This PR is way to big to review and test it properly, tbh. I think, we deliberately stopped at level 8 because the extensions we code against are mostly weakly typed/stubbed. I'm not saying that raising the level is impossible or should not be done. But if we go down that road, we should find better ways than cluttering the codebase with random assertions. And we should do it in smaller steps. |
I can do splitted PR per type of error, or files or so as you desire, would be more acceptable to peer review in that case? Let me know which option do you want and I will start with the small ones, referencing to this one (keep this one opened as reference to link and don't lose the changes). IHMO since all incoming new PHP versions are oriented to strong types and having analysis static helps to detect quickly bugs, I think that it is a good path to follow |
|
Yes, I need smaller and focussed PRs. I leave it up to you to decide how to cut them. And please be aware: Reaching a higher PHPStan level does not mean tossing Let's close this PR for for the reasons given. |
| Q | A |------------- | ----------- | Type | improvement #### Summary Related #7248 This small PR fixes only this 2 warnings in this file: ``` vendor/bin/phpstan analyse --level=9 src/Cache/ArrayResult.php Note: Using configuration file /home/shakaran/Documentos/ProjectsSSD/dbal/phpstan.neon.dist. 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% ------ ---------------------------------------------------------------------------------------------- Line ArrayResult.php ------ ---------------------------------------------------------------------------------------------- 124 Property Doctrine\DBAL\Cache\ArrayResult::$columnNames (list<string>) does not accept mixed. 🪪 assign.propertyType 💡 mixed is not a list. 124 Property Doctrine\DBAL\Cache\ArrayResult::$rows (list<list<mixed>>) does not accept mixed. 🪪 assign.propertyType 💡 mixed is not a list. ------ ---------------------------------------------------------------------------------------------- ``` This pull request makes a minor improvement to the type annotation in the docblock for the `$data` parameter of the `__unserialize` method in `src/Cache/ArrayResult.php`, providing a more precise description of the expected array structure.
3 participants
Summary
Improve phpstan level 9 with all errors resolved