Skip to content

Add PHPStan level 5 #304

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

Merged
merged 13 commits into from
Apr 25, 2025
Merged

Add PHPStan level 5 #304

merged 13 commits into from
Apr 25, 2025

Conversation

devnix
Copy link
Contributor

@devnix devnix commented Apr 19, 2025

Hi @oscarotero, I propose to add PHPStan slowly to improve the type coverage.

It may trigger some architectural questions, for example we are getting this error at level 0:

 ------ ----------------------------------------------------------------------------------- 
  Line   src/Flags.php                                                                      
 ------ ----------------------------------------------------------------------------------- 
  21     Unsafe usage of new static().                                                      
         🪪  new.static                                                                      
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static   
 ------ ----------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------- 
  Line   src/Headers.php                                                                    
 ------ ----------------------------------------------------------------------------------- 
  26     Unsafe usage of new static().                                                      
         🪪  new.static                                                                      
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static   
 ------ ----------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------- 
  Line   src/References.php                                                                 
 ------ ----------------------------------------------------------------------------------- 
  21     Unsafe usage of new static().                                                      
         🪪  new.static                                                                      
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static   
 ------ ----------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------- 
  Line   src/Translation.php                                                                
 ------ ----------------------------------------------------------------------------------- 
  30     Unsafe usage of new static().                                                      
         🪪  new.static                                                                      
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static   
 ------ ----------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------- 
  Line   src/Translations.php                                                               
 ------ ----------------------------------------------------------------------------------- 
  25     Unsafe usage of new static().                                                      
         🪪  new.static                                                                      
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static   
 ------ -----------------------------------------------------------------------------------

It's not a big deal to fix, but making the class or the constructor final would be a breaking change, and I have no idea if it follows how you want to keep evolving this library.

@devnix
Copy link
Contributor Author

devnix commented Apr 19, 2025

Of course, @phpstan-consistent-constructor could be used, but making those classes final doesn't sound stupid to me

@oscarotero
Copy link
Member

Hi @devnix
Thanks for this! I really appreciate your work.

I don't want to introduce breaking changes so @phpstan-consistent-constructor sounds good to me.
Let me know when it's ready to merge and I'll review it.

@devnix
Copy link
Contributor Author

devnix commented Apr 20, 2025

Thank you! I'm goint to iterate a little bit and I'll ask you a couple of specific questions here and there around the code 😄

@devnix
Copy link
Contributor Author

devnix commented Apr 20, 2025

@oscarotero, is the src/Scanner/FunctionsHandlersTrait.php still useful? PHPStan detects it's not used in any class or tested at all, so I'd like to know if it's an untested utility for the library consumer or a leftover.

@devnix devnix changed the title Add PHPStan level 0 Add PHPStan level 5 Apr 20, 2025
@devnix
Copy link
Contributor Author

devnix commented Apr 20, 2025

Also, I'm wondering if you would be OK with adding Rector in another pull request so it can help reach PHPStan level 6 faster, applying each ruleset in granular pull requests.

@devnix devnix marked this pull request as ready for review April 20, 2025 11:58
@oscarotero
Copy link
Member

is the src/Scanner/FunctionsHandlersTrait.php still useful? PHPStan detects it's not used in any class or tested at all, so I'd like to know if it's an untested utility for the library consumer or a leftover.

It's used by other packages, like PHP-Scanner.

@oscarotero
Copy link
Member

Also, I'm wondering if you would be OK with adding Rector in another pull request so it can help reach PHPStan level 6 faster, applying each ruleset in granular pull requests.

I'm not familiarized with Rector but if it helps to improve the code quality, it's okay, as long as it keeps compatibility with PHP 7.2 and newer

@devnix
Copy link
Contributor Author

devnix commented Apr 20, 2025

I'm not familiarized with Rector but if it helps to improve the code quality, it's okay, as long as it keeps compatibility with PHP 7.2 and newer

Excellent!

The pull request should be ready to pass the workflows and be reviewed. Thank you!

@devnix
Copy link
Contributor Author

devnix commented Apr 20, 2025

It looks like PHPStan will not be installed in versions lower than 7.4. It should not pose a problem since it's just a devDependency.

I guess we could invoke PHPStan from Docker and remove the dependency

@oscarotero
Copy link
Member

Can we use 1.x for these versions? I mean ^1 | ^2.

@devnix
Copy link
Contributor Author

devnix commented Apr 22, 2025

I'm unsure about the differences we will find by running it on different major PHP versions, which can be an interesting check. Can we check the CI again?

@devnix
Copy link
Contributor Author

devnix commented Apr 22, 2025

I think there is a conflict between the versions shipped into my machine of php-cs-fixer and phpcs, as there is no composer.lock for the project currently:

image


image

Also, I'm aware that it's well-known that running php-cs-fixer with different PHP versions can bring very different behaviors, so I might have a couple of suggestions that I can address here or in another pull request if you want:

  • Persisting a composer.lock in the repository to have predictable versions of the development packages
  • Run style checkers together, as well as running both fixers in the same fix command too
  • Run style checkers only on the lowest supported version maybe?
  • I think I would worry more about running the test matrix only for PHPUnit, and have separate workflows for PHPStan and the coding style

@devnix
Copy link
Contributor Author

devnix commented Apr 22, 2025

@devnix devnix requested a review from oscarotero April 24, 2025 21:13
@oscarotero oscarotero merged commit 1f56798 into php-gettext:master Apr 25, 2025
8 checks passed
@oscarotero
Copy link
Member

Great work! Thanks you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants