Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DX: update PHPStan #5901

Merged
merged 1 commit into from
Aug 31, 2021
Merged

DX: update PHPStan #5901

merged 1 commit into from
Aug 31, 2021

Conversation

kubawerlos
Copy link
Contributor

Added ignoreErrors are new things PHPStan now recognises and there is no obvious (at least for me) fixes, so let's update PHPStan itself to allow strict rules in: #5900 and later try to fix the errors.

@coveralls
Copy link

coveralls commented Aug 27, 2021

Coverage Status

Coverage remained the same at 92.311% when pulling 3b4d54b on kubawerlos:update_phpstan into 306713a on FriendsOfPHP:master.

phpstan.neon Outdated
@@ -10,4 +10,8 @@ parameters:
- tests
excludePaths:
- tests/Fixtures
ignoreErrors:
- '/^Class [a-zA-Z\\]+ extends @final class [a-zA-Z\\]+\.$/'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those errors ignored because they can't be fixed for some reason? Or do you just want to postpone fixing them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit of both :)

  • @final is because we have 2 classes with annotation @final Only internal extending this class is supported - it can be removed, but should it?
  • Unsafe call to private method is because we have traits defining private methods that are then called via static:: - this can be easily changed to self (but some of them are assertions which we call via static) or method to protected (but there is no need in fact to extend visibility?)
  • last one is because of Tokens class iterating on it's elements and PHPStan "thinks" it can be null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Final is because we have 2 classes with annotation @Final Only internal extending this class is supported - it can be removed, but should it?

We can keep it, but maybe the regular expression should target those classes specifically?

Unsafe call to private method is because we have traits defining private methods that are then called via static:: - this can be easily changed to self (but some of them are assertions which we call via static) or method to protected (but there is no need in fact to extend visibility?)

I don't see how we can fix those indeed.

last one is because of Tokens class iterating on it's elements and PHPStan "thinks" it can be null

Isn't this one legit because of how ArrayAccess work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe related: #4970.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep it, but maybe the regular expression should target those classes specifically?

Done

Isn't this one legit because of how ArrayAccess work?

On the 2nd look it claims that in line $this[$key] = clone $val; the problem is that:

$this(PhpCsFixer\Tokenizer\Tokens) does not accept PhpCsFixer\Tokenizer\Token|null.

which does not make much sense.

I'd go with merging this with exceptions and discuss how to handle it (may be even bug in PHPStan).

@keradus keradus changed the base branch from 3.0 to master August 29, 2021 19:08
@kubawerlos kubawerlos added the RTM Ready To Merge label Aug 30, 2021
@kubawerlos kubawerlos added this to the 3.1.1 milestone Aug 30, 2021
@SpacePossum
Copy link
Contributor

Thank you @kubawerlos.

@SpacePossum SpacePossum merged commit a0f58fa into PHP-CS-Fixer:master Aug 31, 2021
@SpacePossum SpacePossum removed the RTM Ready To Merge label Aug 31, 2021
@kubawerlos kubawerlos deleted the update_phpstan branch August 31, 2021 07:20
@keradus keradus removed this from the 3.1.1 milestone Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants