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

Add phpstan phpunit strict-rules #203

Merged
merged 10 commits into from
Jun 27, 2023

Conversation

phil-davis
Copy link
Contributor

No description provided.

@phil-davis phil-davis changed the title Phpstan phpunit Add phpstan phpunit strict-rules Feb 11, 2023
@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #203 (dbc86e4) into master (fcc83fa) will decrease coverage by 0.19%.
The diff coverage is 86.66%.

@@             Coverage Diff              @@
##             master     #203      +/-   ##
============================================
- Coverage     94.40%   94.21%   -0.19%     
- Complexity      258      260       +2     
============================================
  Files            15       15              
  Lines           875      881       +6     
============================================
+ Hits            826      830       +4     
- Misses           49       51       +2     
Impacted Files Coverage Δ
lib/Response.php 100.00% <ø> (ø)
lib/Client.php 84.16% <75.00%> (-0.32%) ⬇️
lib/Auth/Basic.php 93.75% <80.00%> (-6.25%) ⬇️
lib/Auth/AWS.php 89.61% <100.00%> (+0.13%) ⬆️
lib/Auth/Bearer.php 100.00% <100.00%> (ø)
lib/Auth/Digest.php 96.42% <100.00%> (ø)
lib/Request.php 100.00% <100.00%> (ø)
lib/Sapi.php 98.13% <100.00%> (ø)
lib/functions.php 97.90% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@phil-davis
Copy link
Contributor Author

phil-davis commented Feb 11, 2023

Error: Call to function base64_decode() requires parameter #2 to be set.
Error: Call to function in_array() requires parameter #3 to be set.
Error: Call to function array_key_exists() with 'size' and array{0: int, 1: int, 2: int, 3: int, 4: int, 5: int, 6: int, 7: int, ...} will always evaluate to true.
 ------ ------------------------------------------------------------------- 
  Line   lib/Auth/Basic.php                                                 
 ------ ------------------------------------------------------------------- 
  42     Call to function base64_decode() requires parameter #2 to be set.  
 ------ ------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------- 
  Line   lib/Client.php                                                         
 ------ ----------------------------------------------------------------------- 
  125    Call to function in_array() requires parameter #3 to be set.           
  390    Call to function array_key_exists() with 'size' and array{0: int, 1:   
         int, 2: int, 3: int, 4: int, 5: int, 6: int, 7: int, ...} will always evaluate to true.                                                      
 ------ ----------------------------------------------------------------------- 

Error:  [ERROR] Found 3 errors

Note: the new phpstan-strict-rules enforce that base64_decode and in_array are always called with the "strict" parameter set to true. I have adjusted the code to do that.

I have ignored the "will always evaluate to true" message. The check is a bit superfluous, but it does not hurt to keep it.

@phil-davis phil-davis marked this pull request as ready for review February 11, 2023 10:17
@@ -239,7 +239,7 @@ public function poll(): bool

$curlResult['request'] = $request;

if ($errorCallback) {
if (is_callable($errorCallback)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we instead declar wthe type of the propery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstan-strict-rules wants all if checks to be real booleans.
We can only call $errorCallback if it is a callback anyway.

sendAsyncInternal already has this passed in as a callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Declaring the type as callable will not stop the strict rule complaining that the if condition is not a real boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added details of the "array shape" of curlMultiMap (see below in the diffs).

@phil-davis phil-davis requested a review from staabm February 11, 2023 11:16
@phil-davis phil-davis self-assigned this Apr 12, 2023
@@ -353,7 +354,7 @@ protected function doRequest(RequestInterface $request): ResponseInterface
* Has a list of curl handles, as well as their associated success and
* error callbacks.
*
* @var array<int, mixed>
* @var array<int, array{0: RequestInterface, 1: callable, 2: callable, 3: int}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This at least helps to document what this array-of-arrays should look like.
But I locally tried specifying wrong things here, and I couldn't get phpstan to give any errors - I was expecting that lines of code that set and/or use entries in this array would cause phpstan to complain.

@phil-davis
Copy link
Contributor Author

@staabm I pushed another commit:
dbc86e4

That moves the declaration of the "shape" of $curlMultiMap to the declaration of the property.

Does that seem OK/better to you?

@staabm
Copy link
Member

staabm commented Jun 27, 2023

looks good, thanks.

@phil-davis phil-davis merged commit 5220b2b into sabre-io:master Jun 27, 2023
@phil-davis phil-davis deleted the phpstan-phpunit branch June 27, 2023 10:33
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