The current implementation is rather cumbersome since it has to try twice even if $uri is a local path with no drupal_realpath() and does not support $filename being a stream wrapper.

function drupal_move_uploaded_file($filename, $uri) {
  $result = @move_uploaded_file($filename, $uri);
  // PHP's move_uploaded_file() does not properly support streams if safe_mode
  // or open_basedir are enabled so if the move failed, try finding a real path
  // and retry the move operation.
  if (!$result) {
    if ($realpath = drupal_realpath($uri)) {
      $result = move_uploaded_file($filename, $realpath);
    }
    else {
      $result = move_uploaded_file($filename, $uri);
    }
  }

  return $result;
}

One may ask when $filename would be a stream wrapper, not normally. On Google Appengine (GAE) an upload proxy is used which intercepts file uploads before the PHP process is invoked and then passes a stream wrapper pointing to the file in Google Cloud Storage (GCS). So $filename may look something like gs://mybucket/somerandomfile. Of course Drupal will want to move the file to something like public://somerandomfile at which point PHP will fatal error since move_uploaded_file() [and rename()] do not support moving between two streams. This is rather unfortunate and I could see this proxy approach being useful outside of GAE (Amazon S3, etc).

Additionally, I believe this removes the need for the double invocation of move_uploaded_file(), but I have not tried variations of open_basedir.

Looking at the implementation of move_uploaded_file() it seems that a rename is performed if possible and otherwise a copy is performed. In order to keep this at a similar performance level a similar check should be performed [although removing the double invocation should help]. I am not sure if it would be best to do something like drupal_realpath() to see if stream wrapper or local path, or simply attempt a rename and check for success. My initial patch will simply always do copy() to see what the tests say and gauge reactions.

Also not sure if this will need to be bumped to 9.x release or not. Should functionally be equivalent, but simply adds an additional ability.

Files: 
CommentFileSizeAuthor
#4 2114885-move_upload_file.patch2.73 KBboombatower
PASSED: [[SimpleTest]]: [MySQL] 59,772 pass(es). View
#2 2114885-move_upload_file.patch2.43 KBboombatower
PASSED: [[SimpleTest]]: [MySQL] 59,291 pass(es). View
#1 2114885-move_upload_file.patch2.6 KBboombatower
PASSED: [[SimpleTest]]: [MySQL] 59,751 pass(es). View

Comments

boombatower’s picture

Status: Active » Needs review
FileSize
2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,751 pass(es). View
boombatower’s picture

FileSize
2.43 KB
PASSED: [[SimpleTest]]: [MySQL] 59,291 pass(es). View

Updated patch to support rename() where possible.

boombatower’s picture

If we still want to try and support

  // PHP's move_uploaded_file() does not properly support streams if safe_mode
  // or open_basedir are enabled so if the move failed, try finding a real path
  // and retry the move operation.

then perhaps it makes sense to explicitly check for those settings instead of blindly retrying? ini_get() and use drupal_realpath() flow?

boombatower’s picture

FileSize
2.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,772 pass(es). View

Perhaps something like this.

boombatower’s picture

Status: Needs review » Closed (won't fix)