FileSystem::tempnam() doesn't respect subdirectories for stream wrappers

Good catch, but this feels a little messy:

   if ($scheme && file_stream_wrapper_valid_scheme($scheme)) {
-    $wrapper = file_stream_wrapper_get_instance_by_scheme($scheme);
+    $local_directory = drupal_realpath($directory);
 
-    if ($filename = tempnam($wrapper->getDirectoryPath(), $prefix)) {
-      return $scheme . '://' . basename($filename);
+    if ($filename = tempnam(drupal_realpath($directory), $prefix)) {
+      // If the specified directory doesn't exist then PHP's tempname may return
+      // a file in the system temporary directory, but then we can't assume that
+      // we can just use the same name in our returned URI, so we return failure
+      // in this case.
+      if (dirname($filename) != $local_directory) {
+        return FALSE;
+      }
+      else {
+        $uri = $scheme .'://' . file_uri_target($directory) . '/' . basename($filename);
+        return file_stream_wrapper_uri_normalize($uri);
+      }
     }
     else {
       return FALSE;

I think we just want:

$local_directory = drupal_realpath($directory);
if (is_writable($local_directory) && ($filename = tempnam($local_directory, $prefix))) {
  return $directory . '/' . basename($filename);
}

Sounds like a bug we have in D8 as well, see #2304461: KernelTestBaseTNG™

Bug is still reproduced in latest DEV version of Drupal 7.
Patch is a little bit incorrect (2 'returns' after applying), but it works.

It works for us. I rerolled the patch.

Here's a reroll. I couldn't do an interdiff between this and #8 because it appears that the patch file in #8 is corrupt (Edit: because I wasn't used to the patch format. I added interdiffs in #14)

#11 doesn't use file_stream_wrapper_uri_normalize().

Here's interdiffs for 8-13 and 11-13

Marking as needs review.

I left the PHP doc blocks alone and didn't rename the test class. The new patch is very simple and focuses only on the task at hand. If doc blocks need updating and/or the test class needs to be renamed, I think those should be separate issues. Anyway, that's a minor thing.

Based on the test results from #13, it looks like the testbot had a malfunction. Re-marking as needs review and re-queueing test.

+++ b/modules/simpletest/tests/file.test
@@ -2766,4 +2766,24 @@ function testGetValidStreamScheme() {
+

Argh, I added an extra line on accident. Updating patch.

The patch in #19 looks good, applies cleanly, and executes the desired behavior.

RTBC++ that the patch is working for us. Thanks.

Don't we have this issue in Drupal 8 as well? See FileSystem::tempnam()

I've created a port of the patch for Drupal 8.

Updated the status for tests.

Looks good. The D8 version is missing tests so I ported the D7 ones.

If the directory does not exist or is not writable, tempnam() may generate a file in the system's temporary directory, and return the full path to that file, including its name.

I guess this behavior is documented in php.net, but not documented in Drupal. But the behavior is so confusing, it might be a good idea to document it here. It is after all a reasonable assumption, to expect that the path is not changed.

On doc change, it is better than before, but it might use @link definition. Also I think it could make clear that this function can return 3 different things, and that's the behavior of php tempnam(). I mean - it is written already, but it could be written in a way that's very hard to miss.

---

Is it correct to keep the php tempnam() behavior, or can we have something better? I think it doesn't need to keep to php tempnam(), but it's already in use, so now is a different matter.

---

    // NOTE: tempnam() can ignore the $realpath and use the system temp directory.
    return $scheme . '://' . $this->basename($temp_file_path);

How can we convert it to a proper scheme? Should we just fail if it isn't in the same scheme?

We can't be sure there even is a proper scheme. This may work:

if ($this->realpath('temporary://') === sys_get_temp_dir()) {
  $scheme = 'temporary';
}

Other things we can do is 1. return FALSE or 2. return $temp_file_path despite it not containing a scheme.

I think it is pretty good already. At the very least, much more sensible then what was before. I will try to find time to review and test this - maybe this weekend.

Edit1:
The last failing test may produce different results on different environments. I mean this:
$this->assertNotEquals($temporary_scheme . '://', $file_system->dirname($tempnam), 'Temporary file was created in the root of the temporary stream wrapper.');

in fact the two strings will be equal if sys_get_temp_dir() === $file_system->realpath('temporary://')

Edit2:
Actually from looking at the test, maybe you meant to do assertEquals.

About #41

Does this count as breaking change or not? Should it go into D9 or D8.6/8.5?

After this patch, there is a case when the method doesn't create a temporary file where it used to.

Maybe this change"should be added back as a "deprecated" feature that is behind a flag (controllable via an extra parameter)

That temporary file's path wasn't returned correctly so it was never useful as far as I know. But this will surely be reviewed by others, so we'll know if the method can't be changed. If we end up deprecating things, the whole method should be deprecated, and a better new one should be created in my opinion.

This patch is a re-roll from #11 against Drupal core 7.65

Can someone explain how the D7 patch is supposed to work?

It seems that the patch in #43 still doesn't return subdirectories because they are not being added in any way, and drupal_basename() is going to always only return a filename. I came here from #2402663: Excel and CSV export randomly empty since webform uses drupal_tempnam() and isn't returning the expected path when sending subdirectories to drupal_tempnam(). Here is a simple code sample I am using with devel that shows the subdirectory is not returned from drupal_tempnam() after applying this patch:

// webform_export_path set to private://tmp/webform_results in settings.php
$webform_export_path = variable_get('webform_export_path', 'temporary://');
dpm($webform_export_path);  // prints private://tmp/webform_results
dpm(drupal_tempnam($webform_export_path, 'webform_')); // prints private://webform_zlXpNF

Thanks.

This is a re-roll of #13.

This is a re-roll of #40.

Patch #49 with deprecated file_stream_wrapper_uri_normalize() replaced.

Patch #52 with deprecated FileSystem::validScheme() replaced.

Patch #54 with deprecated file_default_scheme() replaced.

Re-roll.

Deprecations fixed.

Is a version of this for 9.0.x required?

Last patch is working on 8.8.5, I really don't understand why it isn't added to core.

Thanks for all you people that are contributing to this, specially to @Liam Morland that keeps that alive from last year.

Re-roll for 9.1.x.

Re-roll.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. 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.

Created a MR from the patch in #69 and hid patch files.

Left some feedback. NW for that and coding standard issue.

Addressed feedback and coding standards.

Confirmed the test-only job correctly failed with:

Drupal\KernelTests\Core\File\StreamWrapperTest::testTempnam
Temporary file was created in the default stream wrapper subdirectory
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'public://vxdio1mu'
+'public://'

core/tests/Drupal/KernelTests/Core/File/StreamWrapperTest.php:174

NW for the test location.

The test cannot easily be moved because it uses ::createDirectory(), which is not available in any of FileSystem*Test.

Looking good to me.
MR is updated with latest main branch and is green.
I reviewed the changes and all the feedback seems addressed.

I reviewed the comment https://git.drupalcode.org/project/drupal/-/merge_requests/11681#note_48... related to moving class to `FileSystem*Test`
For testing, I tried to move test to `FileSystemTempDirectoryTest` and extended that class from `FileTestBase`
The test worked fine.
Although I am not quite sure whether we should move it to `FileSystemTempDirectoryTest` because based on name it seems like it is restricted to TempDirectoryTest.

Happy to update based on inputs from folks.
I'm keeping this in needs review for now.

Otherwise, all good from me.

Did another review and everything looks good. I'm happy not to move FileSystemTempDirectoryTest.

Nice old issue - I think we need to use Drupal's FileSystemComponent::getOsTemporaryDirectory(); rather than sys_get_temp_dir() - especially if you look at \Drupal\Core\StreamWrapper\TemporaryStream

... or maybe we should be using $this->getTempDirectory()