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

Adjust type declarations a little bit #194

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

phil-davis
Copy link
Contributor

  1. digestParts should be a bit more flexible

  2. Response.php setStatus - be more careful to set the variable to int - specially in the case when the passed-in value is ctype_digit (e.g. "200" "402" ...)

I tried phpstan level 7 and noticed these things that looked like they can and should be fixed/adjusted. Level 7 also reports a lot of stuff about PHP functions that can return "something or false" like string|false - there are quite a few places where the (remote) possibility of false is not checked. We put the return value straight into string (or whatever). In most of those cases, there is no good explanation for when false can happen - "the internet broke", "the operating system fell to pieces"... So I am not planning to analyze each one and work out some path to handle the occurrence of false. Those things could happen already with the code, they are not regressions, and if/when they happen, some unexpected code path is being taken until, presumably, something falls over. Now they are likely to fall over earlier, because PHP will complain if the code tries to assign false to some string $var - and the PHP error and traceback will point more directly the the point in the code where the problem first happened.

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #194 (e95ea94) into master (1c2ac9f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #194   +/-   ##
=========================================
  Coverage     94.78%   94.78%           
  Complexity      256      256           
=========================================
  Files            15       15           
  Lines           806      806           
=========================================
  Hits            764      764           
  Misses           42       42           
Impacted Files Coverage Δ
lib/Auth/Digest.php 96.42% <ø> (ø)
lib/Response.php 100.00% <100.00%> (ø)

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

@phil-davis phil-davis requested a review from staabm August 30, 2022 07:20
@phil-davis
Copy link
Contributor Author

@staabm please review. I think this gets the type declarations and code OK for me to do a major version release of sabre/http

Then I can look at the other repos and keep moving forward with those.

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 390c7be into sabre-io:master Aug 31, 2022
@phil-davis phil-davis deleted the more-type-declarations branch August 31, 2022 07: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