File uploads

[soulseekah]

Working on #289 :)

[rmccue]

IMO, this should either be part of $data or part of $options. For the former, I wouldn't be opposed to having a Requests_File interface (with a default implementation) so that you could do e.g:

$response = Requests::post(
    'http://example.com',
    [ 'Content-Type' => 'text/plain' ],
    new Requests_File( __DIR__ . '/README.txt' )
);

(And potentially allow an array of these.)

[soulseekah]

That's a good idea, would allow us to set filenames and mime-types, just like cURL_File does, I was originally intending to do it as part of data, then remembered how this library is based on the Python Requests library and thought that following their convention would be appreciated.

If not, I have nothing against wrapping into a Requests_File instead.

[rmccue]

Python has the benefit of named parameters, so in Python it's:

requests.post( 'http://example.com', files={ 'file': open( 'x', 'rb' ) } )

The equivalent for PHP is our $options array, but we already deviate a bit anyway due to the nature of the language, so I think overloading the $data parameter makes more sense.

[soulseekah]

True, makes sense. I think Requests_File is the more elegant way of doing it then. I'll work on this a bit and send more stuff into here. Thanks.

[rmccue]

btw, for the interface, I'd recommend calling it Requests_File_Interface, and having get_headers() and get_body() methods, which should allow an advanced implementation to offer multipart uploads (although maybe not in Requests itself) or uploading using a data string.

The default implementation should be Requests_File, and we should be able to imply most stuff (upload name, size, etc) from the filename passed in.

[codecov-io]

Codecov Report

Merging #313 into master will decrease coverage by 0.06%.
The diff coverage is 94.28%.

@@             Coverage Diff              @@
##             master     #313      +/-   ##
============================================
- Coverage     92.11%   92.04%   -0.07%     
- Complexity      760      771      +11     
============================================
  Files            21       21              
  Lines          1762     1785      +23     
============================================
+ Hits           1623     1643      +20     
- Misses          139      142       +3

Impacted Files	Coverage Δ	Complexity Δ
library/Requests/Transport/fsockopen.php	94.68% <100%> (+0.49%)	74 <55> (+6)	⬆️
library/Requests.php	75.25% <100%> (-0.34%)	118 <5> (ø)
library/Requests/Transport/cURL.php	92.95% <83.33%> (-0.74%)	71 <35> (+5)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4055bc4...a47689d. Read the comment docs.

[soulseekah]

Coded things up around a Requests_File object. I thought about the interface, but I think it complicates things without bringing anything to the table whatsoever. And we don't really need get_header, get_body or any such things, since the raw stuff is only done in the fsockopen transport. The cURL simply rewraps into a cURL_File object. So the only functions the Requests_File has right now are:

1. Detect mimtype and filename when not provided.
2. Retrieve content.
3. Allow it to be mixed into the $data parameter and detected as a file upload.

Coding multichunk uploading to save RAM is possible for fsockopen, but it's really just fsockopen low-level internals and none of the other transports will use this as cURL handles large uploads intelligently on its own. Handling large uploads using fsockopen efficiently should be outside of the scope of this PR in order to get the feature in with less blood and tears.

Let me know what you think.

[rmccue]

These definitely need to be part of an interface to allow users of the library to create alternative implementations. For example, it should support the following use cases:

• Uploading a file directly from the filesystem (i.e. you pass a path)
• Uploading a file when you have the contents as a string (i.e. you pass a filename and contents in a string)
• Uploading a file when you have an existing file handle (i.e. you pass a filename and pointer)
• Uploading multiple files with any combination of the above

(For any case other than multiple files, we probably shouldn't use multipart/form-data.)

Having an interface expose get_headers() and get_body() should allow all of these to be solved in custom classes easily. There's no huge advantage I can see of using a CURLFile instance apart from streaming the file; potentially, get_body() could return either a string or a file resource, and we could stream the resource directly to the outbound socket. That feels out-of-scope for an initial implementation though. :)

[soulseekah]

There's no huge advantage I can see of using a cURLFile instance apart from streaming the file

cURL doesn't allow streaming for multipart/form-data, using cURLFile is the only way to upload a file as part of the data. And CURLFile only accepts a path. Thus:

Uploading a file when you have the contents as a string.

On multipart/form-data you'd have to save the contents to a temporary file first for cURL.

Uploading a file when you have an existing file handle

Not possible to get a filepath from a file descriptor, especially since it could be a php://input or something. cURLFile needs a path.

we could stream the resource directly to the outbound socket

This would only be possible in cURL by using CURLOPT_READFUNCTION for the whole of the request body. Employing this would mean assembling everything for the cURL transport from the bottom up, which would end up being sort of fsockopen, where everything is very low level.

For any case other than multiple files, we probably shouldn't use multipart/form-data

This would grow the code by a whole branch in fsockopen. cURL sends one file with multipart/form-data (even from the command line), unless, you want to employ CURLOPT_READFUNCTION and handle all the low-level stuff. But then why not just use fsockopen to begin with, it's already low-level.

These definitely need to be part of an interface

The only common things among the transports at this point and the foreseeable future are: 1. filepath, 2. filename, 3. mimetype. Even get_body or a more low-level read interface would only be used by fsockopen, again, unless you want to uproot the cURL transport (CURLOPT_READFUNCTION, huge nightmare).

get_headers() and get_body()

Pointless unless you want to use them inside CURLOPT_READFUNCTION. Otherwise the header and body structure needs low-level definitions for building the request data in fsockopen only.

To summarize:

1. Providing this flexible magic interface requires the addition of a whole new level of complexity into the cURL transport with little added benefit to the flexibility, "dumbing" cURL down to fsockopen's level.
2. Requests has survived for over 6 years on a very tight $data interface (string or array [that is flattened to a string either way]) and no file upload support.
3. All use cases for file uploads can be worked around by supplying a file path (use named pipes for callback-based data generators, for example) with very little work on the users part.

I feel this over-engineering is hindering a straightforward feature from landing and doing what it's supposed to do - upload a file. Decisions not options.

[dingo-d]

Any info about this feature? It would be a great addition.

I had an issue where the call towards an API passed when I used cURL with CURLOPT_POSTFIELDS, but would error out if I used wp_remote_request with the same value passed to the body argument.