Curl and fragment URLs break testing

Just found this after creating #705854: clickLink() does not work for url_target containing a fragment and breaks testing.

Let's keep this issue for the CURL fragment bug solving, and #705854: clickLink() does not work for url_target containing a fragment and breaks testing as a workaround until this is fixed (if the CURL fragment bug ever gets fixed).

Note that I'm able to reproduce this on both debian PHP 5.2.6-1+lenny4 and MAMP PHP 5.2.10.

Escalating to critical, since I just had to commit a hackish workaround at #705854: clickLink() does not work for url_target containing a fragment and breaks testing in an attempt to get testbot online. We need to make sure we remove this before release.

critical per #3.

Not sure if this is the right way to go here. However, I think that no matter what we are going to end with a 'hack' because cURL has a bug...

I basically fixup the URL in clickLink, and use the proper query and fragment parameters to drupalGet. drupalGet now always NULL's out the fragment parameter, since no matter what, cURL will screw it up...

Whoops, that exploded. Looks like query needs to be broken into an associative array.

I agree with cha0s that we'll have to resort to hacking no matter what. However, the patch does not seem to fix the comment tests mentioned in the original post, most likely because drupalGet is also used by drupalPost, not just clickLink. Note that Apache seems to accept the malformed requests but for example IIS rejects these with a "400 Bad Request - Invalid URL", which is why the testbot doesn't report any problems.

Since drupalGet is called in more than one location, and since it's drupalGet that performs the actual curl_exec, I feel it makes sense to fix it there (despite arguments not to in #705854-12: clickLink() does not work for url_target containing a fragment and breaks testing)

+++ modules/simpletest/drupal_web_test_case.php	2010-02-10 08:28:20 +0000
@@ -1959,19 +1962,69 @@
+      $p = parse_url($url_target);

Variable name should be one or more words. In the context of parse_url I think $parts is used often.

+++ modules/simpletest/drupal_web_test_case.php	2010-02-10 08:28:20 +0000
@@ -1959,19 +1962,69 @@
+           $kv = explode("=", $param);

Same here, $kv should be something more meaningfull

Powered by Dreditor.

there are also plenty of lines ending with whitespaces in #7.

Ok, here is a first stab at resolving this issue. I think the Heine's suggestion of implementing the redirects ourselves is the only viable option, as there is no other way to prevent cURL from submitting the fragment as part of the request.

• removes the fix implemented #705854: clickLink() does not work for url_target containing a fragment and breaks testing again
• Strips the fragment from the URL before submitting it via curl_exec
• Captures 3xx responses and follows the URL specified in the Location header (instead of letting cURL handle the redirects)

All the comment tests now pass on my local Vista machine (previously they broke down horribly because of "400 - Bad Request" responses from the server). This also confirms that this fix takes care of the clickLink() issue.

Still to do:

• Implement a max_redir alternative to cURL's CURLOPT_MAXREDIRS
• Make the fragment removal more robust (and more elegant)

Setting this to CNR to see how this runs on non-IIS servers.

This should fix the remaining fails.

Still some work todo, but running it past the test bot first.

It's not valid to call array_shift() on an expression (instead of a reference). Anyways this is what string functions are for, I'd suggest something like $curl_options[CURLOPT_URL] = strtok($curl_options[CURLOPT_URL], '#');

• Used strtok instead of the ugly array_shift construct as per mfb's suggestion
• Added in a replacement for cURL's maximum redirect option by keeping an internal count of redirects and allowing the maximum number of redirects to be set via simpletest_maximum_redirect. Also included a test case verifying that when setting the maximum # of redirects to 1, handling of a request with 2 redirects stops after the first one.

Sounds like CURL guys are fixing this...I would much prefer that. Not sure when we started testing on IIS, but oh well. Anyhoo this code seems to provide a workaround for the curl bug. I am pained to see such code since we continue to move curl in the DWTC and thus alleviate the advantages of use it.

The Dubai World Trade Center? :P

+++ modules/simpletest/drupal_web_test_case.php	20 Feb 2010 19:45:08 -0000
@@ -1332,12 +1336,26 @@
+    // cURL incorrectly handles URLs with a fragment by including the
+    // fragment in the request to the server, causing some web servers
+    // to reject the request citing "400 - Bad Request". To prevent
+    // this, we strip the fragment from the request.

I'd mention that this is fixed in cURL vX so we keep track of this track of this hack. I think a @todo is appropriate here.

+++ modules/simpletest/drupal_web_test_case.php	20 Feb 2010 19:45:08 -0000
@@ -1348,15 +1366,35 @@
+    // cURL incorrectly handles URLs with fragments, so instead of
+    // letting cURL handle redirects we take of them ourselves to
+    // to prevent fragments being sent to the web server as part 
+    // of the request.

I'd add a @todo here as well.

Rolled in comment.

Confirm this bug on ubuntu server (current version 9.10)
PHP 5.2.10-2ubuntu6.4
curl 7.19.5 (x86_64-pc-linux-gnu) libcurl/7.19.5 OpenSSL/0.9.8g zlib/1.2.3.3 libidn/1.15

After applying this patch testing of pgsql gets easy :)

PS: re-roll against current HEAD

Committed to CVS HEAD. Thanks.