Skip to content

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

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

Closed
lunika opened this issue Dec 14, 2016 · 16 comments
Closed

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

lunika opened this issue Dec 14, 2016 · 16 comments
Assignees
Labels

Comments

@lunika
Copy link

lunika commented Dec 14, 2016

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 :

  • my server is not well configured and does not deal correctly with the request received
  • the content-length should be set with the body size if present.

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
Copy link
Contributor

dbu commented Dec 14, 2016

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
Copy link
Author

lunika commented Dec 14, 2016

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

@lunika
Copy link
Author

lunika commented Dec 14, 2016

So here is my scenario :

  • I use a MultipartStream to upload files to an API
  • I use the ContentLength plugin to not manage this header, the plugin does it for me
  • the plugin set the content-length plugin with the good value
  • the curl client looks if the content-length header is set and if yes, it tries to override it with the CURLOPT_POSTFIELDS length value which is not used in my case, so it set it to 0.

@joelwurtz
Copy link
Member

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
Copy link
Author

lunika commented Dec 15, 2016

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

@mekras mekras self-assigned this Dec 15, 2016
@mekras mekras added the bug label Dec 15, 2016
@mekras
Copy link
Collaborator

mekras commented Dec 15, 2016

This is an error caused by e3ae04c.

@mekras
Copy link
Collaborator

mekras commented Dec 15, 2016

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
Copy link
Author

lunika commented Dec 15, 2016

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 mekras removed the feedback label Dec 15, 2016
@mekras
Copy link
Collaborator

mekras commented Dec 15, 2016

OK, I'll see this tomorrow.

@lunika
Copy link
Author

lunika commented Dec 22, 2016

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
Copy link
Collaborator

mekras commented Dec 26, 2016

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
Copy link
Collaborator

mekras commented Dec 29, 2016

Check current master.

@lunika
Copy link
Author

lunika commented Jan 2, 2017

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

@mekras
Copy link
Collaborator

mekras commented Jan 2, 2017

Did you plan to release a bugfix soon ?

I hope so.

mekras added a commit that referenced this issue Jan 2, 2017
Content-length located in $_SERVER['HTTP_CONTENT_LENGTH'].
@mekras mekras closed this as completed Jan 2, 2017
@mekras
Copy link
Collaborator

mekras commented Jan 2, 2017

Version 1.6.2 released.

@lunika
Copy link
Author

lunika commented Jan 2, 2017

Thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants