Deprecate file_directory_temp() and move to FileSystem service

Maybe it needs separate CR?
Otoh extending https://www.drupal.org/node/3006851 means to keep all FS lated changes easy searchable

1. +++ b/core/includes/file.inc
   @@ -146,9 +145,16 @@ function file_uri_target($uri) {
   +  @trigger_error('file_default_scheme() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemConfigInterface::defaultScheme() instead. See https://www.drupal.org/node/3006851.', E_USER_DEPRECATED);
   

   Same here with Drupal 8.7.0 instead of 8.8.0.

2. +++ b/core/includes/file.inc
   @@ -1167,30 +1173,16 @@ function drupal_tempnam($directory, $prefix) {
   + * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
   

   It won't be released in Drupal 8.7.0. It will be released with 8.8.0 at earliest.

3. +++ b/core/lib/Drupal/Core/File/FileSystemConfigInterface.php
   @@ -0,0 +1,26 @@
   +   * @return mixed|null
   

   Returning "mixed" is really bad I think. It should be string|null. That also says the comment below.

4. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -62,6 +63,13 @@ class ConfigController implements ContainerInjectionInterface {
   +   * The file system config.
   

   Why are you adding this attribute comment? The object type is already known by the constructor comment.

5. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -91,14 +100,17 @@ public static function create(ContainerInterface $container) {
   +  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage, ConfigManagerInterface $config_manager, FileDownloadController $file_download_controller, DiffFormatter $diff_formatter, FileSystemInterface $file_system, FileSystemConfigInterface $file_system_config) {
   

   There are 7 dependencies injected. Wouldn't it be possible to re-think them and maybe encapsulate wisely, so there will be fewer?

6. +++ b/core/modules/system/system.install
   @@ -705,7 +705,7 @@ function system_requirements($phase) {
   +      if ($phase == 'install' || \Drupal::service('file_system_config')->defaultScheme() == 'public') {
   

   Please write 3 equal signs. Because that way the data type will be also checked.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -283,7 +283,7 @@ function hook_modules_uninstalled($modules) {
    function hook_uninstall() {
      // Remove the styles directory and generated images.
   -  \Drupal::service('file_system')->deleteRecursive(file_default_scheme() . '://styles');
   +  \Drupal::service('file_system')->deleteRecursive(\Drupal::service('file_system_config')->defaultScheme() . '://styles');
    }
   

   Totally out of scope, but since I'm seeing it... WTF? ;) What if you change your default scheme config between when you install and uninstall this thing? ;)

2. +++ b/core/lib/Drupal/Core/File/FileSystemConfigInterface.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * Gets the default file stream implementation.
   +   *
   +   * @return string
   +   *   'public', 'private' or any other file scheme defined as the default.
   +   */
   +  public function defaultScheme();
   

   Should this method's name start with the verb "get", as in getDefaultScheme()?

3. +++ b/core/lib/Drupal/Core/File/FileSystemConfigInterface.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * Gets and sets the path of the configured temporary directory.
   +   *
   +   * @return string
   +   *   A string containing the path to the temporary directory.
   +   */
   +  public function tempDirectory();
   

   "Gets and sets"... huh? ;) Does it get or set? If it takes no arguments, how is it setting anything? Weird API. Again, I'd rather have a verb in the function name.

   Looking at the old file_directory_temp() implementation, I see it indeed does both, and perhaps untangling that is "out of scope". But if we're deprecating this and moving it into a "clean" service, isn't this the perfect time/issue to make this not so weird?

4. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -543,13 +543,11 @@ protected function fileUriTarget($uri) {
   -   * @todo: Convert file_default_scheme() into a proper injectable service.
   -   *
       * @return string
       *   'public', 'private' or any other file scheme defined as the default.
       */
      protected function fileDefaultScheme() {
   -    return file_default_scheme();
   +    return \Drupal::service('file_system_config')->defaultScheme();
      }
   

   Is it fair to remove this @todo if we're still calling \Drupal::service for this? Don't we need to actually inject that service properly?

Re: #9, thanks!

1. Needs follow-up? ;) Those docs are seriously bad. We shouldn't be recommending any of this.

2. Great! Much better IMHO.

3. Hrm, yeah, I guess I'm just bothered that calling getTempDirectory() has the side effect of (what should be) initializeTempDirectory() if it there's no temp dir configured. :( It's one thing to have a "get" method that offers a fallback if the thing you're trying to get isn't set. This seems like a bit of a different beast. I wonder if we should actually move all that code into a separate initializeTempDirectory() method? But yeah, if getTempDirectory() calls initializeTempDirectory() on demand, it's not really any better than what we have now. Not sure if there's a better place to call initializeTempDirectory(). Maybe I should stop caring and call this Good Enough(tm). ;)

4. Hrm, yeah, I see what you mean. That file is full of \Drupal::service() already. Okay, I retract my concern -- out of scope, I guess.

If it weren't for my lingering doubts about point 3, I'd call this RTBC. I guess someone else can do that. ;)

Thanks,
-Derek

p.s. #3038988: Remove support for empty destination in FileSystem::copy(), FileSystem::move() and FileSystem::saveData() calls file_default_scheme(), so if this lands first, that'll need a re-roll. If that lands first, this will need to fix that call site, too.

Sounds like a good plan to manage the re-roll dependencies.

Meanwhile, FYI: #8.1 is now at #3039296: Fix examples in hook_install() and hook_uninstall().

Cheers,
-Derek

Blocker is in

Assigned for review.

This should be 8.8.x.

1. +++ b/core/lib/Drupal/Core/File/FileSystemConfig.php
   @@ -0,0 +1,66 @@
   +/**
   + * Provides file system configuration.
   + */
   +class FileSystemConfig implements FileSystemConfigInterface {
   
   +++ b/core/lib/Drupal/Core/File/FileSystemConfigInterface.php
   @@ -0,0 +1,30 @@
   + * Provides and interface for file system configuration.
   

   The documentation for this class isn't really particularily helpful, maybe talking about what this interface does would be helpful.

   s/and/an

   Could we also mark this internal so this patch doesn't increase the API surface?

2. +++ b/core/lib/Drupal/Core/File/FileSystemConfigInterface.php
   @@ -0,0 +1,30 @@
   +/**
   + * Provides and interface for file system configuration.
   + */
   +interface FileSystemConfigInterface {
   

   How about adding a @ingroup file, so it is referenced in https://api.drupal.org/api/drupal/core%21includes%21file.inc/group/file/...

3. +++ b/core/modules/config/tests/src/Functional/ConfigExportUITest.php
   @@ -57,7 +57,9 @@ public function testExport() {
   +    /** @var \Drupal\Core\File\FileSystemConfigInterface $file_system_config */
   +    $file_system_config = \Drupal::service('file_system_config');
   

   I wonder whether we could use something like assert($object instanceof ResourceIdentifier);

+++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
@@ -59,7 +59,9 @@ public function testFileCheckLocalDirectoryHandling() {
+    /** @var \Drupal\Core\File\FileSystemConfigInterface $file_system_config */
+    $file_system_config = \Drupal::service('file_system_config');
+    $directory = $file_system_config->getDefaultScheme() . '://' . $this->randomMachineName() . '/' . $this->randomMachineName();

Here is another place where we could use assert()

1. +++ b/core/includes/file.inc
   @@ -146,9 +145,16 @@ function file_uri_target($uri) {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\File\FileSystemConfigInterface::getDefaultScheme() instead.
   
   @@ -1183,30 +1189,16 @@ function drupal_tempnam($directory, $prefix) {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\File\FileSystemConfigInterface::getTempDirectory() instead.
   

   Exceeds 80 chars. Move "Use" one line up.

2. +++ b/core/includes/file.inc
   @@ -146,9 +145,16 @@ function file_uri_target($uri) {
   +  @trigger_error('file_default_scheme() is deprecated in drupal:8.8.0 and will be removed before drupal:9.0.0. Use \Drupal\Core\File\FileSystemConfigInterface::getDefaultScheme() instead. See https://www.drupal.org/node/3039255.', E_USER_DEPRECATED);
   
   @@ -1183,30 +1189,16 @@ function drupal_tempnam($directory, $prefix) {
   +  @trigger_error('file_directory_temp() is deprecated in drupal:8.8.0 and will be removed before drupal:9.0.0. Use \Drupal\Core\File\FileSystemConfigInterface::getTempDirectory() instead. See https://www.drupal.org/node/3039255.', E_USER_DEPRECATED);
   

   Should not end with period after "See {url}". There was a discussion here #3024461: Adopt consistent deprecation format for core and contrib deprecation messages.

3. +++ b/core/lib/Drupal/Core/File/FileSystemConfig.php
   @@ -0,0 +1,66 @@
   +      $config->set('path.temporary', (string) $temporary_directory)->save(TRUE);
   

   It's weird that we do a "set" within a getter. However, casting $temporary_directory to (string) seems superfluous to me as the variable is always a string.

4. +++ b/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php
   @@ -37,7 +37,7 @@ public function getDescription() {
   +    return \Drupal::service('file_system_config')->getTempDirectory();
   

   Why not injecting the new service?

5. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -114,6 +125,10 @@ public function __construct(StorageInterface $target_storage, StorageInterface $
   +    if (is_null($file_system_config)) {
   +      @trigger_error('Calling ConfigController::__construct() without the $file_system_config argument is deprecated in drupal:8.8.0. The $file_system_config argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3039255', E_USER_DEPRECATED);
   +    }
   

   If we had tested this, we would have detected the error. Tests are good :) The $file_system_config is not set. Let's also test the deprecation.

6. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -93,6 +103,11 @@ public function __construct(ConfigFactoryInterface $config_factory, ModuleHandle
   +    if (!$file_system_config) {
   +      @trigger_error('Calling ThemeSettingsForm::__construct() without the $file_system_config argument is deprecated in drupal:8.7.0. The $entity_repository argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3039255', E_USER_DEPRECATED);
   +      $file_system_config = \Drupal::service('$file_system_config');
   +    }
   

   Let's test this too.

7. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
   @@ -92,12 +95,12 @@ public function testFileCheckDirectoryHandling() {
   +    @drupal_unlink($file_system_config->getDefaultScheme() . '://.htaccess');
   

   As we've touched this line, why still using the deprecated function?

8. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
   @@ -92,12 +95,12 @@ public function testFileCheckDirectoryHandling() {
   +    $this->assertFalse(is_file($file_system_config->getDefaultScheme() . '://.htaccess'), 'Successfully removed the .htaccess file in the files directory.', 'File');
   ...
   +    $this->assertTrue(is_file($file_system_config->getDefaultScheme() . '://.htaccess'), 'Successfully re-created the .htaccess file in the files directory.', 'File');
   

   There is assertFileExists() & assertFileNotExists().

9. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
   @@ -164,14 +167,14 @@ public function testFileDestination() {
   +    $this->assertEqual(empty($tmp_directory), FALSE, 'getTempDirectory() returned a non-empty value.');
   

   assertNotEmpty()

The CR branch should be also incremented to 8.8,x.

Updated latest patch according latest comments.
Added interdiff file.

1. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -114,6 +125,11 @@ public function __construct(StorageInterface $target_storage, StorageInterface $
   +    if (is_null($file_system_config)) {
   +      @trigger_error('Calling ConfigController::__construct() without the $file_system_config argument is deprecated in drupal:8.8.0. The $file_system_config argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3039255', E_USER_DEPRECATED);
   +      $this->fileSystemConfig = \Drupal::service('file_system_config');
   +    }
   +    $this->fileSystemConfig = $file_system_config;
   
   +++ b/core/modules/config/tests/src/Kernel/Controller/ConfigControllerDeprecationTest.php
   @@ -0,0 +1,35 @@
   +    $this->assertNotNull(new ConfigController($target_storage->reveal(), $source_storage->reveal(), $config_manager->reveal(), $file_download_controller->reveal(), $diff_formatter->reveal(), $file_system->reveal()));
   

   This is still not correct: $this->fileSystemConfig will be overridden with NULL just after exiting the if (...) block. It should be $file_system_config = \Drupal::service('file_system_config');. Unfortunately, the test is not catching this. Replace the assertion with:

       $config_controller = new ConfigController($target_storage->reveal(), $source_storage->reveal(), $config_manager->reveal(), $file_download_controller->reveal(), $diff_formatter->reveal(), $file_system->reveal());
       $this->assertNotNull($config_controller);
       $config_controller->downloadExport();
   

   ... and you will see the error. If you add the suggested lines in test, probably it should be a kernel test because involves also other services.

2. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -93,6 +103,11 @@ public function __construct(ConfigFactoryInterface $config_factory, ModuleHandle
   +      @trigger_error('Calling ThemeSettingsForm::__construct() without the $file_system_config argument is deprecated in drupal:8.7.0. The $entity_repository argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3039255', E_USER_DEPRECATED);
   
   +++ b/core/modules/system/tests/src/Kernel/Form/ThemeSettingsFormDeprecationTest.php
   @@ -0,0 +1,35 @@
   +   * @expectedDeprecation Calling ThemeSettingsForm::__construct() without the $file_system_config argument is deprecated in drupal:8.7.0. The $entity_repository argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3039255
   

   s/8.7.0/8.8.0

1. +++ b/core/includes/file.inc
   @@ -579,7 +586,7 @@ function file_unmanaged_prepare($source, &$destination = NULL, $replace = FILE_E
     */
    function file_build_uri($path) {
   -  $uri = file_default_scheme() . '://' . $path;
   +  $uri = \Drupal::service('file_system_config')->getDefaultScheme() . '://' . $path;
      return file_stream_wrapper_uri_normalize($uri);
   

   what are we going to do with this thing?

2. +++ b/core/lib/Drupal/Core/File/FileSystemConfig.php
   @@ -0,0 +1,66 @@
   +  public function getTempDirectory() {
   +    $temporary_directory = $this->configFactory->get('system.file')->get('path.temporary');
   +    if (empty($temporary_directory)) {
   +      // Needs set up.
   +      $config = $this->configFactory->getEditable('system.file');
   +      $temporary_directory = FileSystem::getOsTemporaryDirectory();
   +
   +      if (empty($temporary_directory)) {
   

   What's the plan now with this and #3047257: File system config should be container parameters? Woudl we keep this as public API, but move the tmp directory to a setting? Pretty sure there's already an issue for that, btw.

3. +++ b/core/modules/image/image.install
   @@ -13,7 +13,7 @@
     */
    function image_install() {
      // Create the styles directory and ensure it's writable.
   -  $directory = file_default_scheme() . '://styles';
   +  $directory = \Drupal::service('file_system_config')->getDefaultScheme() . '://styles';
      \Drupal::service('file_system')->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
    }
   

   I find it interesting that an issue was opened to have a better example for hook install/uninstall due to that being problematic, but the actual implementation where it was copied from was not really discussed :)

   I'm not sure the write part is even really necessary. We're dynamically writing nested folders anyway for the actual image styles with their nested paths and everything, why do we need to pre-create the styles folder?

4. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -215,7 +215,7 @@ public function testRenderWithTheme() {
       */
      public function testFileDefaultScheme() {
   -    $this->assertEquals('public', file_default_scheme());
   +    $this->assertEquals('public', \Drupal::service('file_system_config')->getDefaultScheme());
        $this->assertEquals('public', \Drupal::config('system.file')->get('default_scheme'));
      }
   

   somewhat strange that the "helper method" to get the config value is actually longer than getting it directly from config now :)

> If we also replace usages of getDefaultScheme with Settings::get('default_scheme') we don't need to add a new FileSystemConfig service.

Not quite sure about this part, since it's not actually environment-depending.

Also not quite sure yet how the BC for that setting would look like and if that really could be in FileSystem. I guess we could fall back to a non-injected config call?

Another thing to keep in mind is that using Settings means we can't write it back like do now, so there's a certain overhead of having to find it every time we access it. There are also issues like #2817677: Can't remove "Temporary Directoy" path where people are confused about that.

I really thought we had an existing issue about that, but couldn't find it. I did find some related issues:

@kim.pepper:
First of all: thanks for your contribution! :)

In core/lib/Drupal/Core/File/FileSystem.php the ConfigFactory was not injected, but taken via the service container. The dependency injection principle should be followed consistently, if you define a service via a service.yml file. I have implemented that.

In file_directory_temp() you have specified "mixed|null" as return type via the PHPDoc comment. The interface at core/lib/Drupal/Core/File/FileSystemInterface.php specifies a string here. "mixed" is always a trashy PHPDoc return type which is leading nowhere.

What I am not sure: The key-value array pairs should be formatted in columns. Your arrays are not formatted. E.g. for correct formatting:

$array = [
    'key'    => 'value'
    'key-1' => 'some value'
];

Everything else looks okay to me. Please review.

We didn't do that on purpose, as you can see, everything explodes if you try that.

What we should do is better document *why* we don't do that. Because of the dependency issues and because it is only a BC layer, 9.x will then most likely remove that.

@Berdir: Looks like the dependency is not injected correctly on all necessary places. IMHO the dependency should be simply injected correctly. Instead of a lame comment, which is telling that we have been too dump to do it correctly.

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -633,4 +635,39 @@ public function createFilename($basename, $directory) {
+    $temporary_directory = $this->settings->get('file_temp_path');

This is not defined in default.settings.php and I'm unable to find the string 'file_temp_path' anywhere in the HEAD. If we are adding a new settings key then let's add documentation and sample default value.

Thanks, for fixing the feedback. Let's update the change notice with this info as well.

+++ b/sites/default/default.settings.php
@@ -536,6 +536,17 @@
+ * must exist and we writable by Drupal.

s/we/be

This looks ready IMO. Only needs the change notice update as per #62.

1. +++ b/sites/default/default.settings.php
   @@ -536,6 +536,17 @@
   + * Private temporary path:
   

   I'm not sure about the word private starting this off. It makes it sound as though it is for private files. I think the important details are that typically this does not need to be set and should not be accessible to the web server.

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -633,4 +635,39 @@ public function createFilename($basename, $directory) {
   +    // Fallback to config for Backwards compatibility.
   +    // This service is lazy-loaded and not injected, as the file_system service
   +    // is used in the install phase before config_factory service exists. It
   +    // will be removed before Drupal 9.0.0.
   +    $temporary_directory = \Drupal::config('system.file')->get('path.temporary');
   +    if (!empty($temporary_directory)) {
   +      return $temporary_directory;
   +    }
   

   This should trigger a deprecation error. We also need to remove path.temporary from the default config. And we probably need a system_requirements() to telling people to add a line to settings.php if they've customised it.

   We also can provide an upgrade path and remove system.file:path.temporary is it matches the OS determined default.

3. One point that should be made is that this change make Drupal more portable.

+1 to get rid of config setting, moreover instead of so many "fallbacks" I'd prefer php://temp https://www.php.net/manual/en/wrappers.php.php#wrappers.php.memory
So the only difference against private files will be presence of fallback to php://temp - easy to undeestand and just one option in checklist - "temp files exists in settings"

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -633,4 +635,39 @@ public function createFilename($basename, $directory) {
+    // Fallback to OS default.
+    $temporary_directory = FileSystemComponent::getOsTemporaryDirectory();
...
+    if (empty($temporary_directory)) {
+      // If no directory has been found default to 'files/tmp'.
+      $temporary_directory = PublicStream::basePath() . '/tmp';

I wonder why so many "fallbacks" if the approach should be same for every stream wrapper

Just wanna check what is changed

@kim.pepper Re #69 sounds like and looks like separate issue to remove component cos regexp stuff not so much related to filesystem, so filed #3063704: Deprecate \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() and replace with sys_get_temp_dir()

1. +++ b/core/includes/file.inc
   @@ -15,7 +15,7 @@
    use Drupal\Core\Site\Settings;
    use Drupal\Core\StreamWrapper\PrivateStream;
   -use Drupal\Core\StreamWrapper\PublicStream;
   +
    use Drupal\Core\StreamWrapper\StreamWrapperManager;
    
   

   nitpick: empty line added between uses.

2. +++ b/core/includes/file.inc
   @@ -1203,30 +1203,17 @@ function drupal_tempnam($directory, $prefix) {
   + *
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\File\FileSystemInterface::getTempDirectory()
   + *   instead.
   + *
   + * @see \Drupal\Core\File\FileSystemInterface::getTempDirectory()
   + * @see https://www.drupal.org/node/3039255
     */
    function file_directory_temp() {
   

   new wording is removed from drupal:9.0.0 I believe?

3. +++ b/core/includes/install.core.inc
   @@ -1098,21 +1097,6 @@ function install_base_system(&$install_state) {
    
   -  // Prevent the installer from using the system temporary directory after the
   -  // system module has been installed.
   -  if (drupal_valid_test_ua()) {
   -    // While the temporary directory could be preset/enforced in settings.php
   -    // like the public files directory, some tests expect it to be configurable
   -    // in the UI. If declared in settings.php, they would no longer be
   

   I suppose what we need to do instead is write this value to settings.php, which should be writable during the installer?

4. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -625,4 +627,40 @@ public function createFilename($basename, $directory) {
   +    }
   +
   +    // Fallback to config for Backwards compatibility.
   +    // This service is lazy-loaded and not injected, as the file_system service
   +    // is used in the install phase before config_factory service exists. It
   +    // will be removed before Drupal 9.0.0.
   +    $temporary_directory = \Drupal::config('system.file')->get('path.temporary');
   

   Should we in that case also be extra careful and add a Drupal::hasContainer() check and skip the fallback if not?

5. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -86,6 +86,10 @@ protected function prepareSettings() {
        ];
   +    $settings['settings']['file_temp_path'] = (object) [
   +      'value' => $this->tempFilesDirectory,
   +      'required' => TRUE,
   +    ];
   

   Yes, something like this test we'd also need in the installer itself, for test that are testing the installer.

6. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
   @@ -101,9 +102,11 @@ public function testFiles() {
          ->execute();
   +    $file_system = \Drupal::service('file_system');
   +    assert($file_system instanceof FileSystemInterface);
        Database::getConnection('default', 'migrate')
          ->update('variable')
   -      ->fields(['value' => serialize(file_directory_temp())])
   +      ->fields(['value' => serialize($file_system->getTempDirectory())])
          ->condition('name', 'file_directory_temp')
          ->execute();
   

   Interesting. Considering that we are deprecating this, seems like we should also remove/deprecate this migration?

7. +++ b/core/modules/system/system.install
   @@ -529,6 +529,28 @@ function system_requirements($phase) {
   +        'value' => 'Deprecated configuration',
   +        'description' => [
   +          [
   +            '#markup' => t('You are using deprecated configuration for the temporary files path.'),
   +            '#suffix' => ' ',
   +          ],
   +          [
   +            '#markup' => t('Remove the system.files.path.temporary config to use the OS default, or set a custom value in settings.php.'),
   +            '#suffix' => ' ',
   +          ],
   +        ],
   +      ];
   +    }
   

   Maybe we can be a bit more helpful here? Like, we could actually print out the exact statement that you need to add? And in case it matches the default value that we'd get, we could even say it can be removed safely?

8. +++ b/core/modules/system/system.install
   @@ -617,8 +639,8 @@ function system_requirements($phase) {
        }
   -    if (!empty($GLOBALS['config']['system.file']['path']['temporary'])) {
   -      $directories[] = $GLOBALS['config']['system.file']['path']['temporary'];
   +    if (!empty(Settings::get('file_temp_path'))) {
   +      $directories[] = Settings::get('file_temp_path');
        }
        else {
   

   The !empty() isn't necessary anymore, we can just do if (Settings::get()).

   This part is a bit tricky however, in that existing processes might rely on setting this in settings.php and we would now break it, without BC. That said, installer is technically excluded from BC, so this is probably too? But even we decide to explicitly drop it, we could be "nice" and throw an exception if we see that the value is set?

9. +++ b/core/modules/system/system.install
   @@ -2287,3 +2309,14 @@ function system_update_8702() {
   + */
   +function system_update_8801() {
   +  $config = Drupal::configFactory()->getEditable('system.file');
   +  if ($config->get('path.temporary') === FileSystem::getOsTemporaryDirectory()) {
   +    $config->clear('path.temporary')
   +      ->save();
   

   Ah, so you're removing it here, which I guess means we don't have to check for it being equal in the requirements hook, fair :)

10. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
    @@ -169,12 +170,9 @@ public function testFileDestination() {
        * Ensure that the file_directory_temp() function always returns a value.
        */
       public function testFileDirectoryTemp() {
    

    comment still uses the deprecated function nime, maybe we could add a covers annotation and just use something like "Tests default temporary path" or so?

    Not 100% sure whether kernel tests have or don't have a temporary path set, though. Maybe this code is a left-over from when it wasn't a kernel test, maybe it actually was set somewhere and we should convert that to settings to avoid fun race conditions?

11. +++ b/sites/default/default.settings.php
    @@ -536,6 +536,20 @@
    + * Note: Caches need to be cleared when this value is changed to make the
    + * temporary:// stream wrapper available to the system.
    

    lies!

    that was copied from private://, but it doesn't apply here.

+++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
@@ -102,13 +102,6 @@ public function testFiles() {
       ->condition('name', 'file_directory_path')
       ->execute();
-    $file_system = \Drupal::service('file_system');
-    assert($file_system instanceof FileSystemInterface);
-    Database::getConnection('default', 'migrate')
-      ->update('variable')
-      ->fields(['value' => serialize($file_system->getTempDirectory())])
-      ->condition('name', 'file_directory_temp')
-      ->execute();
 

I guess if there's a test there's also a migration defined for this somewhere?

3. Would have assumed in the same place, not sure?
5. That was just a reference to 3, that we need to do something like that there.
10. We have quite a few examples of kernel tests with @covers stuff too, for example \Drupal\KernelTests\Core\Path\AliasStorageTest. My understanding is that it specifically for cases where a test calls multiple things, because then code coverage knows only to mark those parts as covered and not any other code that we happen to call but don't assert for.

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -274,7 +274,7 @@ public function tempnam($directory, $prefix) {
 
-      if ($filename = tempnam($wrapper->getDirectoryPath(), $prefix)) {
+      if ($filename = @tempnam($wrapper->getDirectoryPath(), $prefix)) {
         return $scheme . '://' . static::basename($filename);
       }

I suspect this is caused by having an invalid temporary directory configuration, so we shouldn't add @ but figure out why that happens.

Just a quick nitpick:

1. +++ b/core/includes/file.inc
   @@ -1195,30 +1194,16 @@ function drupal_tempnam($directory, $prefix) {
   + * @deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Use
   + *   \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
   

   Nitpick: This should be "8.0.0" to "8.8.0"

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -272,7 +274,7 @@ public function tempnam($directory, $prefix) {
   -      if ($filename = tempnam($wrapper->getDirectoryPath(), $prefix)) {
   +      if ($filename = @tempnam($wrapper->getDirectoryPath(), $prefix)) {
            return $scheme . '://' . static::basename($filename);
   

   In doubt we should at least document why this might fail

Did a little research about the history of this method and found that it was added 14 years ago as a fix for Win2000 and PHP 5.0.5-dev, probably it's a legacy that can be removed. Initial issue #26249 and it wasn't changed but only moved around.

Ahhh, strolling down memory lane. Remember when "change records" were like comment #26249-29: use upload_tmp_dir as default for FILE_DIRECTORY_TEMP and when all of contrib lived in a single CVS repo that we'd grep through when looking for what we'd break with a given API change? ;)

Found some problems that I'm attempting to fix, we'll see how that goes.

* The updated config tar.gz files had the files inside a "." folder, had to re-create them with e.g. tar -czvf ../testing_config_install.tar.gz *
* The update problem was that the filled dump had /tmp as the configured temp directory, that was unset and then it fell back to default temp folder set up by the test environment, but that wasn't created because it doesn't run with that path in system_requirements() before the updates (or something like that, I suppose). The non-filled dumps probably don't have that set, so it always uses the fallback. I'm not too happy about how this is done now, but not sure where else it should be.
* I also tested to add back the removed code in install.core.inc, but I think the change in #76 is a valid alternative that seems more consistent with the other directories, so leaving that out for now. We might need to make sure this doesn't introduce random fails or something like that.

Something I didn't adjust yet but I think we should is also check the file_temp_path setting in system_update_8801(), because that allows sites to set that already *before* they update (e.g. dynamically based on environment variables like that's common on hosting platforms like platform.sh, then the update will already remove the old configuration if that was set. Likely even remove it if any value is set, although that will make an update test tricky, as this is always set in tests and we can't test that it is kept...

Oops.

1. +++ b/core/includes/file.inc
   @@ -1180,13 +1179,13 @@ function drupal_rmdir($uri, $context = NULL) {
     *
   - * @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
     *   Use \Drupal\Core\File\FileSystem::tempnam().
     *
     * @see https://www.drupal.org/node/2418133
     */
    function drupal_tempnam($directory, $prefix) {
   -  @trigger_error('tempnam() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemInterface::tempnam(). See https://www.drupal.org/node/2418133.', E_USER_DEPRECATED);
   +  @trigger_error('tempnam() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\File\FileSystemInterface::tempnam(). See https://www.drupal.org/node/2418133.', E_USER_DEPRECATED);
      return \Drupal::service('file_system')->tempnam($directory, $prefix);
   

   no need to update this to 8.8.0?

2. +++ b/core/includes/file.inc
   @@ -1195,30 +1194,16 @@ function drupal_tempnam($directory, $prefix) {
   + *
   + * @deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Use
   + *   \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
   

   this deprecation on the other hand should be 8.8.0 as it is a new deprecation.

3. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -625,4 +627,42 @@ public function createFilename($basename, $directory) {
   +  public function getTempDirectory() {
   +    // Use settings.
   +    $temporary_directory = $this->settings->get('file_temp_path');
   +    if (!empty($temporary_directory)) {
   +      return $temporary_directory;
   

   Since we don't even check for a configured value if settings is set, I think we should do what I commented. If any settings value is set, drop the configuration key in the update function.

   We also need a test for this. As I wrote, if we also drop it when any settings value is set then it might be tricky to test a negative case when it's not deleted. But we could at least test the deletion, and separately, also test the requirements message if it is set?

4. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
   @@ -101,11 +101,6 @@ public function testFiles() {
          ->execute();
   -    Database::getConnection('default', 'migrate')
   -      ->update('variable')
   -      ->fields(['value' => serialize(file_directory_temp())])
   -      ->condition('name', 'file_directory_temp')
   -      ->execute();
    
        $this->executeMigration('d6_file');
   

   Not quite sure about this part yet. Maybe it would need the same logic as the update function? migrate the old value only if it doesn't match the default temp path or configured settings path, which would then trigger the deprecation? Then again, what's even the point of migrating this, either it is the system default, or if not, then it's likely actually not correct for the D8 site as it is installed in a different location.

5. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -190,6 +190,9 @@ protected function setUp() {
    
   +    // Ensure the configured temp directory exist and is writable.
   +    \Drupal::service('file_system')->prepareDirectory($this->tempFilesDirectory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
   

   my comment doesn't make too much sense yet, it's not (yet) configured.. maybe the "default temp directory" and explicitly mention that the configured one might be unset during the update?

> Updated the update hook. I might need some direction on the best way to add a test for this.

The "delete" case should be pretty easy, add an assertion to the update filled test to ensure that the config key is no longer set after the update. And possibly check if it is set before, but we might need to directly access the database for that.

1. Need to remove the config from config/system.file.yml - and its schema.

2. +++ b/core/modules/system/system.install
   @@ -529,6 +529,36 @@ function system_requirements($phase) {
   +  // Test that path.temporary config is not set.
   +  if ($phase == 'runtime') {
   +    $filesystem_config = \Drupal::config('system.file');
   +    if ($temp_path = $filesystem_config->get('path.temporary')) {
   +      $requirements['temp_directory'] = [
   +        'title' => t('Temporary Directory'),
   +        'severity' => REQUIREMENT_WARNING,
   +        'value' => 'Deprecated configuration',
   +        'description' => [
   +          [
   +            '#markup' => t('You are using deprecated configuration for the temporary files path.'),
   +            '#suffix' => ' ',
   +          ],
   +        ],
   +      ];
   +      if ($temp_path === FileSystem::getOsTemporaryDirectory()) {
   +        $requirements['temp_directory']['description'][] = [
   +          '#markup' => t('Your temporary directory configuration matches the OS default and can be safely removed.'),
   +          '#suffix' => ' ',
   +        ];
   +      }
   +      else {
   +        $requirements['temp_directory']['description'][] = [
   +          '#markup' => t('Remove the configuration and add the following to <code>settings.php

   .

   $settings["file_temp_path"] = "%temp_path"', ['%temp_path' => $temp_path]),
   +          '#suffix' => ' ',
   +        ];
   +      }
   +    }
   +  }
   

   This logic needs testing.

3. +++ b/core/modules/system/system.install
   @@ -2312,3 +2342,16 @@ function system_update_8702() {
   +      ->save();
   

   ->save(TRUE);

We can disable schema checking for the tests affected tests.

In regards to the migration, do I understand that correctly that the temp directory there is only set to find files in the temporary folder and deal with that? In that case, we should keep that? (there doesn't seem to be any explicit test coverage for it, though?)

+++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
@@ -101,11 +101,6 @@ public function testFiles() {
-    Database::getConnection('default', 'migrate')
-      ->update('variable')
-      ->fields(['value' => serialize(file_directory_temp())])
-      ->condition('name', 'file_directory_temp')
-      ->execute();

Back in the day, we didn't migrate temporary files. in #2394157: Update the EntityFile destination to handle temporary files we started to. I haven't pieced together the entire history of it all, but it seems that the temporary directory in the d6 fixture is set to /files/temp in the variables table, but the temporary file in the fixture has a path of /tmp/xxx. This was all before the fixtures as we know them too.. From what I can gather, at that point, the mismatch would cause the system to decide the temporary file wasn't there (simulating it being deleted by the filesystem) and skip migrating it in the initial migration in the test. Back then the update line explicitly set the variable to '/tmp' to match the testfile, and the test tested that the temporary file was migrated in the second runthrough.

This lead to an issue when the migration was happening on the same server, and the source and destination temporary directories were the same, and the migration was trying to save the file over itself. We fixed this in #2405337: EntityFile destination doesn't handle temporary files when the source and destination are the same and tested it by setting the temporary directory of the test run to /tmp too. This caused failures, which were fixed in #2417549: Drupal\migrate_drupal\Tests\d6\MigrateFileTest fail in MigrateTestBase by adding a temporary file to the fixture on the fly that matched the temporary directory of the test run(usinging simpletests TestBase::getTempFilesDirectory(). This set us on the path of setting the d7 fixture temp directory variable to match the test run's temp directory on the fly in this test. In #2684141: Convert migrate kernel tests to KernelTestBaseTNG The test moved from simpletest to phpunit, and TestBase::getTempFilesDirectory became file_directory_temp().

All of which fun stuff leads up to #2729369: Remove support for migrating temporary files where we hard code ignoring files with a path that start with /tmp in the d6 file source, and no longer migrate temporary files at all. There used to be an assertion that the temporary file was migrated on the second run after setting the variable correctly, but that was removed, making the entire fixture update to change the source site's temporary directory completely useless (it should have been removed in that issue too). (as a side note, why 2729369 skips files whose path starts with /tmp rather than files whose status is 0, or at least whose file path starts with the source site's temp directory variable value, and why the file_url process plugin still processes temporary file uris is completely beyond me, and probably deserves a follow-up)

TLDR; This is fine. The update to the variable table is no longer necessary, (and was done incorrectly anyway)

I think we can't remove the schema until the key is fully removed, the update only conditionally removes it and I guess the update tests that didn't fail before also didn't actually unset it.

Although I'm wondering what the value is then, maybe it's empty, and we could check for empty too and remove that as well? Doesn't change that there are cases when it needs to remain. and we should only remove the schema when we remove the BC layer?

+++ b/core/modules/system/tests/src/Kernel/Migrate/d6/MigrateSystemConfigurationTest.php
@@ -47,11 +47,11 @@ class MigrateSystemConfigurationTest extends MigrateDrupal6TestBase {
       'default_scheme' => 'public',
+      // temporary_maximum_age is not handled by the migration.
+      'temporary_maximum_age' => 21600,
       'path' => [
         'temporary' => 'files/temp',
       ],
-      // temporary_maximum_age is not handled by the migration.
-      'temporary_maximum_age' => 21600,
     ],
     'system.image.gd' => [

So apparently here we still assert that the configuration setting is migrated?

Was looking for it in file.module before, but it's obviously in system.

As commented before, I believe migrating this doesn't make sense at all, files/temp obviously doesn't make much sense, it's definitely not going to be that on D8, seems like it would be much better to just use whatever D8 decided to use?

Signing off on the latest migration changes. I agree that there is no reason to migrate that variable.

1. +++ b/core/includes/file.inc
   @@ -1196,30 +1195,16 @@ function drupal_tempnam($directory, $prefix) {
   + *
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
   + *   \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
   + *
   ...
   +  @trigger_error('file_directory_temp() is deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Use \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead. See https://www.drupal.org/node/3039255', E_USER_DEPRECATED);
   

   inconsistent deprecation message: will be vs is.

2. +++ b/core/includes/install.core.inc
   @@ -1098,21 +1097,6 @@ function install_base_system(&$install_state) {
   -  // Prevent the installer from using the system temporary directory after the
   -  // system module has been installed.
   -  if (drupal_valid_test_ua()) {
   -    // While the temporary directory could be preset/enforced in settings.php
   -    // like the public files directory, some tests expect it to be configurable
   -    // in the UI. If declared in settings.php, they would no longer be
   -    // configurable. The temporary directory needs to match what is set in each
   

   for reviewers: This part is now instead handled in FunctionalTestSetupTrait, by writing the value to settings.php, like we do public/private, which is much cleaner.

3. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -625,4 +627,42 @@ public function createFilename($basename, $directory) {
   +        @trigger_error("The 'system.file' config 'path.temporary' is deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Set 'file_temp_path' in settings.php instead. See https://www.drupal.org/node/3039255", E_USER_DEPRECATED);
   

   another will be that should be is removed from.

4. +++ b/core/modules/system/system.install
   @@ -2312,3 +2342,16 @@ function system_update_8702() {
   +
   +/**
   + * Remove 'path.temporary' config if it matches OS default, or 'file_temp_path' setting is already set.
   + */
   +function system_update_8801() {
   +  // If settings is already being used, or the config is set to the OS default,
   

   I didn't see an update function that's over 80 characters, I assume the limit applies here too. I think it is OK to be less specific in the description, maybe say remove if redundant or something like that?

   Also, the change record should recommend to set the replacement (if customized) in settings.php before running updates, then it's already getting cleaned up :)

5. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemRequirementsTest.php
   @@ -0,0 +1,70 @@
   +    module_load_install('system');
   

   I thought this has to be deprecated too, but apparently not yet :) and doesn't matter if it will be as this is a legacy test anyway.

6. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemTempDirectoryTest.php
   @@ -0,0 +1,86 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $strictConfigSchema = FALSE;
   +
   

   not needed anymore.

7. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemTempDirectoryTest.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * Tests 'path.temporary' config deprecation.
   +   *
   +   * @covers ::getTempDirectory
   +   * @expectedDeprecation The 'system.file' config 'path.temporary' is deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Set 'file_temp_path' in settings.php instead. See https://www.drupal.org/node/3039255
   +   */
   +  public function testGetTempDirectoryDeprecation() {
   +    $tempDir = '/var/tmp/' . $this->randomMachineName();
   +    $this->config('system.file')
   +      ->set('path.temporary', $tempDir)
   +      ->save(TRUE);
   

   this overlaps a bit with the requirements test, which also asserts that message, not quite sure if we ned both but also doesn't hurt.

   The other two methods here aren't deprecated, so not sure if they are legacy or not. Maybe only this method should have @group legacy? That is supported.

8. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -1037,7 +1037,7 @@ public function __get($name) {
   diff --git a/core/tests/fixtures/config_install/multilingual.tar.gz b/core/tests/fixtures/config_install/multilingual.tar.gz
   
   diff --git a/core/tests/fixtures/config_install/multilingual.tar.gz b/core/tests/fixtures/config_install/multilingual.tar.gz
   index 36a08ae69a..e4961cc7de 100644
   
   index 36a08ae69a..e4961cc7de 100644
   --- a/core/tests/fixtures/config_install/multilingual.tar.gz
   
   --- a/core/tests/fixtures/config_install/multilingual.tar.gz
   +++ b/core/tests/fixtures/config_install/multilingual.tar.gz
   
   +++ b/core/tests/fixtures/config_install/multilingual.tar.gz
   +++ b/core/tests/fixtures/config_install/multilingual.tar.gz
   @@ -1,51 +1,47 @@
   

   note to reviewers: the config exports are updated to remove the deprecated key as that would otherwise result in deprecation warnings.

New setting could use @see to os default component
Why update hook used instead of post update one?

I think this is ready, I did several full reviews of this. I worked a bit on it as well and fixed some tests, but that was already several iterations ago. (interdiff there is large, but only because I had to re-create the config archives)

It still needs follow-up for #88

@andypost: Isn't that comment about the component method for which you already created an issue? #3063704: Deprecate \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() and replace with sys_get_temp_dir()

+++ b/core/modules/system/system.install
@@ -529,6 +529,36 @@ function system_requirements($phase) {
+      ];
+      if ($temp_path === FileSystem::getOsTemporaryDirectory()) {
+        $requirements['temp_directory']['description'][] = [
+          '#markup' => t('Your temporary directory configuration matches the OS default and can be safely removed.'),
+          '#suffix' => ' ',
+        ];
+      }
+      else {
+        $requirements['temp_directory']['description'][] = [
+          '#markup' => t('Remove the configuration and add the following to <code>settings.php

.

$settings["file_temp_path"] = "%temp_path"', ['%temp_path' => $temp_path]),
+          '#suffix' => ' ',
+        ];

How exactly do you remove the dead configuration apart from editing the file directly or using drush?

I was expecting the following logic:

1. If set in settings, do nothing.
2. If only set in config, but matching system default, do nothing - since no change when it's removed.
3. If only set in config, but not matching system default, then warn with instructions to add the same value to $settings.

Then Drupal 9 can have an update to remove the old config key, all we need prior to that is people to take the decision to rely on system default or put it in $settings.

We already have an update function that removes it if it either matches the system default or there is a settings, so I think 1 and 2 are already kind of covered? Not quite sure why we again check if it it matches the default.

OK I see the update now, that helps to reduce the number of people who'll see the message, but the logic in system_requirements() still feels off - I'd think we'd at least want to check if the $settings key is there before telling people to add it for example. If someone sees the message, adds the settings key, they'll get an identical message as it is at the moment.

This is good clean-up. Thanks @andypost for opening #3063704: Deprecate \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() and replace with sys_get_temp_dir() would also be great to remove it completely if we can!

Committed f4c9f6e and pushed to 8.8.x. Thanks!

Shouldn't there be a CR to show this change:
$config['system.file']['path']['temporary'] = 'foo/bar';
to
$settings['file_temp_path'] = 'foo/bar';

At least a more explicit CR than the current one.

I added the old value as well to the message, seems fairly clear to me otherwise.