contact.test broken (was upload.test)

I agree that the user picture test belongs in user.test (none of the functionality tested depends on upload.module). I started working on this issue, and there's a good amount of work/clean-up that needs to be done. Here's my plan:
1) move the test class UploadPictureTestCase found in modules/user/upload.test to user.test and rename it UserPictureTestCase.
2) remove modules/user/upload.test (this file is slightly different from modules/upload/upload.test as pointed out in the original issue. I looked at the differences, and we do want to delete this version).
3) remove the class UploadPictureTestCase from modules/upload/upload.test, as this class is now in user.test.
4) There is a lot of duplicate code in various places, and a lot of code clean-up. The goal is to have a patch with clean code that passes the tests with no failures.

Here's a patch for steps 1-4 from my last comment, and some code clean-up in the UserPictureTestCase class.

The tests still fail whenever a test tries to simulate a file upload (upload.test and user.test are the only core tests that use the simpletest upload feature). The simpletest handbook page is outdated and wasn't helpful, it just shows that the upload feature was working at some point.

The debugging I've done so far shows that everything is ok until the call to curl_exec(), at which point the file doesn't get uploaded.

I ran this upload test on an up-to-date copy of D7 HEAD. True enough, the upload test fails at the

File attached successfully. at [/example.com/modules/upload/upload.test line 173] [Other] Fail

It appears that the test is failing at the point in which the form's file field is populated with a path. Or, supposedly, it's being given a valid path.

Changing 'component' in relation to http://drupal.org/node/253744.

As a note you linked to this issue as a duplicate. :)

Oops. Sorry! :) Subscribing.

Well, it turns out that CURLOPT_POST => TRUE must be set before CURLOPT_POSTFIELDS => $post if $post is an array, as documented (in a comment!) on the curl_setop() manual page [1].

This patch does that, and remove code that is not needed anymore thanks to that change. Preliminary testing seems to show the change doesn't break anything (but more testing is definitely required).

Damien

[1] http://php.net/manual/en/function.curl-setopt.php

@Damien: thank you, thank you, thank you!

The ordering of the cURL parameters was indeed what caused the problem, and if you hadn't found this comment on the php manual this issue would stay open for a much longer time.

The only problem that I see with the patch is the line:

$post = array();

I don't see why we would reset the $post array at this point (it's already initialized earlier in the code, and then processed by handleForm()). With this line removed, both the upload test and the user picture test (with the patch from #2) pass without failure!

Here's an updated patch combining #2, #9.

Note that the user picture upload test fails when GD is not enabled. That is because the value of image_get_toolkit() is evaluated to TRUE even when GD is not enabled, so image_gd_check_settings() should be used to check for the presence of the gd library instead. That should probably be fixed after the main issue from duplicate tests and inability to upload files with simpletest.

@flobruit, oh sorry. I messed up with my merging, the $post initialization was lying there by mistake.

Committed to CVS HEAD. We're down to 56 failures now. Woot, woot.

Removing the duplicate upload.test inside of the user module's folder was part of the patch but hasn't been committed.

Sorry, the additional changes to user.test shouldn't have been included in the patch (they're for the non-GD part of the user upload test, which is a different issue).

Committed to CVS HEAD. Thanks.

Yes, this code allows field values to start with the '@' character when there are no file uploads on a form. It's a messy piece of code, but I agree that it shouldn't have been removed. This does fix the issue with contact.test (where the test used a value starting with '@' for the e-mail field).

This behavior regarding values starting with '@' is only "documented" by the code itself. We need to mention this in the documentation of drupalPost().

No longer applies. :(

Oh. Well that was the easiest re-roll ever. Just the documentation fix hunk. Re-rolled.

Confirmed that this still fixes the problem.

We really need to step back and think on this one.

I did a very fast review of libcurl code that convinced me that when CURLOPT_POSTFIELDS is an array, cURL does not urlencode the fields. So we need to answer these questions first:

• Can we escape the '@' for fields that are not upload fields, for example by using the entity-encoded version of the character (a &#xxx; something)?
• What to do with the key part, does this also needs to be urlencoded? What to do with key parts that contains '='?
• Should we choose urlencode or rawurlencode?

Could someone dig up the relevant part of the HTTP RFC and answer these questions? I would really like to see a simplified version of that ugly code that work on both cases (with an upload file and without one).

I investigated this, and the bad news is that there is no real solution for that problem. We will have to handle file upload ourselves, because the PHP extension leave us no choice to escape the '@' prefix.

The good news is that I found where these form submission are defined. It turns out that they are only informally defined in the HTML specification (!), here: http://www.w3.org/TR/html4/interact/forms.html#h-17.13.4.1

This also clarifies that we really need urlencode() and not rawurlencode(). So I updated the comments in the patch. Keeping the RTBC because only comments where modified.

I guess we could, with some work, handle the file upload submission ourselves and definitely get rid of the 'if you have an upload you can't have a field starting with @' annoyance.

And here is the patch.

Fixes the tests indeed. The patch looks valid looking at comment #33

Personally, I think the '@' issue is a bit ugly. Before I commit this patch, I'd like to discuss what alternative solution we could implement.

@Dries: as the author of the patch that broke things, I investigated possible solutions in #31 and #33. It turns out that php_curl implementation is really broken (I reviewed the source code) and does not allow us to do that.

The only alternative that I can see is rewriting the HTTP client ourselves in PHP and getting rid of cURL completely. This would not be such a very big work (implementations probably do exist in GPL already), but requires some careful review and thinking.

In the meantime, we need to fix the regression I introduced, so let's commit this fix, but keep that issue open. We will come up with something else at a latter time.

Damien, if you extend the documentation a bit, I'll commit it. It's good to capture why we do it this way and how we could do it better if curl wasn't broken. In other words, it would be good to leave a TODO list in the code so we can revisit this later. Thanks.

OK, the documentation addition is good enough. Committed to CVS HEAD.