php-http / curl-client

cURL client
http://httplug.io
MIT License
445 stars 28 forks source link

Request not using CURLOPT_POSTFIELDS have content-length set to 0 #29

Closed lunika closed 7 years ago

lunika commented 7 years ago

I don't know if this is a normal behaviour but if I send a large body (a file > 1MB) the CURLOPT_POSTFIELDS option is not used but the CURLOPT_READFUNCTION instead and in the createHeaders method the content-length header is set to 0.

Doing this, my server doest not reveive data, all is empty.

So 2 possible scenario :

If I set the content-length header with the body, my server receive all the data and have an expected behaviour, I put this for my test :

if ('content-length' === $header) {
                $values = [0];
                if (array_key_exists(CURLOPT_POSTFIELDS, $options)) {
                    $values = [strlen($options[CURLOPT_POSTFIELDS])];
                } else {
                    $body = $request->getBody();
                    $bodySize = $body->getSize();
                    if ($bodySize > 0) {
                        $values = [(string) $bodySize];
                    }
                }
            }

but maybe the CURLOPT_INFILESIZE is a better option

dbu commented 7 years ago

i remember there was something about having to use the content length plugin: http://docs.php-http.org/en/latest/plugins/content-length.html

we use curl quite minimal by purpose to avoid problems when switching from one to another client. but in this case i somehow assume that the guzzle6 adapter will end up doing the content lenght, so maybe this is too strict?

lunika commented 7 years ago

Oh I use the ContentLength plugin and if I remove it everything works perfectly... this is annoying because I use this plugin everywhere.

lunika commented 7 years ago

So here is my scenario :

joelwurtz commented 7 years ago

It look likes a bug from Curl, as guzzle is handling this by removing the header, look at : https://github.com/guzzle/guzzle/blob/9f75c64ba74ea0523821d6679df97e7db023fc80/src/Handler/CurlFactory.php#L263

lunika commented 7 years ago

Thanks @joelwurtz should I do the same and open a PR ? Is it normal to not have a content-length header ?

mekras commented 7 years ago

This is an error caused by e3ae04c6146b0f45072648f25a328af5d20f3285.

mekras commented 7 years ago

Hm… Can not reproduce this at my local machine. @lunika, can you run this script (changing URL if needed):

<?php

$ch = curl_init();

$options = [
    CURLOPT_URL => 'http://localhost/',
    CURLOPT_CUSTOMREQUEST => 'POST',
    CURLOPT_UPLOAD => true,
    CURLOPT_INFILESIZE => 16,
    CURLOPT_READFUNCTION => function () {
        static $done = 0;
        return $done++ ? '' : str_repeat('x', 16);
    }
];

curl_setopt_array($ch, $options);
curl_exec($ch);
curl_close($ch);

and dump request headers, for example with tcpdump like this:

tcpdump -i lo -A tcp dst port 80

My headers:

POST / HTTP/1.1
Host: localhost
Accept: */*
Content-Length: 16
Expect: 100-continue
lunika commented 7 years ago

The real problem is here https://github.com/php-http/curl-client/blob/master/src/Client.php#L327-L331

If you already have the content-length header, you set it to 0 because CURLOPT_POSTFIELDS is not used with a large body.

You can reproduce it in the tests, replace HttpClientTestCase::testSendLargeFile method by this one :

/**
     * Test sending large files.
     */
    public function testSendLargeFile()
    {
        $filename = $this->createTempFile();
        $fd = fopen($filename, 'a');
        $buffer = str_repeat('x', 1024);
        for ($i = 0; $i < 2048; $i++) {
            fwrite($fd, $buffer);
        }
        fclose($fd);
        $body = $this->createFileStream($filename);

        $request = self::$messageFactory->createRequest(
            'POST',
            PHPUnitUtility::getUri(),
            ['content-length' => 42],
            $body
        );

        $response = $this->httpAdapter->sendRequest($request);
        $this->assertResponse(
            $response,
            [
                'body' => 'Ok',
            ]
        );

        $request = $this->getRequest();
        self::assertArrayHasKey('CONTENT_LENGTH', $request['SERVER']);
        self::assertEquals($body->getSize(), $request['SERVER']['CONTENT_LENGTH']);
    }

Your test will fail now.

A solution can be to remove the content-length header like in guzzle6 or to set the right content-length depending the scerio you are with something like this in createHeaders method :

            if ('content-length' === $header) {
                switch (true) {
                    case array_key_exists(CURLOPT_POSTFIELDS, $options):
                        $values = [strlen($options[CURLOPT_POSTFIELDS])];
                        break;
                    case array_key_exists(CURLOPT_INFILESIZE, $options):
                        $values = [$options[CURLOPT_INFILESIZE]];
                        break;
                    default:
                        $values = [0];
                        break;
                }
            }

Doing this the previous test is now green

mekras commented 7 years ago

OK, I'll see this tomorrow.

lunika commented 7 years ago

any news ? How can I help you ? Both solutions (removing the header or recompute it) work well, which one would you like to pick ?

mekras commented 7 years ago

Sorry for the delay, @lunika. I had trouble with the hard drive and I lost all the files. Fortunately I have a backup. I've already reinstalled OS and now restoring files. I hope to continue with curl-client in a few days.

mekras commented 7 years ago

Check current master.

lunika commented 7 years ago

I tested it this morning, it works. Do you know why travis is failing ? Did you plan to release a bugfix soon ?

mekras commented 7 years ago

Did you plan to release a bugfix soon ?

I hope so.

mekras commented 7 years ago

Version 1.6.2 released.

lunika commented 7 years ago

Thank you 👍