ixudra / curl

Custom PHP curl library for the Laravel 5 framework - developed by Ixudra
MIT License
561 stars 128 forks source link

Add warning in documentation as an argument for setting headers should be not key value array, but indexed array. Or make according changes in withHeader method #112

Closed vasymus closed 4 years ago

vasymus commented 4 years ago

After spending several hours on finding the bug, decide to use raw https://www.php.net/manual/ru/ref.curl.php

And found out that in the first time working with your library i just put headers wrong

Instead of correct way:

$response->withHeaders([
     'X-Requested-With: XMLHttpRequest',
    'Content-Type: application/x-www-form-urlencoded',
]);

i did wrong way:

$response->withHeaders([
     'X-Requested-With' => 'XMLHttpRequest',
    'Content-Type' => 'application/x-www-form-urlencoded',
]);

So, my proposition for you: either put a warning in your documentation about headers (i know, you describe the right way, but maybe you should put some more highlighted description) or you could rewrite you methods in vendor/ixudra/curl/src/Builder.php public function withHeader and public function withHeaders

for example

    public function withHeader($header)
    {
        if (is_array($header)) $header = implode(": ", $header);
        $this->curlOptions[ 'HTTPHEADER' ][] = $header;
        return $this;
    }

and

    public function withHeaders(array $headers)
    {
        $h = [];
        foreach ($headers as $key => $value) {
            if (!is_numeric($key)) {
                $value = "$key: $value";
            }
            $h[] = $value;
        }
        $this->curlOptions[ 'HTTPHEADER' ] = array_merge(
            $this->curlOptions[ 'HTTPHEADER' ], $h
        );
        return $this;
    }
elimentz commented 4 years ago

Hi @vasymus thanks for letting me know. An argument could be made for either case, of course, and I understand what you're saying. Since the point of this package is increased clarity and ease of use, I fully agree with your proposal. I'll make the necessary changes some time this weekend

vasymus commented 4 years ago

Thanks @elimentz ! :-)

elimentz commented 4 years ago

Now available in version 6.18.0! I only changed the withHeaders() method, as it seems very unlikely to me that the withHeader() method will be used incorrectly