Properly deprecate drupal_unlink()

+++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
@@ -11,6 +11,21 @@
  */
 class DirectoryTest extends FileTestBase {
 
+  /**
+   * A file system service.
+   *
+   * @var \Drupal\Core\File\FileSystemInterface
+   */
+  protected $fileSystem;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    $this->fileSystem = $this->container->get('file_system');
+  }
+

Tests don't need injection IMHO, I think they can just use \Drupal::service() directly, maybe with a local variable if there are repeated calls in the same method.

Looks streams are serialized somehow, attempt to fix it

That is in, but this needs a reroll now.

Reroll was trivial, just one conflict in file.inc for a change that is no longer necessary.

1. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -172,7 +172,7 @@ public function delete($name) {
        }
        $this->fileCache->delete($this->getFilePath($name));
   -    return drupal_unlink($this->getFilePath($name));
   +    return \Drupal::service('file_system')->unlink($this->getFilePath($name));
      }
   

   I would leave this snippet out of this issue and instead postpone on #2940135: Use file_system service to \Drupal\Core\Config\FileStorage, possibly only after it passes.

2. +++ b/core/lib/Drupal/Core/FileTransfer/Local.php
   @@ -7,6 +7,21 @@
   +   * {@inheritdoc}
   +   */
   +  public function __construct($jail) {
   +    parent::__construct($jail);
   +    $this->fileSystem = \Drupal::service('file_system');
   +  }
   +
      /**
   

   Looks like FileTransfer comes with a factory method that we can use to "inject", similar to controllers/forms. It doesn't have $container, but we can then still make $fileSystem a real injected dependency.

   Also, maybe add DependencySerializationTrait to this class, that might help with those weird test fails?

3. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -568,4 +568,14 @@ public function dir_closedir() {
   +   *
   +   * @return \Drupal\Core\File\FileSystemInterface
   +   *   The file system.
   

   Id' also add .. service here, it doesn't return the actual "file system". That only works with things like manager/handler or so in the class/service name.

4. +++ b/core/modules/config/src/Form/ConfigImportForm.php
   @@ -22,14 +23,24 @@ class ConfigImportForm extends FormBase {
   -  public function __construct(StorageInterface $config_storage) {
   +  public function __construct(StorageInterface $config_storage, FileSystemInterface $file_system) {
        $this->configStorage = $config_storage;
   +    $this->fileSystem = $file_system;
   

   lets do the NULL/@trigger_error() dance here.

5. +++ b/core/modules/file/tests/src/Kernel/ValidatorTest.php
   @@ -104,7 +104,7 @@ public function testFileValidateImageResolution() {
    
   -      drupal_unlink('temporary://druplicon.png');
   +      $this->container->get('file_system')->unlink('temporary://druplicon.png');
        }
   

   \Drupal::service(), same with some other tests.

6. +++ b/core/tests/Drupal/KernelTests/Core/File/LegacyFileSystemTest.php
   @@ -0,0 +1,29 @@
   +   *
   +   * @expectedDeprecation drupal_unlink() is deprecated. Use unlink() method of file_system service instead. See https://www.drupal.org/node/2418133
   +   */
   +  public function testUnlink() {
   +    vfsStream::setup('dir');
   +    vfsStream::create(['test.txt' => 'asdf']);
   

   we can put this in \Drupal\KernelTests\Core\File\FileSystemDeprecationTest now.

1. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -572,4 +572,14 @@ public function dir_closedir() {
   +   */
   +  protected function getFileSystem() {
   +    return \Drupal::service('file_system');
   +  }
   +
    }
   

   lets make this private, like in FileStorage.

2. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   @@ -139,4 +140,19 @@ public function testDeprecatedDrupalTempnam() {
   +  /**
   +   * Tests deprecation of the drupal_unlink() function.
   +   *
   +   * @expectedDeprecation drupal_unlink() is deprecated. Use unlink() method of file_system service instead. See https://www.drupal.org/node/2418133
   

   the deprecation message needs to be updated to say deprecated in 8.0.0 (I think?) and will be removed before 9.0.0

Set the visibility in LocalStream:: getFileSystem() to private.

Regarding the deprecation message. From what I see: there is already one. Could you please specify more detailed where it's missing?

The message needs to include the information that I mentioned. Here's a full example from format_date():

/** 
 *
 * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
 *   Use \Drupal::service('date.formatter')->format().
 *
 * @see \Drupal\Core\Datetime\DateFormatter::format()
 * @see https://www.drupal.org/node/1876852
 */
function format_date($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL) {
  @trigger_error("format_date() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::service('date.formatter')->format() instead. See https://www.drupal.org/node/1876852", E_USER_DEPRECATED);
  return \Drupal::service('date.formatter')->format($timestamp, $type, $format, $timezone, $langcode);
}

Also looks like we still have 2 failing tests.

The failing tests are weird. In Jenkings are several classes mentioned. Like FileTransferAuthorizeFormTest. However, all tests seem to be ok. How can I see in Jenkins, which test is actually failing?

FileTransferAuthorizeFormTest is failing on my machine. Other tests are running. I am using Chromedriver. Do I miss something in my phpunit.xml settings?

  <php>
    <!-- Set error reporting to E_ALL. -->
    <ini name="error_reporting" value="32767"/>
    <!-- Do not limit the amount of memory tests take to run. -->
    <ini name="memory_limit" value="-1"/>
    <!-- Example SIMPLETEST_BASE_URL value: http://localhost -->
    <env name="SIMPLETEST_BASE_URL" value="http://localhost:8887"/>
    <!-- Example SIMPLETEST_DB value: mysql://username:password@localhost/databasename#table_prefix -->
    <env name="SIMPLETEST_DB" value="sqlite://localhost//tmp/db-drupal-1.sqlite"/>
    <!-- Example BROWSERTEST_OUTPUT_DIRECTORY value: /path/to/webroot/sites/simpletest/browser_output -->
    <env name="BROWSERTEST_OUTPUT_DIRECTORY" value=""/>
    <env name="MINK_DRIVER_ARGS_WEBDRIVER" value='["chrome", null, "http://localhost:4444/wd/hub"]'/>
    <!-- To disable deprecation testing completely uncomment the next line. -->
    <!-- <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/> -->
    <!-- Example for changing the driver class for mink tests MINK_DRIVER_CLASS value: 'Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver' -->
    <!-- Example for changing the driver args to mink tests MINK_DRIVER_ARGS value: '["http://127.0.0.1:8510"]' -->
    <!-- Example for changing the driver args to phantomjs tests MINK_DRIVER_ARGS_PHANTOMJS value: '["http://127.0.0.1:8510"]' -->
    <!-- Example for changing the driver args to webdriver tests MINK_DRIVER_ARGS_WEBDRIVER value: '["firefox", null, "http://localhost:4444/wd/hub"]' -->
  </php>

• Merged in 8.8.x branch.
• Updated the PHPDoc comment.
• Removed the deprecations, which made the tests failing.

Updated PHPDoc block for @deprecated tag. Added interdiff file.

1. +++ b/core/includes/file.inc
   @@ -1081,6 +1081,7 @@ function drupal_chmod($uri, $mode = NULL) {
     */
    function drupal_unlink($uri, $context = NULL) {
   +  @trigger_error(__FUNCTION__ . '() is deprecated. Use unlink() method of file_system service instead. See https://www.drupal.org/node/2418133', E_USER_DEPRECATED);
      return \Drupal::service('file_system')->unlink($uri, $context);
    }
   

   This is the place where you need to update the deprecation message. You only touched the test that asserts that it is printed.

   Also the @deprecated above.

2. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   @@ -139,4 +140,24 @@ public function testDeprecatedDrupalTempnam() {
   +   *
   +   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   +   * @expectedDeprecation drupal_unlink() is deprecated.
   +   *  Use \Drupal::service('file_system')->unlink().
   +   *
   

   In the test then you have the identical string and only @expectedDeprecation.

Moved the deprecation message to drupal_unlink(). Left only @expectedDeprecation and the already existing comment in testUnlink().

+++ b/core/includes/file.inc
@@ -1075,12 +1075,14 @@ function drupal_chmod($uri, $mode = NULL) {
 function drupal_unlink($uri, $context = NULL) {
+  @trigger_error(__FUNCTION__ . '() is deprecated. Use unlink() method of file_system service instead. See https://www.drupal.org/node/2418133', E_USER_DEPRECATED);
   return \Drupal::service('file_system')->unlink($uri, $context);

The trigger error is not updated yet, both messages need to be basically identical.

@Berdir: I do not get you. Could you please express your remark more precisely? See details below.

1. +++ b/core/includes/file.inc
   @@ -1075,12 +1075,14 @@ function drupal_chmod($uri, $mode = NULL) {
   +  @trigger_error(__FUNCTION__ . '() is deprecated. Use unlink() method of file_system service instead. See https://www.drupal.org/node/2418133', E_USER_DEPRECATED);
   

   Identical with contents of @expectedDeprecation annotation in FileSystemDeprecationTest::testDeprecatedDrupalTempnam(). __FUNCTION__ will pass the function name.

2. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   @@ -139,4 +140,19 @@ public function testDeprecatedDrupalTempnam() {
   +   * @expectedDeprecation drupal_unlink() is deprecated. Use unlink() method of file_system service instead. See https://www.drupal.org/node/2418133
   

   @expectedDeprecation annotation text is Identical with @trigger_error() parameter in drupal_chmod().

Do you want me to change the @trigger_error() function parameter in the constructor of Drupal\Core\FileTransfer\Local?

  public function __construct($jail, FileSystemInterface $file_system = NULL) {
    parent::__construct($jail);
    if (!isset($file_system)) {
      @trigger_error('The $file_system parameter was added in Drupal 8.7.x and will be required in 9.0.0. See https://www.drupal.org/node/3021434', E_USER_DEPRECATED);
      $file_system = \Drupal::service('file_system');
    }
    $this->fileSystem = $file_system;
  }

I don't know how else to explain it, look at the date_format() example in #32 and updated examples in file.inc that have @trigger_error(). @deprecated and @trigger_error() and @expectedDeprecation *all* need to have the same structure that includes the version.

I have edited the patch. The assertion checks now the concatenated text from drupal_unlink() annotation tags.

Some last nitpicks, then I think we are done here.

If you're looking for other things to work on, I had a look at file.inc and it looks like #3021450: Add @trigger_error to deprecated functions in file.inc and replace their usages missed to add a @trigger_error() to drupal_realpath() and file_directory_os_temp(). Others are already covered in other issues like #3034072: Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager. there's also file_directory_temp(), not sure what do with that. We can't have FileSystem depend on configuration. This is the same problem as the one I wrote about in the stream wrapper manager issue above in regards to file_default_scheme().

1. +++ b/core/includes/file.inc
   @@ -1075,12 +1075,14 @@ function drupal_chmod($uri, $mode = NULL) {
     *
   - * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   - *   Use \Drupal\Core\File\FileSystem::unlink().
   + * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   + *  Use \Drupal::service('file_system')->unlink().
     *
   + * @see \Drupal\Core\File\FileSystem::unlink()
     * @see https://www.drupal.org/node/2418133
     */
    function drupal_unlink($uri, $context = NULL) {
   +  @trigger_error(__FUNCTION__ . '() is deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use \Drupal::service(\'file_system\')->unlink() instead. See https://www.drupal.org/node/2418133', E_USER_DEPRECATED);
      return \Drupal::service('file_system')->unlink($uri, $context);
   

   Hm, the change to \Drupal::service() instead of just the classname is different compared to all other functions that we've been deprecated. Lets undo that and keep it like it was before, with ..FileSystem::unlink() in all places.

2. +++ b/core/lib/Drupal/Core/FileTransfer/Local.php
   @@ -2,11 +2,35 @@
   +    parent::__construct($jail);
   +    if (!isset($file_system)) {
   +      @trigger_error('The $file_system parameter was added in Drupal 8.7.x and will be required in 9.0.0. See https://www.drupal.org/node/3021434', E_USER_DEPRECATED);
   

   This should be "8.7.0" instead of "8.7.x"

3. +++ b/core/modules/config/src/Form/ConfigImportForm.php
   @@ -22,14 +23,28 @@ class ConfigImportForm extends FormBase {
   +      @trigger_error('The $file_system parameter was added in Drupal 8.7.x and will be required in 9.0.0. See https://www.drupal.org/node/3021434', E_USER_DEPRECATED);
   

   same here.

Found #931794: 'file_temporary_path' variable and file_directory_temp() function should match in regards to the temp function.

Last patch solves the remaining.

Would you please review #3038513: Replace drupal_valid_test_ua() with static method for Drupal 9?

+++ b/core/lib/Drupal/Core/FileTransfer/Local.php
@@ -25,7 +25,7 @@
     parent::__construct($jail);
     if (!isset($file_system)) {
-      @trigger_error('The $file_system parameter was added in Drupal 8.7.x and will be required in 9.0.0. See https://www.drupal.org/node/3021434', E_USER_DEPRECATED);
+      @trigger_error('The $file_system parameter was added in Drupal 8.8.0 and will be required in 9.0.0. See https://www.drupal.org/node/3021434.', E_USER_DEPRECATED);
       $file_system = \Drupal::service('file_system');

beta is the deadline for deprecations, at least simple stuff like constructors: https://drupal.slack.com/archives/CDDD98AMN/p1552125380214700.

So we can keep 8.7.0 for now.

Thanks, looks good now.

1. +++ b/core/lib/Drupal/Core/FileTransfer/Local.php
   @@ -18,7 +42,7 @@ public function connect() {
      public static function factory($jail, $settings) {
   -    return new Local($jail);
   

   Did we give thought to changing the signature of the factory too, to include passing the file system? As I can see there is only one call in core for it - \Drupal\Core\FileTransfer\Form\FileTransferAuthorizeForm::getFiletransfer

   We could do the same thing here, make it optional, trigger_error etc?

   Then we could actually do DI in \Drupal\Core\FileTransfer\Form\FileTransferAuthorizeForm::getFiletransfer

2. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -374,7 +374,7 @@ public function stream_truncate($new_size) {
   -    return drupal_unlink($this->getLocalPath());
   +    return $this->getFileSystem()->unlink($this->getLocalPath());
      }
    
      /**
   @@ -572,4 +572,14 @@ public function dir_closedir() {
   
   @@ -572,4 +572,14 @@ public function dir_closedir() {
        return TRUE;
      }
    
   +  /**
   +   * Returns file system service.
   +   *
   +   * @return \Drupal\Core\File\FileSystemInterface
   +   *   The file system service.
   +   */
   +  private function getFileSystem() {
   +    return \Drupal::service('file_system');
   +  }
   

   The file system has a dependency on the stream wrapper manager, and this then makes the wrappers depend on it, which is a pseudo-circular dependency. How about a follow-up where we explore extending stream wrapper and stream wrapper manager interfaces to support add a setFileSystem method, which FileSystem could call in its constructor

1. I'd try to keep this minimal. There's an issue to use a more modern approach for discovery of those classes, I guess plugins, and then they would be able to use the standard plugin create method.

2. We can try to improve it in a follow-up, yeah. This dependency already exists, the only thing we do is remove one level of indirection. We did basically the same in #2940135: Use file_system service to \Drupal\Core\Config\FileStorage.

Still need to create the follow up issue?

I bet yes, cos injection of file system is pita every service

I bet yes, cos injection of file system is pita every service

If the file system is a real dependency, then I don't think injecting it is an issue. The real problem is the circular dependencies that have most likely alway been there, that are now being exposed by dependency injection.

We're trying to untangle the mess.

I created the following two issues, would be nice to get this in, I don't think it makes sense to improve the patch as it is now:

#3048126: Make it easier to inject the FileSystem service
#3048120: Improve dependency injection for FileTransfer classes

Re-roll

Committed 870b564 and pushed to 8.8.x. Thanks!