Corrected a problem with memory

[jack-theripper]

I have corrected some inconvenient moments and have up to the end corrected a problem with memory.

fix inheritance support. fix custom reading function (CURLOPT_READFUNCTION). fix custom methods. fix "Expect" and "Accept" header. fix PATCH with send body. fix support native PHP stream filters. fix support follow location (CURLOPT_FOLLOWLOCATION). fix compatible stream with PSR-7 specifications. fix out of memory when a large body send of response. fix out of memory with sendAsyncRequest in some cases.

[sagikazarmark]

:+1:

Wow, this is super awesome, thank you. Needs a rebase though, and @mekras's review.

[mekras]

@jack-theripper, can you split these changes into separate PRs? Also I have some questions and notices.

1. HttpClient::sendRequest() have only one argument — $request, so this change is useless.
2. I doubt that Client class should allow descendants. @sagikazarmark, what do you think? My be better mark it final?
3. Please follow PSR-2.

[jack-theripper]

@mekras, how to split? This complex correction. What do you mean ?

1. I changed the signature because it is necessary, because it is Curl. It differs from the interface - in it there is nothing bad.

Simple example: how to set the timeout to 1 request?

$client = new Client(MessageFactory, StreamFactory, [
    CURLOPT_TIMEOUT => 10
]);

// send request. timeout = 10
$client->sendRequest($request);

// send other request. timeout = 200
$client->sendRequest($request, [
    CURLOPT_TIMEOUT => 200
]);

// send other request. timeout = 10
$client->sendRequest($request);

2 . The client must be flexible. Maybe, but artificial restrictions - it is bad. Should not be artificial restrictions.

it's all the questions?

[mekras]

how to split? This complex correction. What do you mean ?

At least "replace property visibility" is not necessary for other changes, so can be moved to a separate PR, to be discussed. Setting "Accept" header. May be something else. So this is the reason for splitting PR: clearly define the purpose of each code change.

[sagikazarmark]

I changed the signature because it is necessary, because it is Curl. It differs from the interface - in it there is nothing bad.

Options must be injected in the constructor.

The client must be flexible. Maybe, but artificial restrictions - it is bad. Should not be artificial restrictions.

Not quite sure what you are trying to say, but I agree with @mekras. Please don't change the visibility. And yes, it should probably be final.

[jack-theripper]

@sagikazarmark, "PSR conforming" I didn't pay attention to it, I just set in IDE code style "PSR-2" and auto code align.

you use a "php-http/curl-client" in your code ?

I will ask a few questions:

1. how to install Curl option only for 1 request ?
2. if I use a custom StreamFactory, how to get an object StreamFactory from ResponseParser ? ( if the only access to the variable $client)
3. if I need to change some of the methods (for example "createHeaders"), i make "MyClient extends Client" (it will not work because "private") ?

[sagikazarmark]

1. Set up multiple clients if you really need separate options
2. Not sure what you mean. The StreamFactory is injected into the ResponseParser. Why would you need access to it?
3. Not sure how this works in this client, but the behaviour should be 100% configurable. So when adding headers in the client, it either doesn't make sense to change the behaviour or you can do so by some configuration. Also, you can always add custom headers to the request itself. Extending clients is not recommended, we suggest composition over inheritance.

[jack-theripper]

@sagikazarmark, There are situations when you need to get control on CreateResponse (not to spend cpu time on reading and writing streams). I work with a very very large body. I need "Except" headers and change the logic, but the client does not support. That is a quick fix, I need "extends Client", but it is forbidden to artificially.

NOTE: Sorry, but it was a mistake to use this client in my project, he is such incomplete.

[sagikazarmark]

@jack-theripper although I see your problem, and I think it is important to address it, I am pretty sure that it can be addressed in a proper way.

The Client should be CPU and memory efficient out of the box, I don't see why you should extend it to achieve that. I saw some discussion about except headers as well.

If you allow me a personal comment: crticizing others work and being rude doesn't seem to be the correct behaviour, especially if people want to help you solving your issues, even if they see the solution a bit differently. (Not to mention you are trying to enforce bad design)

I think @mekras did quite a good job with this client and he keeps solving the upcoming issues efficiently. If it doesn't fit your use case, you are free to fork it and add any features you want, if you are not satisfied with our support.

[jack-theripper]

@sagikazarmark

The Client should be CPU and memory efficient out of the box, I don't see why you should extend it to achieve that. I saw some discussion about except headers as well.

because the client out of the box works not effectively. If the client can be inherited, then it will save developers time.

I didn't criticize, I just corrected a row of serious problems.

fix out of memory. (issues #15) not correctly, leads to hang the server.

I think I should cancel pull request. I have corrected https://github.com/jack-theripper/curl-client/commit/abbf8fa509763c4fc5b9c460b1f9763364689667 you can use my solution.

NOTE: A similar problem exists in Guzzle Client.

[sagikazarmark]

@mekras can you take over this and see how we can implement it based on @jack-theripper's solution?

[mekras]

@sagikazarmark, I'll check this PR any way. It would have taken less time if @jack-theripper would remove non relevant changes, such as "private → protected", "Accept" header and others.