phpstan fixes #157

thomasvargiu · 2018-10-16T09:38:50Z

some fixes on PHPDoc
added checks on types when needed

Changed

Throw an exception when unable to uncompress response content
Relative URIs are not allowed anymore in Zend\Http\Client.
Anyway, usage of relative URIs has no sense and no adapter can handle it.

Fixed

Fixed Zend\Http\Cookies::getAllCookies(Zend\Http\Cookies::COOKIE_STRING_ARRAY).
Fixed Zend\Http\Cookies::getAllCookies(Zend\Http\Cookies::COOKIE_STRING_CONCAT).
Fixed use of HTTP URIs with empty path in Request

src/Cookies.php

src/Header/AbstractAccept.php

src/Request.php

weierophinney

There's a lot of great changes here, but also a ton that look like potential backwards compatibility breaks and/or unrelated changes. Please review and address all comments from myself and @Ocramius, and we can then do a final review.

Thanks, @thomasvargiu !

src/Client.php

weierophinney · 2019-01-07T22:18:01Z

src/Client.php

+        /** @var Client\Adapter\AdapterInterface $adapter */
+        $adapter = $this->adapter;
+
+        return $adapter;


I disagree with this change. If $this->adapter was not previously set, we call setAdapter() which will either raise an exception, or set the adapter to an appropriate value.

I think the solution to the phpstan errors in this case would be to say that $adapter can only be an AdapterInterface instance, and not allow null. There are no paths through the client that will allow it to remain null when it needs to be used.

I think we can't say that $adapter can only be an AdapterInterface because is not set in constructor. $adapter will be null until some method will call getAdapter(), then a developer should know that $this->adapter can be null.

Maybe, can we initialize it in constructor?

test/ClientTest.php

weierophinney · 2019-01-07T22:22:01Z

src/Client.php

+
+            if (null === $nextHost) {
+                throw new InvalidArgumentException('Invalid URI provided. No host found');
+            }


This is new behavior, and does not have a corresponding test. Please revert, or add a test.

Tests added

weierophinney · 2019-01-07T23:13:04Z

src/Header/SetCookie.php

     */
-    public function isHttponly()
+    public function isHttpOnly()


Same comment here as for setHttponly(); revert, or introduce the new method as a proxy to the original.

weierophinney · 2019-01-07T23:14:19Z

src/Headers.php

