Cleaning code and typehinting#5064
Merged
Alkarex merged 24 commits intoFreshRSS:edgefrom Mar 26, 2023
ColonelMoutarde:feature/cleaning-code
Merged
Cleaning code and typehinting#5064Alkarex merged 24 commits intoFreshRSS:edgefrom ColonelMoutarde:feature/cleaning-code
Alkarex merged 24 commits intoFreshRSS:edgefrom
ColonelMoutarde:feature/cleaning-code
Conversation
ColonelMoutarde
commented
Feb 2, 2023
Alkarex
reviewed
Feb 2, 2023
Alkarex
reviewed
Feb 2, 2023
Member
|
Please remember to indicate PHPStan status before/after the changes |
Contributor
Author
|
My code pass phpstan level 6 |
added 2 commits
February 2, 2023 13:09
Alkarex
reviewed
Feb 3, 2023
Alkarex
reviewed
Feb 3, 2023
Alkarex
reviewed
Feb 3, 2023
Member
It does not look like it is passing PHPStan level 6 (cf. #4112 ): $ vendor/bin/phpstan analyse --level 6 app/Models/Context.php
Note: Using configuration file /home/alex/GitHub/FreshRSS/phpstan.neon.
1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
------ ------------------------------------------------------------------------------------------
Line Context.php
------ ------------------------------------------------------------------------------------------
:19 Property FreshRSS_Context::$categories has no type specified.
:20 Property FreshRSS_Context::$tags has no type specified.
:22 Property FreshRSS_Context::$name has no type specified.
:23 Property FreshRSS_Context::$description has no type specified.
:25 Property FreshRSS_Context::$total_unread has no type specified.
:26 Property FreshRSS_Context::$total_starred has no type specified.
:32 Property FreshRSS_Context::$get_unread has no type specified.
:33 Property FreshRSS_Context::$current_get has no type specified.
:41 Property FreshRSS_Context::$next_get has no type specified.
:43 Property FreshRSS_Context::$state has no type specified.
:44 Property FreshRSS_Context::$order has no type specified.
:45 Property FreshRSS_Context::$number has no type specified.
:48 Property FreshRSS_Context::$first_id has no type specified.
:49 Property FreshRSS_Context::$next_id has no type specified.
:50 Property FreshRSS_Context::$id_max has no type specified.
:51 Property FreshRSS_Context::$sinceHours has no type specified.
:53 Property FreshRSS_Context::$isCli has no type specified.
:58 Method FreshRSS_Context::initSystem() has parameter $reload with no type specified.
:73 Method FreshRSS_Context::initUser() has no return type specified.
:73 Method FreshRSS_Context::initUser() has parameter $userMustExist with no type specified.
:73 Method FreshRSS_Context::initUser() has parameter $username with no type specified.
:196 Method FreshRSS_Context::isStateEnabled() has no return type specified.
:204 Method FreshRSS_Context::getRevertState() has no return type specified.
:217 Method FreshRSS_Context::currentGet() has no return type specified.
:217 Method FreshRSS_Context::currentGet() has parameter $array with no type specified.
:278 Method FreshRSS_Context::isCurrentGet() has parameter $get with no type specified.
:314 Method FreshRSS_Context::_get() has parameter $get with no type specified.
------ ------------------------------------------------------------------------------------------
[ERROR] Found 27 errors |
Alkarex
requested changes
Feb 3, 2023
Member
Alkarex
left a comment
There was a problem hiding this comment.
Please pass at least PHPStan level 6, and more if possible
aledeg
reviewed
Feb 8, 2023
Member
|
Still not passing PHPStan level 6, right? |
Contributor
Author
Pass phpstan6, but not 9 |
Member
|
Nope, still not level 6. Did you try the command from my comment above? $ vendor/bin/phpstan analyse --level 6 app/Models/Context.php
Note: Using configuration file /home/alexandre/GitHub/FreshRSS/phpstan.neon.
1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
------ -------------------------------------------------------------------------------------------------------
Line Context.php
------ -------------------------------------------------------------------------------------------------------
:19 Property FreshRSS_Context::$categories has no type specified.
:20 Property FreshRSS_Context::$tags has no type specified.
:22 Property FreshRSS_Context::$name has no type specified.
:23 Property FreshRSS_Context::$description has no type specified.
:25 Property FreshRSS_Context::$total_unread has no type specified.
:26 Property FreshRSS_Context::$total_starred has no type specified.
:32 Property FreshRSS_Context::$get_unread has no type specified.
:33 Property FreshRSS_Context::$current_get has no type specified.
:41 Property FreshRSS_Context::$next_get has no type specified.
:43 Property FreshRSS_Context::$state has no type specified.
:44 Property FreshRSS_Context::$order has no type specified.
:45 Property FreshRSS_Context::$number has no type specified.
:48 Property FreshRSS_Context::$first_id has no type specified.
:49 Property FreshRSS_Context::$next_id has no type specified.
:50 Property FreshRSS_Context::$id_max has no type specified.
:51 Property FreshRSS_Context::$sinceHours has no type specified.
:53 Property FreshRSS_Context::$isCli has no type specified.
:58 Method FreshRSS_Context::initSystem() has parameter $reload with no type specified.
:76 Method FreshRSS_Context::initUser() has parameter $userMustExist with no type specified.
:219 Method FreshRSS_Context::currentGet() return type has no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:278 Method FreshRSS_Context::isCurrentGet() has parameter $get with no type specified.
:314 Method FreshRSS_Context::_get() has parameter $get with no type specified.
------ -------------------------------------------------------------------------------------------------------
[ERROR] Found 22 errors |
Contributor
Author
Fixed for level 6 |
Alkarex
reviewed
Mar 18, 2023
| 'order', self::$user_conf->sort_order | ||
| ); | ||
| self::$number = intval(Minz_Request::param('nb', self::$user_conf->posts_per_page)); | ||
| self::$number = (int)Minz_Request::param('nb', self::$user_conf->posts_per_page); |
Member
There was a problem hiding this comment.
In PHPStan level 9, such changes introduce a new error: Cannot cast mixed to int.
Cf. phpstan/phpstan#4490 (comment)
Member
There was a problem hiding this comment.
Although intval() and (int) might be doing exactly the same thing (I have not checked), there are semantically not exactly equivalent
Alkarex
reviewed
Mar 18, 2023
app/Controllers/indexController.php
Outdated
| case 'f': | ||
| // We most likely already have the feed object in cache | ||
| $feed = FreshRSS_CategoryDAO::findFeed($categories, $id); | ||
| $feed = FreshRSS_CategoryDAO::findFeed($categories, (int)$id); |
Alkarex
reviewed
Mar 18, 2023
app/Models/Context.php
Outdated
|
|
||
| /** | ||
| * @return bool true if the current request targets all feeds (main view), false otherwise. | ||
| * true if the current request targets all feeds (main view), false otherwise. |
Member
There was a problem hiding this comment.
Removing the @return instruction breaks the understanding of the PHPDocs.
Same comment than #5106 (comment)
Alkarex
reviewed
Mar 18, 2023
| "ext-gmp": "*", | ||
| "ext-intl": "*", | ||
| "ext-json": "*", | ||
| "ext-libxml": "*", |
Member
There was a problem hiding this comment.
Although this is fine, I would have preferred another PR, as it is not related to the topic of the current PR.
No need to revert, though, but for another time.
Alkarex
reviewed
Mar 26, 2023
app/Models/Context.php
Outdated
| * @return string|array{string,bool|int} | ||
| */ | ||
| public static function currentGet($array = false) { | ||
| public static function currentGet(?bool $bool = false) { |
Member
There was a problem hiding this comment.
Why renaming this variable? It was more informative before
Alkarex
reviewed
Mar 26, 2023
app/Models/Context.php
Outdated
| */ | ||
| public static $total_unread = 0; | ||
| /** | ||
| * @var array<string, int> |
Member
There was a problem hiding this comment.
Please use tuples syntax instead. Fix coming
Alkarex
reviewed
Mar 26, 2023
app/Models/Context.php
Outdated
| */ | ||
| public static $get_unread = 0; | ||
| /** | ||
| * @var array<string, bool> |
Member
There was a problem hiding this comment.
Wrong type, the values can also be integers. Fix coming
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
3 participants
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.
Changes proposed in this pull request:
Pull request checklist: