Skip to content

Ugrade phpstan to ^1.3 #531

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 4 commits into from
Jan 9, 2022
Merged

Conversation

alamirault
Copy link
Contributor

Phpstan 1.0.0 was released in november.

This PR use last version !

We may also add @deprecated annotation on getUsername methods . It's deprecated in UserInterface since sf 5.3 in favor of getUserIdentifier()

@alamirault alamirault marked this pull request as draft December 31, 2021 12:46
@alamirault alamirault marked this pull request as ready for review December 31, 2021 13:00
@alamirault alamirault changed the title [Draft] Ugrade phpstan to ^1.3 Ugrade phpstan to ^1.3 Dec 31, 2021
@alamirault
Copy link
Contributor Author

With no use of deprectaed getUsername method coverage is now bellow 99%
Code coverage is 98.87%, which is below the accepted 99%

Should I add an unit test on getUsername ?

@akondas
Copy link
Member

akondas commented Jan 6, 2022

Should I add an unit test on getUsername ?

Yes please (in fact, this coverage is mainly for checking open source PR) ... or you can also lower it to 98% 🤷

@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #531 (fb8b9cd) into master (3e26961) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #531      +/-   ##
============================================
- Coverage     99.33%   99.21%   -0.12%     
  Complexity     1893     1893              
============================================
  Files           301      301              
  Lines          6013     6014       +1     
============================================
- Hits           5973     5967       -6     
- Misses           40       47       +7     
Impacted Files Coverage Δ
src/Command/ProxySyncReleasesCommand.php 100.00% <100.00%> (ø)
src/Repository/UserRepository.php 100.00% <100.00%> (ø)
src/Security/Model/Organization.php 100.00% <100.00%> (ø)
src/Security/Model/User.php 100.00% <100.00%> (ø)
src/Security/OAuthAuthenticator.php 100.00% <100.00%> (ø)
src/Service/Twig/DateExtension.php 97.82% <100.00%> (ø)
src/Security/OrganizationProvider.php 84.61% <0.00%> (-12.83%) ⬇️
src/Security/UserProvider.php 94.59% <0.00%> (-5.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e26961...fb8b9cd. Read the comment docs.

@akondas akondas merged commit 00e207e into repman-io:master Jan 9, 2022
@akondas
Copy link
Member

akondas commented Jan 9, 2022

Big thanks @alamirault 🍻

@@ -53,7 +53,7 @@ public function getUsername()

public function getUserIdentifier(): string
{
return $this->getUsername();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make this change? I suppose we return the Organization alias here now, instead of the userIdentifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getUsername is deprecated in symfony, phpstan extension for symfony now report it. In fact, getUsername return also alias so it's same

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.

3 participants