@@ -89,6 +90,10 @@ public static function fromString($string)

            if (preg_match("/^[ \t][^\r\n]*$/", $line, $matches)) {
                // continuation: append to current line
+                if (! isset($current['line'])) {
+                    $current['line'] = '';
+                }


This likely needs a unit test.

Fixed and tested

weierophinney · 2019-01-07T23:15:16Z

src/PhpEnvironment/Request.php

@@ -93,6 +96,9 @@ public function getContent()
    {
        if (empty($this->content)) {
            $requestBody = file_get_contents('php://input');
+            if (false === $requestBody) {
+                throw new RuntimeException('Unable to get contents from php://input');
+            }


This is new behavior and requires a corresponding test.

Fixed and replaced with simpler condition

weierophinney · 2019-01-07T23:16:36Z

src/Response.php

@@ -198,6 +202,9 @@ public static function fromString($string)
        if ($response->statusCode === static::STATUS_CODE_100) {
            $next = array_shift($lines); // take next line
            $next = empty($next) ? array_shift($lines) : $next; // take next or skip if empty
+            if (null === $next) {
+                throw new RuntimeException('Invalid response content');
+            }


This is new behavior and requires a corresponding test.

Tests added

weierophinney · 2019-01-07T23:17:11Z

src/Response.php

+        if (false === $uncompressed) {
+            throw new Exception\RuntimeException(
+                'An error occurred during inflation'
+            );


This is new behavior and requires a corresponding test.

Tests added

weierophinney · 2019-01-08T16:18:29Z

Removing the milestone; I suspect this patch will take a bit of back and forth before it's ready, and we're otherwise ready for the 2.9.0 release. We can target 2.10.0 for this.

weierophinney · 2019-01-08T16:18:56Z

@thomasvargiu You will need to rebase against current develop before you start applying changes.

thomasvargiu · 2019-01-13T12:04:16Z

@thomasvargiu You will need to rebase against current develop before you start applying changes.

@weierophinney should I target develop branch for merge?

weierophinney · 2019-01-13T13:11:30Z

Yes, please.

…

On Sun, Jan 13, 2019, 6:04 AM Thomas Mauro Vargiu ***@***.*** wrote: @thomasvargiu <https://github.com/thomasvargiu> You will need to rebase against current develop before you start applying changes. @weierophinney <https://github.com/weierophinney> should I target develop branch for merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlVw0jtiMLKWbVU8T8dhB4rMJ0eWwAks5vCyDAgaJpZM4Xd9qx> .

src/ClientStatic.php

michalbundyra · 2019-01-17T12:10:31Z

.travis.yml

@@ -9,6 +9,7 @@ env:
    - COMPOSER_ARGS="--no-interaction"
    - COVERAGE_DEPS="php-coveralls/php-coveralls"
    - TESTS_ZEND_HTTP_CLIENT_ONLINE=true
+    - PHPSTAN_DEPS="phpstan/phpstan:^0.10.3"


In other repositories we are adding it to composer dev requirements. Maybe we should do the same here?
Then it will be much easier to run it locally. See: https://github.com/zendframework/zend-expressive/blob/master/composer.json#L40

We can also update to version 0.11 :)

Someone else told me to remove it and to add it in CI.

phpstan fixes phpstan fixes Removed deprecations Fixed wrong condition Better phpdoc types Better tests with relative datetimes Some fixes and removed false return types Replaced return values $this with self Replaced usage of stdClass with object in param doc Removed useless check and getter of adapter Fixed typo Reverted rename of method setHttponly, fixed usage with incorrect case PHPStan fixes Fix variable overwrite Reverted composer changes Fix and test header factory Added test for invalid 100 response Test gzinflate error Added test for default query argument separator Fixed missing char in phpdoc Added test to handle tmpname failure Added test for dispatch() method Fixed cookies usage

weierophinney · 2019-12-31T22:54:58Z

This repository has been closed and moved to laminas/laminas-http; a new issue has been opened at laminas/laminas-http#4.

weierophinney · 2019-12-31T22:56:28Z

This repository has been moved to laminas/laminas-http. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-http to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-http.
In your clone of laminas/laminas-http, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Ocramius suggested changes Oct 16, 2018

View reviewed changes

Ocramius added enhancement WIP labels Oct 16, 2018

thomasvargiu changed the title ~~WIP: phpstan fixes~~ phpstan fixes Oct 17, 2018

thomasvargiu mentioned this pull request Oct 17, 2018

Deprecated multiple behavior methods #158

Closed

weierophinney suggested changes Jan 7, 2019

View reviewed changes

weierophinney modified the milestone: 2.9.0 Jan 7, 2019

thomasvargiu changed the base branch from master to develop January 13, 2019 13:19

thomasvargiu force-pushed the phpstan_fixes branch from 478fc5c to 0921eb1 Compare January 13, 2019 13:20

froschdesign reviewed Jan 13, 2019

View reviewed changes

src/ClientStatic.php Outdated Show resolved Hide resolved

thomasvargiu changed the title ~~phpstan fixes~~ WIP: phpstan fixes Jan 14, 2019

michalbundyra reviewed Jan 17, 2019

View reviewed changes

thomasvargiu force-pushed the phpstan_fixes branch from 8d17521 to 6caf9ef Compare February 19, 2019 23:33

thomasvargiu changed the title ~~WIP: phpstan fixes~~ phpstan fixes Feb 19, 2019

thomasvargiu added 11 commits December 4, 2019 22:27

phpstan fixes

4ec3898

Updated CHANGELOG

2ee0df0

Replaced @return __CLASS__ with @return self

aae9e1e

Replaced stdClass with object in docs

a29e168

Changed inline doc type

4c8c637

Added tests for URIs with no host

c638944

Fixed set default encType

d52d58c

Added tests for doRequest with URI with no host

07e13df

Assignment align

1de59dd

Removed optimized use of core functions

8181076

thomasvargiu added 8 commits December 4, 2019 22:36

Replaced variable names

4847820

Pass $refUri adding cookies to avoid BC break with extended classes

c9fd13b

Fixed cookies class

837bdd9

Removed unused method

b90d014

Other fixes and tests

2b156c3

Removed check of tempnam result

88fda9d

PHPCS

9158ecf

Fixed variable type

ccbbb5f

michalbundyra force-pushed the phpstan_fixes branch from 23d2b27 to ccbbb5f Compare December 4, 2019 22:37

michalbundyra added 2 commits December 4, 2019 22:47

Fix merge error

ce2d910

Fixes other minor merge issues

ec10c86

weierophinney mentioned this pull request Dec 31, 2019

phpstan fixes laminas/laminas-http#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

phpstan fixes #157

phpstan fixes #157

thomasvargiu commented Oct 16, 2018 •

edited

Loading

weierophinney left a comment

weierophinney Jan 7, 2019

thomasvargiu Jan 13, 2019

weierophinney Jan 7, 2019

thomasvargiu Feb 20, 2019

weierophinney Jan 7, 2019

weierophinney Jan 7, 2019

thomasvargiu Feb 20, 2019

weierophinney Jan 7, 2019

thomasvargiu Feb 20, 2019

weierophinney Jan 7, 2019

thomasvargiu Feb 20, 2019

weierophinney Jan 7, 2019

thomasvargiu Feb 20, 2019

weierophinney commented Jan 8, 2019

weierophinney commented Jan 8, 2019

thomasvargiu commented Jan 13, 2019

weierophinney commented Jan 13, 2019 via email

michalbundyra Jan 17, 2019

thomasvargiu Feb 20, 2019

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019

phpstan fixes #157

Are you sure you want to change the base?

phpstan fixes #157

Conversation

thomasvargiu commented Oct 16, 2018 • edited Loading

Changed

Fixed

weierophinney left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Jan 8, 2019

weierophinney commented Jan 8, 2019

thomasvargiu commented Jan 13, 2019

weierophinney commented Jan 13, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019

thomasvargiu commented Oct 16, 2018 •

edited

Loading