-
Notifications
You must be signed in to change notification settings - Fork 299
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
Add proper Content-Length based off of the request body's length #85
Conversation
Make sure to use serialized_payload length as Content-Length if the request has a body
if (isset($this->payload)) { | ||
$this->serialized_payload = $this->_serializePayload($this->payload); | ||
curl_setopt($ch, CURLOPT_POSTFIELDS, $this->serialized_payload); | ||
$this->headers['Content-Length'] = strlen($this->serialized_payload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't checked the HTTP spec yet, but should this be multi byte string length mb_strlen
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I think you are correct. We want strlen
even for multibyte characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. It is the size in bytes not number of characters.
On Jul 3, 2013 12:00 PM, "Nate Good" [email protected] wrote:
In src/Httpful/Request.php:
@@ -784,6 +784,14 @@ public function _curlPrep()
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, $this->strict_ssl);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
// https://github.com/nategood/httpful/issues/84
// set Content-Length to the size of the payload if present
if (isset($this->payload)) {
$this->serialized_payload = $this->_serializePayload($this->payload);
curl_setopt($ch, CURLOPT_POSTFIELDS, $this->serialized_payload);
$this->headers['Content-Length'] = strlen($this->serialized_payload);
Nope, I think you are correct. We want strlen even for multibyte
characters.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/85/files#r5012797
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Silly question. Brain is moving slow this morning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result:
string 'test' (length=4)
int 4
int 4
int 4
string 'тест' (length=8)
int 4
int 4
int 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That fix is incorrect:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.13
More specifically: Applications SHOULD use this field to indicate the
transfer-length of the message-body, unless this is prohibited by the rules
in section 4.4.
On Fri, Sep 13, 2013 at 11:12 AM, wackyfrog [email protected]:
In src/Httpful/Request.php:
@@ -784,6 +784,14 @@ public function _curlPrep()
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, $this->strict_ssl);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
// https://github.com/nategood/httpful/issues/84
// set Content-Length to the size of the payload if present
if (isset($this->payload)) {
$this->serialized_payload = $this->_serializePayload($this->payload);
curl_setopt($ch, CURLOPT_POSTFIELDS, $this->serialized_payload);
$this->headers['Content-Length'] = strlen($this->serialized_payload);
Result:
string 'test' (length=4)
int 4
int 4
int 4
string 'тест' (length=8)
int 4
int 4
int 8—
Reply to this email directly or view it on GitHubhttps://github.com//pull/85/files#r6349942
.
It looks correct to me! It should contains the number of OCTETs (8bit), so the number of bytes, not chars! |
Add proper Content-Length based off of the request body's length
Hi! Could you update it on packagist? |
Sorry about that. Forgot the version bump in composer.json. Try 0.2.6 via On Thu, Jul 4, 2013 at 7:04 AM, Gabriele Tondi [email protected]:
Nate Good Ph: 412.259.5869 |
https://packagist.org/packages/nategood/httpful is still showing dev-master, dev-dev, and 0.2.5 as the latest versions even though composer.json shows 0.2.6 as the version info. Do you have to force packagist to update? |
Okay figured it out. My git tag and composer.json update weren't aligned. On Fri, Jul 5, 2013 at 11:59 AM, Josh Johnston [email protected]:
Nate Good Ph: 412.259.5869 |
Wrong length for mutlibyte body (utf-8). So, form submits incorrect. |
Are you sure? Content length should be the number of bytes not the number
|
I'm sure. Test example: Result: So, header 'content-length' contains invalid length with strlen (shorter) |
Ah. Yes. This makes sense if |
I misunderstood the problem. I thought you were suggesting that it should be sending the number of characters. Disregard my previous response(s). I agree, the |
Make sure to use serialized_payload length as Content-Length if the request has a body