Refactor DrupalFiles to remove deprecated system_retrieve_file()

modules/common/common.services.yml

@@ -18,6 +18,8 @@ services:
     arguments:
       - '@file_system'
       - '@stream_wrapper_manager'
+      - '@http_client_factory'
+      - '@messenger'
 
   dkan.common.node_storage:
     class: Drupal\node\NodeStorage

modules/common/src/Util/DrupalFiles.php

@@ -3,8 +3,14 @@
 namespace Drupal\common\Util;
 
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
+use Drupal\Core\File\Exception\FileException;
+use Drupal\Core\File\Exception\InvalidStreamWrapperException;
 use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\Http\ClientFactory;
+use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\StreamWrapper\StreamWrapperManager;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Psr\Http\Client\ClientExceptionInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
@@ -18,6 +24,8 @@
  */
 class DrupalFiles implements ContainerInjectionInterface {
 
+  use StringTranslationTrait;
+
   /**
    * Drupal file system service.
    *
@@ -33,23 +41,44 @@ class DrupalFiles implements ContainerInjectionInterface {
   private $streamWrapperManager;
 
   /**
-   * Inherited.
+   * HTTP client factory service.
+   *
+   * @var \Drupal\Core\Http\ClientFactory
+   */
+  private ClientFactory $httpClientFactory;
+
+  /**
+   * Messenger service.
    *
-   * @inheritdoc
+   * @var \Drupal\Core\Messenger\MessengerInterface
+   */
+  private MessengerInterface $messenger;
+
+  /**
+   * {@inheritDoc}
    */
   public static function create(ContainerInterface $container) {
     return new static(
       $container->get('file_system'),
-      $container->get('stream_wrapper_manager')
+      $container->get('stream_wrapper_manager'),
+      $container->get('http_client_factory'),
+      $container->get('messenger')
     );
   }
 
   /**
    * Constructor.
    */
-  public function __construct(FileSystemInterface $filesystem, StreamWrapperManager $streamWrapperManager) {
+  public function __construct(
+    FileSystemInterface $filesystem,
+    StreamWrapperManager $streamWrapperManager,
+    ClientFactory $httpClientFactory,
+    MessengerInterface $messenger,
+  ) {
     $this->filesystem = $filesystem;
     $this->streamWrapperManager = $streamWrapperManager;
+    $this->httpClientFactory = $httpClientFactory;
+    $this->messenger = $messenger;
   }
 
   /**
@@ -64,6 +93,8 @@ public function getFilesystem(): FileSystemInterface {
 
   /**
    * Getter.
+   *
+   * @deprecated
    */
   public function getStreamWrapperManager(): StreamWrapperManager {
     return $this->streamWrapperManager;
@@ -98,7 +129,91 @@ public function retrieveFile($url, $destination) {
       return $this->fileCreateUrl("{$destination}/{$filename}");
     }
     // Handle http(s):// URIs.
-    return system_retrieve_file($url, $destination, FALSE, FileSystemInterface::EXISTS_REPLACE);
+    return $this->systemRetrieveFile($url, $destination);
+  }
+
+  /**
+   * Attempts to get a file using Guzzle HTTP client and to store it locally.
+   *
+   * @param string $url
+   *   The URL of the file to grab.
+   * @param string $destination
+   *   Stream wrapper URI specifying where the file should be placed. If a
+   *   directory path is provided, the file is saved into that directory under
+   *   its original name. If the path contains a filename as well, that one will
+   *   be used instead.
+   *   If this value is omitted, the site's default files scheme will be used,
+   *   usually "public://".
+   * @param bool $managed
+   *   If this is set to TRUE, the file API hooks will be invoked and the file
+   *   is registered in the database.
+   * @param int $replace
+   *   Replace behavior when the destination file already exists:
+   *   - FileSystemInterface::EXISTS_REPLACE: Replace the existing file.
+   *   - FileSystemInterface::EXISTS_RENAME: Append _{incrementing number} until
+   *   the filename is unique.
+   *   - FileSystemInterface::EXISTS_ERROR: Do nothing and return FALSE.
+   *
+   * @return mixed
+   *   One of these possibilities:
+   *   - If it succeeds and $managed is FALSE, the location where the file was
+   *   saved.
+   *   - If it succeeds and $managed is TRUE, a \Drupal\file\FileInterface
+   *   object which describes the file.
+   *   - If it fails, FALSE.
+   *
+   * @see \system_retrieve_file()
+   * @see https://www.drupal.org/node/3223362
+   */
+  protected function systemRetrieveFile($url, $destination = NULL, $managed = FALSE, $replace = FileSystemInterface::EXISTS_RENAME) {
+    $parsed_url = parse_url($url);
+    if (!isset($destination)) {
+      $path = $this->filesystem->basename($parsed_url['path']);
+      $path = \Drupal::config('system.file')->get('default_scheme') . '://' . $path;
+      $path = $this->streamWrapperManager->normalizeUri($path);
+    }
+    else {
+      if (is_dir($this->filesystem->realpath($destination))) {
+        // Prevent URIs with triple slashes when glueing parts together.
+        $path = str_replace('///', '//', "$destination/") . \Drupal::service('file_system')->basename($parsed_url['path']);
+      }
+      else {
+        $path = $destination;
+      }
+    }
+    try {
+      $data = (string) $this->httpClientFactory->fromOptions()
+        ->get($url)
+        ->getBody();
+      if ($managed) {
+        /** @var \Drupal\file\FileRepositoryInterface $file_repository */
+        $file_repository = \Drupal::service('file.repository');
+        $local = $file_repository->writeData($data, $path, $replace);
+      }
+      else {
+        $local = $this->filesystem->saveData($data, $path, $replace);
+      }
+    }
+    catch (ClientExceptionInterface $exception) {
+      $this->messenger->addError($this->t('Failed to fetch file due to error "%error"', [
+        '%error' => $exception->getMessage(),
+      ]));
+      return FALSE;
+    }
+    catch (FileException | InvalidStreamWrapperException $e) {
+      $this->messenger->addError($this->t('Failed to save file due to error "%error"', [
+        '%error' => $e->getMessage(),
+      ]));
+      return FALSE;
+    }
+    if (!$local) {
+      $this->messenger->addError($this->t('@remote could not be saved to @path.', [
+        '@remote' => $url,
+        '@path' => $path,
+      ]));
+    }
+
+    return $local;
   }
 
   /**
@@ -111,7 +226,7 @@ public function fileCreateUrl($uri) : string {
     if (substr_count($uri, 'http') > 0) {
       return $uri;
     }
-    elseif ($wrapper = $this->getStreamWrapperManager()->getViaUri($uri)) {
+    elseif ($wrapper = $this->streamWrapperManager->getViaUri($uri)) {
       return $wrapper->getExternalUrl();
     }
     throw new \Exception("No stream wrapper available for {$uri}");

modules/common/tests/src/Functional/Util/DrupalFilesTest.php

@@ -0,0 +1,78 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\common\Functional\Util;
+
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * This is a copy of \Drupal\Tests\system\Functional\System\RetrieveFileTest.
+ *
+ * @covers \Drupal\common\Util\DrupalFiles
+ * @coversDefaultClass \Drupal\common\Util\DrupalFiles
+ *
+ * @group dkan
+ * @group common
+ * @group functional
+ *
+ * @see \Drupal\Tests\system\Functional\System\RetrieveFileTest
+ */
+class DrupalFilesTest extends BrowserTestBase {
+
+  protected static $modules = [
+    'common',
+  ];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * @covers ::systemRetrieveFile
+   */
+  public function testFileRetrieving(): void {
+    /** @var \Drupal\common\Util\DrupalFiles $drupal_files */
+    $drupal_files = \Drupal::service('dkan.common.drupal_files');
+    $ref_system_retrieve_file = new \ReflectionMethod($drupal_files, 'systemRetrieveFile');
+    $ref_system_retrieve_file->setAccessible(TRUE);
+
+    // Test 404 handling by trying to fetch a randomly named file.
+    /** @var \Drupal\Core\File\FileSystemInterface $file_system */
+    $file_system = \Drupal::service('file_system');
+    $file_system->mkdir($source_dir = 'public://' . $this->randomMachineName());
+    // cSpell:disable-next-line
+    $filename = 'Файл для тестирования ' . $this->randomMachineName();
+    $url = \Drupal::service('file_url_generator')->generateAbsoluteString($source_dir . '/' . $filename);
+    $retrieved_file = $ref_system_retrieve_file->invokeArgs($drupal_files, [$url]);
+    $this->assertFalse($retrieved_file, 'Non-existent file not fetched.');
+
+    // Actually create that file, download it via HTTP and test the returned path.
+    file_put_contents($source_dir . '/' . $filename, 'testing');
+    $retrieved_file = $ref_system_retrieve_file->invokeArgs($drupal_files, [$url]);
+
+    // URLs could not contains characters outside the ASCII set so $filename
+    // has to be encoded.
+    $encoded_filename = rawurlencode($filename);
+
+    $this->assertEquals('public://' . $encoded_filename, $retrieved_file, 'Sane path for downloaded file returned (public:// scheme).');
+    $this->assertFileExists($retrieved_file);
+    $this->assertEquals(7, filesize($retrieved_file), 'File size of downloaded file is correct (public:// scheme).');
+    /** @var \Drupal\Core\File\FileSystemInterface $file_system */
+    $file_system = \Drupal::service('file_system');
+    $file_system->delete($retrieved_file);
+
+    // Test downloading file to a different location.
+    $file_system->mkdir($target_dir = 'temporary://' . $this->randomMachineName());
+    $retrieved_file = $ref_system_retrieve_file->invokeArgs($drupal_files, [$url, $target_dir]);
+    $this->assertEquals("{$target_dir}/{$encoded_filename}", $retrieved_file, 'Sane path for downloaded file returned (temporary:// scheme).');
+    $this->assertFileExists($retrieved_file);
+    $this->assertEquals(7, filesize($retrieved_file), 'File size of downloaded file is correct (temporary:// scheme).');
+    $file_system->delete($retrieved_file);
+
+    $file_system->deleteRecursive($source_dir);
+    $file_system->deleteRecursive($target_dir);
+  }
+
+}

modules/common/tests/src/Kernel/Util/DrupalFilesTest.php

@@ -0,0 +1,78 @@
+<?php
+
+namespace Drupal\Tests\common\Kernel\Util;
+
+use Drupal\common\Util\DrupalFiles;
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * @covers \Drupal\common\Util\DrupalFiles
+ * @coversDefaultClass \Drupal\common\Util\DrupalFiles
+ *
+ * @group dkan
+ * @group common
+ * @group kernel
+ */
+class DrupalFilesTest extends KernelTestBase {
+
+  protected static $modules = [
+    'common',
+  ];
+
+  public function provideExceptions() {
+    return [
+      ['Only file:// and http(s) urls are supported', 'badscheme://', 'any_destination'],
+      ["Only moving files to Drupal's public directory (public://) is supported", 'file://', 'badscheme://'],
+    ];
+  }
+
+  /**
+   * @covers ::retrieveFile
+   *
+   * @dataProvider provideExceptions
+   */
+  public function testExceptions($exception_message, $url, $destination) {
+    /** @var \Drupal\common\Util\DrupalFiles $drupal_files */
+    $drupal_files = $this->container->get('dkan.common.drupal_files');
+    $this->expectException(\Exception::class);
+    $this->expectExceptionMessage($exception_message);
+    $drupal_files->retrieveFile($url, $destination);
+  }
+
+  public function provideRetrieve() {
+    return [
+      ['http://'],
+      ['https://'],
+    ];
+  }
+
+  /**
+   * @covers ::retrieveFile
+   *
+   * @dataProvider provideRetrieve
+   */
+  public function testHttpSource($url) {
+    // We're checking the internal logic of retrieveFile(), to make sure it
+    // calls systemRetrieveFile() given the inputs, and not testing whether the
+    // file is successfully retrieved.
+    // Mock a DrupalFiles object so that we can mock systemRetrieveFile().
+    $drupal_files = $this->getMockBuilder(DrupalFiles::class)
+      ->setConstructorArgs([
+        $this->container->get('file_system'),
+        $this->container->get('stream_wrapper_manager'),
+        $this->container->get('http_client_factory'),
+        $this->container->get('messenger'),
+      ])
+      ->onlyMethods(['systemRetrieveFile'])
+      ->getMock();
+    $drupal_files->expects($this->once())
+      ->method('systemRetrieveFile')
+      ->willReturn('/your/fake/path');
+
+    $this->assertEquals(
+      '/your/fake/path',
+      $drupal_files->retrieveFile($url, 'public://')
+    );
+  }
+
+}

modules/common/tests/src/Unit/Util/DrupalFilesTest.php

@@ -4,6 +4,8 @@
 
 use Drupal\common\Util\DrupalFiles;
 use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\Http\ClientFactory;
+use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\StreamWrapper\StreamWrapperInterface;
 use Drupal\Core\StreamWrapper\StreamWrapperManager;
 use MockChain\Chain;
@@ -12,7 +14,9 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
- *
+ * @group dkan
+ * @group common
+ * @group unit
  */
 class DrupalFilesTest extends TestCase {
 
@@ -34,6 +38,8 @@ private function getContainer(): ContainerInterface {
     $options = (new Options())
       ->add('file_system', FileSystemInterface::class)
       ->add('stream_wrapper_manager', StreamWrapperManager::class)
+      ->add('http_client_factory', ClientFactory::class)
+      ->add('messenger', MessengerInterface::class)
       ->index(0);
 
     return (new Chain($this))

modules/harvest/tests/src/Kernel/Transform/ResourceImporterTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Drupal\Tests\harvest\Kernel\Transform;
 
 use Drupal\common\Util\DrupalFiles;
@@ -85,6 +87,8 @@ public function testSaveFile() {
       ->setConstructorArgs([
         $this->container->get('file_system'),
         $this->container->get('stream_wrapper_manager'),
+        $this->container->get('http_client_factory'),
+        $this->container->get('messenger'),
       ])
       ->onlyMethods(['retrieveFile'])
       ->getMock();