The PHP curl library in the simpletest browser uses a special HTTP 1.1: 100 (Continue) Status on POST requests, that have a certain size. Some webservers (like lighttpd) do not support this behaviour and return HTTP error codes (417 in case of lighttpd). Here is a small patch that fixes this issue.

See also:
http://at.php.net/manual/de/function.curl-setopt.php#82418
http://www.gnegg.ch/2007/02/the-return-of-except-100-continue/

CommentFileSizeAuthor
#9 simpletest_434336.patch1.44 KBklausi
#7 simpletest_434336.patch1.24 KBklausi
#2 simpletest_434336.patch1.13 KBklausi
simpletest_curl_expect_100.patch1.13 KBklausi

Comments

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

I believe that we already had another issue for this, but I can find it right now.

Anyway, the patch looks good. Some English style nitpick:

+          // This is a fix for the Curl library to avoid Expect: 100-continue
+          // headers, that may cause unexpected HTTP resonse codes from some
+          // webservers (like lighttpd returns 417 error code).

* "to avoid" => "to prevent"
* "resonse" => "response"
* "... (like lighttpd *that* returns *a* 417 error code)"

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new simpletest_434336.patch1.13 KB

Improved comment as proposed.

fago’s picture

fago
Primary language German
Location Vienna
commented

I've tested it with lighty, it fixes the problem. Can someone verify it still works with other servers like apache?

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

All our test slaves run Apache, so I'm very confident it works ;)

dries’s picture

dries commented

Just wondering -- if it is a fix for the curl library, wouldn't it be better if this lived in curlExec() instead of outside curlExec().

It would also be useful to describe how this fixes things.

I would also add a new line after the fix so that it is clear that the comment relates to that one line only.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Marking needs work, per Dries.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new simpletest_434336.patch1.24 KB

Dries wrote:
> Just wondering -- if it is a fix for the curl library, wouldn't it be better if this lived in curlExec() instead of outside curlExec().

The bug is related to POST requests, so I would suggest to keep it in drupalPost().

> It would also be useful to describe how this fixes things.

It manually sets an empty "Expect:" header field that is not overwritten by curl. I added this in the code comment.

> I would also add a new line after the fix so that it is clear that the comment relates to that one line only.

Agreed.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

I suggested moving this code to curlExec(), with a if (!empty($options[CURLOPT_POST]) wrapper.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new simpletest_434336.patch1.44 KB

OK, moved to curlExec().

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

This is good. I had this problem with my ghetto twitter client which uses the curl command line utility and http://www.shoemoney.com/2008/12/29/lib-curl-twitter-api-expect-100-cont... same problem. Did not occur to me tho it was more than twitter affected. Good catch.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Thanks for the additional comments and improvements. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.