Set min PHP to 7.4 #192

phil-davis · 2022-08-26T05:58:35Z

WIP

ToDo:

declare types in code
declare types in tests
finish releasing the new major version of sabre/event and then use that in composer.json
bump phpstan to level 6

lib/functions.php

composer.json

phil-davis · 2022-08-26T07:06:59Z

lib/HttpException.php

     */
-    public function getHttpStatus();
+    public function getHttpStatus(): ?string;


The PHP doc had string|null as the return type. I preserved that in the explicit declaration of return type ?string

That has flow-on effects - this is saying that the integer 200 is not acceptable to return, it must return the string "200".

Implementations of this, like in ClientHttpException above, already had the return value declared as int

What to do? What is the "requirement"? Do we refactor to make all this allow both string|int to be passed around, or require string or require int ?

phil-davis · 2022-08-26T07:09:06Z

lib/Response.php

     */
-    protected $status;
+    protected int $status;


And this one says that the HTTP status code is an int

phil-davis · 2022-08-26T07:15:24Z

lib/ClientHttpException.php

-    public function getHttpStatus(): int
+    public function getHttpStatus(): string
    {
-        return $this->response->getStatus();
+        return (string) $this->response->getStatus();
    }


See comment below - various things were "mixed" - do we consistently pass HTTP status as string or int ?

codecov · 2022-08-26T07:24:57Z

Codecov Report

Merging #192 (5453449) into master (3000a8a) will increase coverage by 0.67%.
The diff coverage is 98.07%.

@@             Coverage Diff              @@
##             master     #192      +/-   ##
============================================
+ Coverage     94.11%   94.78%   +0.67%     
+ Complexity      260      256       -4     
============================================
  Files            15       15              
  Lines           884      806      -78     
============================================
- Hits            832      764      -68     
+ Misses           52       42      -10

Impacted Files	Coverage Δ
lib/Auth/AbstractAuth.php	`100.00% <ø> (ø)`
lib/Auth/AWS.php	`88.73% <50.00%> (+2.06%)`	⬆️
lib/Auth/Basic.php	`100.00% <100.00%> (ø)`
lib/Auth/Bearer.php	`100.00% <100.00%> (ø)`
lib/Auth/Digest.php	`96.42% <100.00%> (-0.24%)`	⬇️
lib/Client.php	`84.26% <100.00%> (-0.31%)`	⬇️
lib/ClientHttpException.php	`100.00% <100.00%> (ø)`
lib/Message.php	`100.00% <100.00%> (ø)`
lib/MessageDecoratorTrait.php	`100.00% <100.00%> (ø)`
lib/Request.php	`100.00% <100.00%> (ø)`
... and 20 more

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

phil-davis · 2022-08-29T14:59:07Z

There are a lot of things reported when I raise phpstan to level 6 - currently working through it, Down to 87 "errors" remaining!

phil-davis · 2022-08-30T04:56:40Z

With phpstan level 6, after adding all the phpdoc array types, we are left with:

------ -------------------------------------------- 
  Line   lib/Client.php                              
 ------ -------------------------------------------- 
  152    Negated boolean expression is always true.  
  157    If condition is always false.               
  160    Left side of || is always false.            
  234    If condition is always false.               
  249    If condition is always false.               
 ------ -------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------- 
  Line   lib/functions.php                                                                                 
 ------ -------------------------------------------------------------------------------------------------- 
  131    Strict comparison using === between null and array<string, mixed> will always evaluate to false.  
 ------ --------------------------------------------------------------------------------------------------

They are all related to variables that have address references (&) - phpstan is not able to understand exactly how they can change their value. I will add them to the ignore-errors.

There were a couple of similar things in the test code - I tagged those with phpstan-ignore-line rather than polluting phpstan.neon with items that are only in the test code.

phil-davis · 2022-08-30T05:47:28Z

lib/Client.php

+     * @var resource|null
     */
-    private $curlHandle;
+    private $curlHandle = null;


This helps phpstan to process potential access to $curlHandle before it is written (checks for === null are seen as valid)

phil-davis · 2022-08-30T05:48:04Z

lib/Client.php

+     * @var resource|null
     */
-    private $curlMultiHandle;
+    private $curlMultiHandle = null;


This helps phpstan to process potential access to $curlMultiHandle before it is written (checks for === null are seen as valid)

