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 phpstan-strict-rules to phpstan analysis #90

Merged
merged 12 commits into from
May 10, 2023

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Feb 13, 2023

see my comments - fixes a few edge-cases when a URI component might be the string "0" (path, username, query, fragment...). These are not really common in everyday use!

Gets the strict phpstan rules passing.

@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #90 (5ff60c8) into master (85d9f7c) will decrease coverage by 1.09%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   98.80%   97.72%   -1.09%     
==========================================
  Files           1        1              
  Lines         168      176       +8     
==========================================
+ Hits          166      172       +6     
- Misses          2        4       +2     
Impacted Files Coverage Δ
lib/functions.php 97.72% <94.87%> (-1.09%) ⬇️

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

@phil-davis
Copy link
Contributor Author

------ ---------------------------------------------------------------------- 
  Line   lib/functions.php                                                     
 ------ ---------------------------------------------------------------------- 
  65     Short ternary operator is not allowed. Use null coalesce operator if  
         applicable or consider using long ternary.                            
  90     Construct empty() is not allowed. Use more strict comparison.         
  90     Construct empty() is not allowed. Use more strict comparison.         
  90     Construct empty() is not allowed. Use more strict comparison.         
  116    Construct empty() is not allowed. Use more strict comparison.         
  144    Construct empty() is not allowed. Use more strict comparison.         
  152    Construct empty() is not allowed. Use more strict comparison.         
  254    Construct empty() is not allowed. Use more strict comparison.         
  256    Construct empty() is not allowed. Use more strict comparison.         
  259    Construct empty() is not allowed. Use more strict comparison.         
  264    Construct empty() is not allowed. Use more strict comparison.         
  268    Construct empty() is not allowed. Use more strict comparison.         
  273    Construct empty() is not allowed. Use more strict comparison.         
  276    Construct empty() is not allowed. Use more strict comparison.         
  279    Construct empty() is not allowed. Use more strict comparison.         
 ------ ---------------------------------------------------------------------- 

I will have a go at refactoring these. But it often seems a bit painful to remove use of empty() - need to check if something is null, the empty string etc.

@staabm
Copy link
Member

staabm commented Feb 13, 2023

if you don't see value in changing existing code, or don't have time todo it now, feel free to create a baseline for them.

@phil-davis phil-davis self-assigned this Apr 12, 2023
$authority = $parts['host'];
if (!empty($parts['user'])) {
if (isset($parts['user'])) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes from !empty() to isset() now allow the value to be "0".
I added unit test cases for ways that I could see that the value of these things might be "reasonably" passed in as the value "0". For example, a user "name" might be the string "0"! Or the query string could be just ?0 (very edge case - an application that just sends a number in the query string). Or the fragment could be #0. All of those unit test cases fail with the !empty() check, and pass with isset()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we need to use isset() because the $parts array passed to build() may not have all the component keys set.

In other places we could compare directly to null because resolve(), normalize() etc. we guarantee that all the URI components are actually set in the array (and "missing" components are set to value null

@phil-davis phil-davis marked this pull request as ready for review April 13, 2023 07:03
@phil-davis
Copy link
Contributor Author

@staabm please review.

lib/functions.php Outdated Show resolved Hide resolved
lib/functions.php Outdated Show resolved Hide resolved
Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

lgtm - except a few nits

thank you

@phil-davis
Copy link
Contributor Author

I broke a heap of unit tests - I will look...

lib/functions.php Outdated Show resolved Hide resolved
Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

lgtm

@phil-davis phil-davis merged commit 52f3fbf into sabre-io:master May 10, 2023
@phil-davis phil-davis deleted the phpstan-phpunit branch May 10, 2023 13:59
@phil-davis phil-davis mentioned this pull request Jun 9, 2023
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