Fixes #36 Client::setUri() should keep relative uri status by samsonasik · Pull Request #149 · zendframework/zend-http

samsonasik · 2018-05-06T03:26:43Z

Provide a narrative description of what you are trying to accomplish:

Are you fixing a bug?
- Detail how the bug is invoked currently.
  Supply relative uri to Client::setUri()
- Detail the original, incorrect behavior.
  It convert to absolute uri
- Detail the new, expected behavior.
  It should keep as relative uri
- Base your feature on the master branch, and submit against that branch.
- Add a regression test that demonstrates the bug, and proves the fix.
- Add a CHANGELOG.md entry for the fix.

samsonasik · 2018-05-06T04:26:05Z

After some check, I think relative url should not be allowed instead in Zend\Http\Client, so, instead of keeping it, we can do these options:

throw Exception to warn user to use absolute URI, OR
set URI to use current host detected as $lastHost = $this->getRequest()->getUri()->getHost();

thought ?

michalbundyra · 2018-05-06T22:57:18Z

src/Client.php

            // Set auth if username and password has been specified in the uri
-            if ($this->getUri()->getUser() && $this->getUri()->getPassword()) {
-                $this->setAuth($this->getUri()->getUser(), $this->getUri()->getPassword());
+            if ($user = $uri->getUser() && $password = $uri->getPassword()) {


We need brackets around expressions here, see following example - https://3v4l.org/oV5F5:

<?php function a() { return 1; } function b() { return 2; } if ($a = a() && $b = b()) { var_dump($a, $b); }

results:

bool(true) int(2)

and because of that I think there is missing proper test for that change.

I see bracket added, but still I can't see any test for it :)

michalbundyra · 2018-05-06T22:58:55Z

src/Client.php

-            if (! $this->getUri()->getPort()) {
-                $this->getUri()->setPort(($this->getUri()->getScheme() == 'https' ? 443 : 80));
+            if (! $uri->getPort() && $uri->isAbsolute()) {
+                $uri->setPort(($uri->getScheme() == 'https' ? 443 : 80));


I know it was like that before, but we can do two changes here:

remove redundant bracket

change == to ===

michalbundyra · 2018-05-06T23:00:01Z

test/ClientTest.php

+    {
+        $client = new Client('/example');
+        $client->setAdapter(Test::class);
+        $client->send();


There is no any assertion in that test. It is really unclear what we are testing here.

samsonasik · 2018-05-07T08:56:06Z

@webimpress I've added the tests for check isAbsolute()

weierophinney · 2018-08-13T18:39:00Z

src/Client.php

            // Set auth if username and password has been specified in the uri
-            if ($this->getUri()->getUser() && $this->getUri()->getPassword()) {
-                $this->setAuth($this->getUri()->getUser(), $this->getUri()->getPassword());
+            if (($user = $uri->getUser()) && ($password = $uri->getPassword())) {


Please don't do assignments in conditionals.

weierophinney · 2018-08-13T18:41:19Z

test/ClientTest.php

+    /**
+     * @dataProvider uriDataProvider
+     */
+    public function testValidRelativeURI($uri, $isValidRelativeURI)


What behavior is this testing, exactly? This should likely be "testUriCorrectlyDeterminesWhetherOrNotItIsAValidRelativeUri".

weierophinney · 2018-08-13T18:42:28Z

test/ClientTest.php

+        return [
+            ['/example', true],
+            ['http://localhost/example', false]
+        ];


Please add names for each case in the provider. This makes it simpler to determine which case failed when a failure occurs. As an example:

'valid-relative' => ['/example', true], 'invalid-absolute' => ['http://localhost/example', false],

weierophinney · 2018-08-13T18:42:48Z

test/ClientTest.php

+        return [
+            ['https://localhost/example', 443],
+            ['http://localhost/example', 80]
+        ];


Same comment here about adding names for each provider case.

weierophinney · 2018-08-13T18:44:24Z

test/ClientTest.php

+    /**
+     * @dataProvider portChangeDataProvider
+     */
+    public function testAbsoluteSetPortWhenNoPort($absoluteURI, $port)


This needs a name that better describes the behavior being tested: "testUriPortIsSetToAppropriateDefaultValueWhenAnAbsoluteUriOmittingThePortIsProvided".

weierophinney · 2018-08-13T18:44:47Z

test/ClientTest.php

+        $this->assertSame($port, $client->getUri()->getPort());
+    }
+
+    public function testRelativeURIDoesnotSetPort()


Need a better method name here; I'll leave this as an exercise for you!

samsonasik · 2018-08-13T19:28:33Z

@weierophinney done ;). All incorporated.

weierophinney · 2018-08-13T19:41:26Z

@samsonasik https://travis-ci.org/zendframework/zend-http/jobs/415602200#L792

samsonasik · 2018-08-13T19:54:05Z

@weierophinney should be fixed now

samsonasik · 2018-08-13T20:00:46Z

travis green

samsonasik · 2018-09-29T18:54:41Z

@weierophinney mergeable ?

weierophinney

🚢

Fixes #36 Client::setUri() should keep relative uri status Conflicts: CHANGELOG.md

Close #149 Fixes #36

Forward port #149

weierophinney · 2019-01-08T15:51:23Z

Thanks, @samsonasik !

thomasvargiu · 2019-01-13T17:23:56Z

@weierophinney @samsonasik can you help me to understand this PR?
Tests doesn't work with any adapter because without the host it can't connect to anything.

I was working to another branch adding checks to ensure uri host exists.

samsonasik · 2019-01-13T21:38:58Z

it make keep relative uri status, for host exists check, I already commented before at #149 (comment) but that will not related to this PR, keeping exisitng URI host for relative uri seems need another PR

samsonasik added a commit to samsonasik/zend-http that referenced this pull request May 6, 2018

zendframework#149 changelog entry

6eba00d

samsonasik mentioned this pull request May 6, 2018

setUri() invalidates relative URI when setting default ports #36

Closed

michalbundyra suggested changes May 6, 2018

View reviewed changes

weierophinney suggested changes Aug 13, 2018

View reviewed changes

samsonasik added 14 commits August 14, 2018 02:09

Fixes zendframework#36 Client::setUri() should keep relative uri status

3a8fa0b

zendframework#149 changelog entry

fa28d86

move repetitive getUri() call to variable uri

7f71222

relative uri / prefix

cd7c029

test Client::send() with relative uri

145acdb

bracket and === fix

f6707c0

add assertion for testSendRequestWithRelativeURI()

39ca2b4

test isValidRelative() === false on absolute uri client send

5198676

add test for isAbsolute() check set port

14f74f1

assertNull()

89ea24e

no port test

e362298

no port test

5e96389

no port test

3132fe9

move changelog to 2.8.3

427e54e

samsonasik force-pushed the fix-36 branch from b4e1344 to 427e54e Compare August 13, 2018 19:12

samsonasik added 6 commits August 14, 2018 02:14

move assignment outside if

951098f

add name on each provider for uriDataProvider

74325d6

name test for correctly determine valid uri for relative and absolute

92f8cf3

dataProvider naming in each row fix

2b5e588

test name for approprite port on set absolute uri

47dbd2e

test name for port not set on relative uri

b8f12e0

cs fix

aa48298

weierophinney approved these changes Jan 7, 2019

View reviewed changes

weierophinney added this to the 2.8.3 milestone Jan 7, 2019

weierophinney merged commit aa48298 into zendframework:master Jan 8, 2019

weierophinney added a commit that referenced this pull request Jan 8, 2019

Merge pull request #149 from samsonasik/fix-36

75969b6

Fixes #36 Client::setUri() should keep relative uri status Conflicts: CHANGELOG.md

weierophinney added a commit that referenced this pull request Jan 8, 2019

Merge branch 'hotfix/149'

b98e979

Close #149 Fixes #36

weierophinney added a commit that referenced this pull request Jan 8, 2019

Merge branch 'hotfix/149' into develop

a2c5548

Forward port #149

samsonasik deleted the fix-36 branch January 8, 2019 16:13

thomasvargiu mentioned this pull request Jan 17, 2019

Zend\Http\Client should not accept relative URIs #170

Open

weierophinney mentioned this pull request Dec 31, 2019

Zend\Http\Client should not accept relative URIs laminas/laminas-http#3

Closed

Conversation

samsonasik commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsonasik commented May 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsonasik commented Aug 13, 2018

Uh oh!

weierophinney commented Aug 13, 2018

Uh oh!

samsonasik commented Aug 13, 2018

Uh oh!

samsonasik commented Aug 13, 2018

Uh oh!

samsonasik commented Sep 29, 2018

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

weierophinney commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasvargiu commented Jan 13, 2019

Uh oh!

samsonasik commented Jan 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samsonasik commented May 6, 2018 •

edited

Loading

samsonasik commented May 6, 2018 •

edited

Loading

weierophinney commented Jan 8, 2019 •

edited

Loading