phil-davis · 2022-08-30T05:52:11Z

lib/MessageDecoratorTrait.php

-     *
-     * @var MessageInterface
-     */
-    protected $inner;


I took this out, and added it to the Request and Response decorators. In those places, I can declare it is RequestInterface and/or ResponseInterface and then phpstan can work out what is going on and be made happy.

phil-davis · 2022-08-30T05:57:28Z

lib/MessageInterface.php

+    /**
+     * Returns the HTTP version.
+     */
+    public function __toString(): string;


I needed to do this because otherwise PHPstan complains:

------ ------------------------------------------------------------------------ Line lib/RequestDecorator.php ------ ------------------------------------------------------------------------ 189 Call to an undefined method Sabre\HTTP\RequestInterface::__toString(). ------ ------------------------------------------------------------------------ ------ ------------------------------------------------------------------------- Line lib/ResponseDecorator.php ------ ------------------------------------------------------------------------- 76 Call to an undefined method Sabre\HTTP\ResponseInterface::__toString(). ------ -------------------------------------------------------------------------

The Decorator classes have __toString that expects __toStringto exist in aRequestInterfaceandResponseInterface. And they both extend MessageInterface`

staabm

looks great. please merge and do a separate PR in case you plan todo another level up

phil-davis · 2022-08-30T06:05:22Z

tests/HTTP/MessageTest.php

+    public function __toString(): string
+    {
+        return 'mock text';
+    }


This needs __toString because Message implements MessageInterface and MessageInterface now has __toString

staabm · 2022-08-30T06:05:53Z

(and don't squash while merging.. these independent commits are useful)

phil-davis self-assigned this Aug 26, 2022

Set min PHP to 7.4

509cf73

phil-davis force-pushed the php-7.4 branch from 9e68d56 to 509cf73 Compare August 26, 2022 06:01

staabm reviewed Aug 26, 2022

View reviewed changes

lib/functions.php Outdated Show resolved Hide resolved

Declare types in code

cbf8393

phil-davis commented Aug 26, 2022

View reviewed changes

composer.json Outdated Show resolved Hide resolved

phil-davis commented Aug 26, 2022

View reviewed changes

lib/Response.php

*/

protected $status;

protected int $status;

Copy link

Contributor Author

phil-davis Aug 26, 2022

Choose a reason for hiding this comment

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

And this one says that the HTTP status code is an int

throw InvalidArgumentException when mime-type format is wrong

697b090

phil-davis commented Aug 26, 2022

View reviewed changes

Init absoluteUrl to null in Request.php

4dace4c

phil-davis added 2 commits August 26, 2022 16:59

Declare types in tests

fcb6ecb

Require current versions of sabre/event and sabre/uri

735df25

Increase phpstan to level 6

15e4bb3

Add remaining phpstan errors to ignoreErrors

5453449

phil-davis commented Aug 30, 2022

View reviewed changes

staabm approved these changes Aug 30, 2022

View reviewed changes

phil-davis commented Aug 30, 2022

View reviewed changes

phil-davis requested a review from staabm August 30, 2022 06:06

phil-davis merged commit 1c2ac9f into sabre-io:master Aug 30, 2022

phil-davis deleted the php-7.4 branch August 30, 2022 06:08

kesselb mentioned this pull request Jun 14, 2023

Unknown scalar type #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set min PHP to 7.4 #192

Set min PHP to 7.4 #192

phil-davis commented Aug 26, 2022 •

edited

Loading

phil-davis Aug 26, 2022

phil-davis Aug 26, 2022

phil-davis Aug 26, 2022

codecov bot commented Aug 26, 2022 •

edited

Loading

phil-davis commented Aug 29, 2022

phil-davis commented Aug 30, 2022

phil-davis Aug 30, 2022

phil-davis Aug 30, 2022

phil-davis Aug 30, 2022

phil-davis Aug 30, 2022

staabm left a comment

phil-davis Aug 30, 2022

staabm commented Aug 30, 2022

Set min PHP to 7.4 #192

Set min PHP to 7.4 #192

Conversation

phil-davis commented Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 26, 2022 • edited Loading

Codecov Report

phil-davis commented Aug 29, 2022

phil-davis commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Aug 30, 2022

phil-davis commented Aug 26, 2022 •

edited

Loading

codecov bot commented Aug 26, 2022 •

edited

Loading