Suggestions to make file/create_raw more developer friendly

I use the file/create_raw endpoint on my REST service so I can reduce file upload size and offload some necessary processing to my server. When I started using create_raw I encountered some things that blocked my progress and I'd like to suggest my modifications as points of improvement.

Empty $_FILES

It is assumed $_FILES['files']['name'] is set, which may result in an error when $_FILES is empty:

function _file_resource_create_raw() {
  ...
  foreach ($_FILES['files']['name'] as $field_name => $file_name) {

I have changed this to:

function _file_resource_create_raw() {
  ...
  if(!isset($_FILES['files']['name']))
    return services_error(t("Missing data the file upload can not be completed"), 500);

  foreach ...

HTML file upload field name

The foreach also assumes $_FILES['files']['name'] is an array. This can be very confusing, because sites like W3C instruct readers to use 'normal' names without square brackets when defining a single file upload field. What I mean with this is the following:

Example #1
<input type="file" name="files">

$_FILES content:

Array
(
    [files] => Array
        (
            [name] => flippy.jpg
            [type] => image/jpeg
            [tmp_name] => /opt/lampp/temp/phpshR6Ie
            [error] => 0
            [size] => 250166
        )
)

Example #2 (Correct HTML format)
<input type="file" name="files[]">

$_FILES content:

Array
(
    [files] => Array
        (
            [name] => Array
                (
                    [0] => flippy.jpg
                )
            [type] => Array
                (
                    [0] => image/jpeg
                )
            [tmp_name] => Array
                (
                    [0] => /opt/lampp/temp/phpACSNAu
                )
            [error] => Array
                (
                    [0] => 0
                )
            [size] => Array
                (
                    [0] => 250166
                )
        )
)

The first one results in an error, the second one works well. I was looking for a solution that wouldn't force a user to change his HTML form, I came up with this:

  ...
  if(!is_array($_FILES['files']['name'])) {
    $_FILES['files']['name'] = array($_FILES['files']['name']);
    $_FILES['files']['type'] = array($_FILES['files']['type']);
    $_FILES['files']['tmp_name'] = array($_FILES['files']['tmp_name']);
    $_FILES['files']['error'] = array($_FILES['files']['error']);
    $_FILES['files']['size'] = array($_FILES['files']['size']);
  }

  foreach ...

Redundant $validators variable

The _services_file_check_name_extension function will be called from the create_raw function. At the end of this function, it states return file_munge_filename("$name.$extension", $extensions);. After returning to the create_raw function the default file scheme will be decided, $validators will be set and file_save_upload will be called.

We set $validators by doing $validators['file_validate_extensions'] = $extensions;, but this is not the way this is supposed to go. Drupal core will throw several errors, because it is expecting the $extensions to be set on $...['..._ate_extensions'][0]. When it evaluates $validators, it will find a string instead of an array. So this piece of code should be rewritten to:

    $validators = array();
    if ($extensions = variable_get('services_allowed_extensions', SERVICES_ALLOWED_EXTENSIONS)) {
      $validators['file_validate_extensions'] = array();
      $validators['file_validate_extensions'][0] = $extensions;
    }

The Drupal errors:

    Warning: array_unshift() expects parameter 1 to be array, string given in file_validate() (line 1677 of /opt/lampp/htdocs/includes/file.inc).
    Warning: call_user_func_array() expects parameter 2 to be array, string given in file_validate() (line 1678 of /opt/lampp/htdocs/includes/file.inc).
    Warning: array_merge(): Argument #2 is not an array in file_validate() (line 1678 of /opt/lampp/htdocs/includes/file.inc).
    Warning: array_merge(): Argument #1 is not an array in file_validate() (line 1678 of /opt/lampp/htdocs/includes/file.inc).
    Warning: array_merge(): Argument #1 is not an array in file_validate() (line 1683 of /opt/lampp/htdocs/includes/file.inc).

The reason for my correction above is found deeper withing drupal core, inside the file_save_upload function. If we take a look at the source code we find:

  $extensions = '';
  if (isset($validators['file_validate_extensions'])) {
    if (isset($validators['file_validate_extensions'][0])) {
      // Build the list of non-munged extensions if the caller provided them.
      $extensions = $validators['file_validate_extensions'][0];
    }
    else {
      // If 'file_validate_extensions' is set and the list is empty then the
      // caller wants to allow any extension. In this case we have to remove the
      // validator or else it will reject all extensions.
      unset($validators['file_validate_extensions']);
    }
  }
  else {
    // No validator was provided, so add one using the default list.
    // Build a default non-munged safe list for file_munge_filename().
    $extensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp';
    $validators['file_validate_extensions'] = array();
    $validators['file_validate_extensions'][0] = $extensions;
  }

If you follow the path, you'll notice we will always end up at unset($validators['file_validate_extensions']);. This means that the following check will never evaluate true and the code inside the if-statement will never be executed when called from the create_raw function. But this isn't a problem, because _services_file_check_name_extension already calls file_munge_filename.

  if (!empty($extensions)) {
    // Munge the filename to protect against possible malicious extension hiding
    // within an unknown file type (ie: filename.html.foo).
    $file->filename = file_munge_filename($file->filename, $extensions);
  }

If we take this into consideration with my previous findings, to achieve the same functionality, we can rewrite this to:

    $validators = array('file_validate_extensions');
    $file = file_save_upload($field_name, $validators, "$scheme://");

In total

Applying these suggestions results in the following code:

function _file_resource_create_raw() {
  $files = array();
  
  if(!isset($_FILES['files']['name']))
    return services_error(t("Missing data the file upload can not be completed"), 500);
  
  if(!is_array($_FILES['files']['name'])) {
    $_FILES['files']['name'] = array($_FILES['files']['name']);
    $_FILES['files']['type'] = array($_FILES['files']['type']);
    $_FILES['files']['tmp_name'] = array($_FILES['files']['tmp_name']);
    $_FILES['files']['error'] = array($_FILES['files']['error']);
    $_FILES['files']['size'] = array($_FILES['files']['size']);
  }
  
  foreach ($_FILES['files']['name'] as $field_name => $file_name) {
    // Sanitize the user-input file name before saving to the file system.
    $_FILES['files']['name'][$field_name] = _services_file_check_name_extension($file_name);

    // file_save_upload() validates the file extension and name's length. File
    // size is limited by the upload_max_filesize directive in php.ini.
    $scheme = file_default_scheme();
    // Set file validators: allowed extension
    
    $validators = array('file_validate_extensions');
    $file = file_save_upload($field_name, $validators, "$scheme://");

PS: Sorry for the revision messages below, I don't know how to stop them.