use upload_tmp_dir as default for FILE_DIRECTORY_TEMP

I had the same problem when installed on a host with open_base_dir enabled (it should be on a decent hoster machine)...

Here is a patch for this, seems like a good idea to me, works on a new HEAD install on SUSe Linux.

change status

Works for me as well. Question though, Drupal Admin > Settings allows a user to specify a temp directory, does this interfere with that? This patch defines a tmp directory but Drupal Admin > Settings allows another one to be defined. Haven't looked into this but this could be a deeper issue.

If this isn't an issue, then +1 from me.

+1 - let's please get this fixed.

-define('FILE_DIRECTORY_TEMP', IS_WINDOWS ? 'c:\\windows\\temp' : '/tmp');
+define('FILE_DIRECTORY_TEMP', ini_get('upload_tmp_dir'));

Do we still need the define then? Can't we write ini_get('upload_tmp_dir') rather than FILE_DIRECTORY_TEMP? Opinions?

(Patch didn't apply against HEAD. Only 'garbage' was found.)

Dries: Agreed. Here's a patch that does just that. We can also remove the IS_WINDOWS define, as it's not used anywhere else.

Committed to HEAD. Thanks.

Switching status. eaton ran into a problem with this on IRC.

* upload_tmp_dir is not set by default in a normal PHP (see the NULL here)
* upload_tmp_dir can't be set with ini_get (same URL above).
* when upload_tmp_dir is NULL, admin > settings complains loudly.

After some further discussion with m3averick, we're leaning toward something like:

define('FILE_DIRECTORY_TEMP', ini_get('upload_tmp_dir) ? ini_get('upload_tmp_dir')
  : (IS_WINDOWS ? windows_temp() : '/tmp'))

where new function windows_temp() would run through stuff like $ENV{'TEMP'], $ENV{TMP}, is_dir(c:\\windows\\temp), is_dir(c:\\winnt\\temp) and so forth, attempting to find a valid location. The final default value should probably be something like files/temp (since there's no 'always gonna be there' /tmp on Windows, as per Linux).

Here's a first initial attempt per our discussions. I don't have a Windows machine, so i can't test it.

Attached is a new patch. I was returning too late.

Last one, based on IRC comments.

This should be the final version, checked against a Win32 machine and a eaton's Linux machine.

+1 works and is full of happy goodness.

Yup been reviewing with Morbus, appears to be working here now too. +1

The tmp_dir variable is declared static but recalculated every time file_tmp_dir() is called. Here's a patch that uses the static declaration to avoid recalculating the variable.

On the other hand... how useful is it to declare this variable static? I can't immediately see any paths through the code that can call this function twice for the same page view. Perhaps it would be better to remove the static declaration all together.

And... there's another way to structure the logic in file_tmp_dir().
This has the benefits of:
- being a little more readable, IMHO
- always checking that the directory returned exists.

function file_tmp_dir() {

  // Has PHP been set with an upload_tmp_dir?
  if (ini_get('upload_tmp_dir')) {
    $directories[] = ini_get('upload_tmp_dir');
  }

  // Determine based on operating system.
  if (substr(PHP_OS, 0, 3) == 'WIN') {
    $directories[] = $_ENV['TEMP'];
    // Windows env var TEMP may not exist, so try other common locations too.
    $directories[] = 'c:\\windows\\temp';
    $directories[] = 'c:\\winnt\\temp';
    $directories[] = variable_get('file_directory_path', 'files') . '\\tmp';
  }
  else {
    $directories[] = '/tmp';
  }

  foreach ($directories as $directory) {
    if (is_dir($directory)) {
      return $directory;
    }
  }

  return NULL;
}

I'm fine with tmpdir_3.patch.

That's what I meant to do in my original one (ie., test against the static first). The idea for the static is that calls to the file system are "heavy" and should be reduced as much as possible. I admit, though, that I didn't do any tests to see how many time the function would be called. As for your revised patch, I'm not a fan of it. You've added "files/temp" as a "does this directory exist?", when it deliberately never would - that was added just as a fallback on Win32. And the test against Linux /tmp is extraneous: we KNOW that directory will always exist. In essence, this routine should NEVER return a NULL value, otherwise, we'll just be back to eaton's problem on his Linux box: an ugly error message whenever you hit admin > settings.

Cleanup per Dries.

Alright, here's a new patch.

* discussion suggests that file system calls ARE NOT heavy, so I've gone with Archer's approach in #19.
* Dries suggested we just call file_directory_temp instead of variable_set. I've done this.
* I didn't like, however, the disparty between file_directory_temp and variable_get('file_directory_path').
* Thus, there's now a matching file_directory_path() that syncs up the API.
* files/tmp is now the default tmp directory when everything else fails.

Minor bug fix.

tested over here, patch is working!

Just to confirm, did a clean install of Drupal, database, etc... didn't realize how broken tmp directory was, it does *not* even work on HEAD! This *has* to be fixed before an RC goes out and it's working over here once again after this patch.

+1 for _26249_tmpdir.patch

I downloaded CVS today and couldn't configure it because it would never see the temp directory. With the patch everything looks good.

+1 here, too. I'm glad other people noticed how broken HEAD was without this patch... This week I've been setting up three sites for development work, and they're almost unusable without this.

+1 for the http://drupal.org/files/issues/_26249_tmpdir.patch from me too..

This is a SHOWSTOPPING error with the CVS version of Drupal. But the patch linked worked for me.

Dub

Committed to HEAD. Thanks.

If your module uses variable_get('file_directory_temp', ...) or variable_get('file_directory_path', ...) please update it to use the new API.

A list of affected modules:

acidfree/acidfree.module
audio/audio.module
banner/banner.module
browscap/browscap.module
carto/carto.module
cvslog/cvs.module
download/download.module
ecommerce/file/file.module
edit_template/edit_template.module
image/image.module
image_import/image_import.module
img_assist/img_assist.module
mail_archive/mail_archive.module
project/project.module
simpletest/tests/upload_tests.test
swish/swish.module
taxonomy_image/taxonomy_image.module
theme_editor/theme_editor.module
upload/upload.module
upload_indexer/upload_indexer.module
variables/example.module.txt
vimcolor/vimcolor.module
webform/webform_report.inc

Morbus: can you update the upgrade guide?

What are the odds of getting $path_delimiter replaced by the PHP constant PATH_SEPARATOR? Actually, it seems to me that we really should be using PATH_SEPARATOR instead of '/'.

I've got a patch to do this on on #38083.

danielc, you're right I ment DIRECTORY_SEPARATOR, the patch had the right code ;)

While PHP may be fine using both \ and / in Windows paths, some 3rd party PHP code isn't. I got screwed up trying to pass in the "incorrect"-ly formatted paths to one such library, hence my patch.

Module upgrade documentation updated. Closing.