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

[HttpClient] Document the state object that is passed around by the HttpClient #30967

Merged
merged 1 commit into from Apr 12, 2019

Conversation

@derrabus
Copy link
Contributor

derrabus commented Apr 7, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

In an attempt to make the code of the new HttpClient component more understandable, I've introduced internal classes that document the $multi object that is being passed around between *Client and *Response classes.

My goal is to make the code more accessible to potential contributors and static code analyzers.

@derrabus derrabus force-pushed the derrabus:improvement/http-client-state branch 2 times, most recently from 5a6a40b to 1407181 Apr 7, 2019

@xabbuh xabbuh added this to the next milestone Apr 7, 2019

@derrabus

This comment has been minimized.

Copy link
Contributor Author

derrabus commented Apr 7, 2019

@linaori
Copy link
Contributor

linaori left a comment

I like these changes from a reader's perspective, it makes things easier to understand!

@@ -49,7 +50,7 @@ trait ResponseTrait
'error' => null,
];
private $multi;

This comment has been minimized.

Copy link
@derrabus

derrabus Apr 7, 2019

Author Contributor

The trait itself does not use that property. And since both implementations use different classes for $multi, I've moved the property to the classes.

@derrabus derrabus changed the title Document the state object that is passed around by the HttpClient [HttpClient] Document the state object that is passed around by the HttpClient Apr 7, 2019

@derrabus derrabus marked this pull request as ready for review Apr 7, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

nice improvements, thanks :)

Show resolved Hide resolved src/Symfony/Component/HttpClient/ClientState/CurlClientState.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/ClientState/DnsCache.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/CurlResponse.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/CurlResponse.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/CurlResponse.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/NativeResponse.php Outdated
@@ -162,7 +166,7 @@ private function open(): void
/**
* {@inheritdoc}
*/
private function close(): void
protected function close(): void

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 8, 2019

Member

private was good - traits are not contracts (same below)

This comment has been minimized.

Copy link
@derrabus

derrabus Apr 10, 2019

Author Contributor

This was done inconsistently. Sometimes the overridden trait methods were private, sometimes protected.

Apart from that, PhpStorm has troubles inspecting this, but this is probably something JetBrains should fix.

Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/NativeResponse.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/NativeResponse.php Outdated
@stof
Copy link
Member

stof left a comment

Symfony\Component\VarDumper\Caster\SymfonyCaster should be updated for the new type of the multi property.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 8, 2019

Symfony\Component\VarDumper\Caster\SymfonyCaster should be updated for the new type of the multi property.

actually, there is nothing to do on the topic, the caster just cuts the property:

public static function castHttpClient($client, array $a, Stub $stub, $isNested)
{
$multiKey = sprintf("\0%s\0multi", \get_class($client));
$a[$multiKey] = new CutStub($a[$multiKey]);
return $a;
}

@derrabus derrabus force-pushed the derrabus:improvement/http-client-state branch from af3eeee to 2d579d6 Apr 10, 2019

@derrabus

This comment has been minimized.

Copy link
Contributor Author

derrabus commented Apr 10, 2019

PR is ready for review again.

@derrabus derrabus force-pushed the derrabus:improvement/http-client-state branch from ac249d1 to 20f4eb3 Apr 10, 2019

public $hostnames = [];
/**
* @var string[]

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 11, 2019

Contributor

below you're inlining docblocks into a single line :/

This comment has been minimized.

Copy link
@derrabus

derrabus Apr 11, 2019

Author Contributor

Shall I do this here as well?

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 11, 2019

Contributor

i believe multi-line is the most common CS rule in symfony.

@fabpot

fabpot approved these changes Apr 12, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Apr 12, 2019

Thank you @derrabus.

@fabpot fabpot merged commit 20f4eb3 into symfony:master Apr 12, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 12, 2019

bug #30967 [HttpClient] Document the state object that is passed arou…
…nd by the HttpClient (derrabus)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[HttpClient] Document the state object that is passed around by the HttpClient

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

In an attempt to make the code of the new HttpClient component more understandable, I've introduced internal classes that document the `$multi` object that is being passed around between *Client and *Response classes.

My goal is to make the code more accessible to potential contributors and static code analyzers.

Commits
-------

20f4eb3 Document the state object that is passed around by the HttpClient.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.