Deprecate \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() and replace with sys_get_temp_dir()

> The whole component trying to maintain multi-os compatibility instead of using php defaults

That depends on whether the php defaults are actually sufficient and we'll need to test that.

All those low-level functions we have in the FileSystem class are to work around and expand on incomplete/missing features in the corresponding php functions.

@Berdir I've done a little research and found that this code was added 13 years ago (!) because there was an error on Windows 2000 and PHP 5.0.5-dev. Initial issue #26249 and it wasn't changed but only moved around.
Right now I think that there's no reason to keep it there. Seems we need to start getting rid of legacy in the core.

Would definitely be great to remove this if possible. I guess the first step is a patch and then try to get it tested on windows?

Discovered this issue while helping @mglaman and @mixologic figure out why the wrong tmp dir was being used in upgrade_status even though sys_temp_dir was being explicitly set.

$ php -d sys_temp_dir=/var/tmp vendor/bin/drush ev 'print \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory();'
/tmp

This is because \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() looks at these directories in order of preference:

• upload_tmp_dir
• /tmp (or Windows equivalent)
• sys_temp_dir

This feels like a bug to me, because if sys_temp_dir is set in php.ini I would expect any application to follow that - but if /tmp exists (and it almost always will on a Unix system) then it will always be used in preference.

Initial patch that just swaps usage with 'php://temp'. Let's see what breaks!

From what I've read, 'php://temp' is not a location, but will create a new one-time-use temp dir each time the stream is opened, and cannot be re-used.

Instead we want to use sys_get_temp_dir().

Here's a patch that does that.

Fix coding standards and adds a deprecation test.

+++ b/core/lib/Drupal/Component/FileSystem/FileSystem.php
@@ -13,34 +13,15 @@ class FileSystem {
    */
   public static function getOsTemporaryDirectory() {
-    $directories = [];
-
-    // Has PHP been set with an upload_tmp_dir?
-    if (ini_get('upload_tmp_dir')) {
-      $directories[] = ini_get('upload_tmp_dir');
-    }
-
-    // Operating system specific dirs.
-    if (substr(PHP_OS, 0, 3) == 'WIN') {
-      $directories[] = 'c:\\windows\\temp';
-      $directories[] = 'c:\\winnt\\temp';
-    }
-    else {
-      $directories[] = '/tmp';
-    }
-    // PHP may be able to find an alternative tmp directory.
-    $directories[] = sys_get_temp_dir();
-
-    foreach ($directories as $directory) {
-      if (is_dir($directory) && is_writable($directory)) {
-        // Both sys_get_temp_dir() and ini_get('upload_tmp_dir') can return paths
-        // with a trailing directory separator.
-        return rtrim($directory, DIRECTORY_SEPARATOR);
-      }
-    }
-    return FALSE;
+    @trigger_error(__METHOD__ . ' is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use sys_get_temp_dir() instead. See https://www.drupal.org/node/3225275', E_USER_DEPRECATED);
+    return sys_get_temp_dir();
   }
 

I think I'm pretty confused now.

Are we certain that we no longer need all those existing checks?

The trailing / inconsistency for example, and according to some comments on https://www.php.net/manual/en/function.sys-get-temp-dir.php, it might return a per-user path in some cases.

Also, https://www.php.net/manual/en/ini.core.php#ini.upload-tmp-dir does mention open_basedir, what about hosting setups that use that (is that still a thing these days?) and have a customized upload dir.

Feels like that might be a lot of work to verify all those things on all kinds of operationg systems to deprecate a handful lines of code?

I guess we are testing those assumptions? Some of the comments are quite old. For example, trailing slash seems to always be removed https://github.com/php/php-src/blob/af6c11c5f060870d052a2b765dc634d9e47d...

sys_temp_dir can be set in php.ini since PHP 5.5 and we check an empty value in \Drupal\Core\File\FileSystem::getTempDirectory().

Feels like that might be a lot of work to verify all those things on all kinds of operationg systems to deprecate a handful lines of code?

You might be right. 🤔

+1 for just deprecating the method.

+++ b/core/lib/Drupal/Component/FileSystem/FileSystem.php
@@ -13,34 +13,15 @@ class FileSystem {
-    $directories = [];
-
-    // Has PHP been set with an upload_tmp_dir?
-    if (ini_get('upload_tmp_dir')) {
-      $directories[] = ini_get('upload_tmp_dir');
-    }
-
-    // Operating system specific dirs.
-    if (substr(PHP_OS, 0, 3) == 'WIN') {
-      $directories[] = 'c:\\windows\\temp';
-      $directories[] = 'c:\\winnt\\temp';
-    }
-    else {
-      $directories[] = '/tmp';
-    }
-    // PHP may be able to find an alternative tmp directory.
-    $directories[] = sys_get_temp_dir();
-
-    foreach ($directories as $directory) {
-      if (is_dir($directory) && is_writable($directory)) {
-        // Both sys_get_temp_dir() and ini_get('upload_tmp_dir') can return paths
-        // with a trailing directory separator.
-        return rtrim($directory, DIRECTORY_SEPARATOR);
-      }
-    }
-    return FALSE;
...
+    return sys_get_temp_dir();

I think that changing what the method does might result in a BC break. Just doing a deprecation is enough.

The method has been deprecated.
Deprecation message test has been added.
All use of the code has been replaced.
It is not used in documentaton.
The testbot is happy.
The Is and the CR are in order.
For meit is RTBC.

I'm still uncertain about this, see #18. Just not removing the code in the existing method doesn't really address my concerns IMHO.

Are we _certain_ that those checks for the ini setting and windows aren't required anymore? Did we test the impact of this patch on Windows?

What exactly do we gain from removing and deprecating this code? Is it worth the risk of possibly breaking someones hosting setup?

Let's get this manually tested on windows.

Unfortunately missed the D10 window

And still needs windows testing. (Can't do as I'm on a mac)

Added patch against #26 in 10.1 version

Made changes as per comment #35.

Raised a MR against 10.x branch from the patch in #26, addressed #35 and #37.

Came here for something else, decided to do a code review instead. 😇

Merge request and changes looks good! @Bhanu951

Still needs review for the manual Windows testing mentioned in #33 also on a Mac/Linux here.

Asked around and can't find anyone on a windows to do testing.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I have a Windows VM on which I could test it. Although it's not the "real deal" of course.

Given the issue summary of #3225473: Use \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead of sys_get_temp_dir() where possible I'm closing this in favour of that.