From 84fcf292a530ed7e584e68697d22476b59e4c327 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Mon, 26 Feb 2024 10:38:47 +0000
Subject: [PATCH] Issue #2838474 by kim.pepper, rpayanm, alexpott, longwave,
Spokje, andypost, 20th, mfb: Remove dependency of "file_system" service on
"logger"
---
core/core.services.yml | 2 +-
core/lib/Drupal/Core/File/FileSystem.php | 72 ++++---------------
.../Drupal/Core/File/FileSystemInterface.php | 3 +-
.../Core/File/FileSystemTempDirectoryTest.php | 3 +-
.../Drupal/Tests/Core/File/FileSystemTest.php | 5 +-
5 files changed, 19 insertions(+), 66 deletions(-)
diff --git a/core/core.services.yml b/core/core.services.yml
index 2eaf74d271f7..20c03e16815f 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -455,7 +455,7 @@ services:
Drupal\Component\Datetime\TimeInterface: '@datetime.time'
file_system:
class: Drupal\Core\File\FileSystem
- arguments: ['@stream_wrapper_manager', '@settings', '@logger.channel.file']
+ arguments: ['@stream_wrapper_manager', '@settings']
Drupal\Core\File\FileSystemInterface: '@file_system'
file_url_generator:
class: Drupal\Core\File\FileUrlGenerator
diff --git a/core/lib/Drupal/Core/File/FileSystem.php b/core/lib/Drupal/Core/File/FileSystem.php
index f88ba9d8f54c..d58cfee04df5 100644
--- a/core/lib/Drupal/Core/File/FileSystem.php
+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -4,6 +4,7 @@
use Drupal\Component\FileSystem\FileSystem as FileSystemComponent;
use Drupal\Component\Utility\Unicode;
+use Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait;
use Drupal\Core\File\Exception\DirectoryNotReadyException;
use Drupal\Core\File\Exception\FileException;
use Drupal\Core\File\Exception\FileExistsException;
@@ -15,13 +16,21 @@
use Drupal\Core\StreamWrapper\PublicStream;
use Drupal\Core\StreamWrapper\StreamWrapperManager;
use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface;
-use Psr\Log\LoggerInterface;
/**
* Provides helpers to operate on files and stream wrappers.
*/
class FileSystem implements FileSystemInterface {
+ use DeprecatedServicePropertyTrait;
+
+ /**
+ * {@inheritdoc}
+ */
+ protected array $deprecatedProperties = [
+ 'logger' => 'logger.channel.file',
+ ];
+
/**
* Default mode for new directories. See self::chmod().
*/
@@ -39,13 +48,6 @@ class FileSystem implements FileSystemInterface {
*/
protected $settings;
- /**
- * The file logger channel.
- *
- * @var \Psr\Log\LoggerInterface
- */
- protected $logger;
-
/**
* The stream wrapper manager.
*
@@ -60,13 +62,10 @@ class FileSystem implements FileSystemInterface {
* The stream wrapper manager.
* @param \Drupal\Core\Site\Settings $settings
* The site settings.
- * @param \Psr\Log\LoggerInterface $logger
- * The file logger channel.
*/
- public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger) {
+ public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings) {
$this->streamWrapperManager = $stream_wrapper_manager;
$this->settings = $settings;
- $this->logger = $logger;
}
/**
@@ -102,12 +101,7 @@ public function chmod($uri, $mode = NULL) {
}
}
- if (@chmod($uri, $mode)) {
- return TRUE;
- }
-
- $this->logger->error('The file permissions could not be set on %uri.', ['%uri' => $uri]);
- return FALSE;
+ return @chmod($uri, $mode);
}
/**
@@ -302,10 +296,6 @@ public function copy($source, $destination, $replace = self::EXISTS_RENAME) {
$real_source = $this->realpath($source) ?: $source;
$real_destination = $this->realpath($destination) ?: $destination;
if ($real_source === FALSE || $real_destination === FALSE || !@copy($real_source, $real_destination)) {
- $this->logger->error("The specified file '%source' could not be copied to '%destination'.", [
- '%source' => $source,
- '%destination' => $destination,
- ]);
throw new FileWriteException("The specified file '$source' could not be copied to '$destination'.");
}
}
@@ -322,27 +312,23 @@ public function copy($source, $destination, $replace = self::EXISTS_RENAME) {
public function delete($path) {
if (is_file($path)) {
if (!$this->unlink($path)) {
- $this->logger->error("Failed to unlink file '%path'.", ['%path' => $path]);
throw new FileException("Failed to unlink file '$path'.");
}
return TRUE;
}
if (is_dir($path)) {
- $this->logger->error("Cannot delete '%path' because it is a directory. Use deleteRecursive() instead.", ['%path' => $path]);
throw new NotRegularFileException("Cannot delete '$path' because it is a directory. Use deleteRecursive() instead.");
}
- // Return TRUE for non-existent file, but log that nothing was actually
- // deleted, as the current state is the intended result.
+ // Return TRUE for non-existent file as the current state is the intended
+ // result.
if (!file_exists($path)) {
- $this->logger->notice('The file %path was not deleted because it does not exist.', ['%path' => $path]);
return TRUE;
}
// We cannot handle anything other than files and directories.
// Throw an exception for everything else (sockets, symbolic links, etc).
- $this->logger->error("The file '%path' is not of a recognized type so it was not deleted.", ['%path' => $path]);
throw new NotRegularFileException("The file '$path' is not of a recognized type so it was not deleted.");
}
@@ -398,17 +384,9 @@ public function move($source, $destination, $replace = self::EXISTS_RENAME) {
// been implemented. It's not necessary to use FileSystem::unlink() as the
// Windows issue has already been resolved above.
if (!@copy($real_source, $real_destination)) {
- $this->logger->error("The specified file '%source' could not be moved to '%destination'.", [
- '%source' => $source,
- '%destination' => $destination,
- ]);
throw new FileWriteException("The specified file '$source' could not be moved to '$destination'.");
}
if (!@unlink($real_source)) {
- $this->logger->error("The source file '%source' could not be unlinked after copying to '%destination'.", [
- '%source' => $source,
- '%destination' => $destination,
- ]);
throw new FileException("The source file '$source' could not be unlinked after copying to '$destination'.");
}
}
@@ -449,16 +427,9 @@ protected function prepareDestination($source, &$destination, $replace) {
if (!file_exists($source)) {
if (($realpath = $this->realpath($original_source)) !== FALSE) {
- $this->logger->error("File '%original_source' ('%realpath') could not be copied because it does not exist.", [
- '%original_source' => $original_source,
- '%realpath' => $realpath,
- ]);
throw new FileNotExistsException("File '$original_source' ('$realpath') could not be copied because it does not exist.");
}
else {
- $this->logger->error("File '%original_source' could not be copied because it does not exist.", [
- '%original_source' => $original_source,
- ]);
throw new FileNotExistsException("File '$original_source' could not be copied because it does not exist.");
}
}
@@ -472,10 +443,6 @@ protected function prepareDestination($source, &$destination, $replace) {
// Perhaps $destination is a dir/file?
$dirname = $this->dirname($destination);
if (!$this->prepareDirectory($dirname)) {
- $this->logger->error("The specified file '%original_source' could not be copied because the destination directory '%destination_directory' is not properly configured. This may be caused by a problem with file or directory permissions.", [
- '%original_source' => $original_source,
- '%destination_directory' => $dirname,
- ]);
throw new DirectoryNotReadyException("The specified file '$original_source' could not be copied because the destination directory '$dirname' is not properly configured. This may be caused by a problem with file or directory permissions.");
}
}
@@ -483,10 +450,6 @@ protected function prepareDestination($source, &$destination, $replace) {
// Determine whether we can perform this operation based on overwrite rules.
$destination = $this->getDestinationFilename($destination, $replace);
if ($destination === FALSE) {
- $this->logger->error("File '%original_source' could not be copied because a file by that name already exists in the destination directory ('%destination').", [
- '%original_source' => $original_source,
- '%destination' => $destination,
- ]);
throw new FileExistsException("File '$original_source' could not be copied because a file by that name already exists in the destination directory ('$destination').");
}
@@ -494,9 +457,6 @@ protected function prepareDestination($source, &$destination, $replace) {
$real_source = $this->realpath($source);
$real_destination = $this->realpath($destination);
if ($source == $destination || ($real_source !== FALSE) && ($real_source == $real_destination)) {
- $this->logger->error("File '%source' could not be copied because it would overwrite itself.", [
- '%source' => $source,
- ]);
throw new FileException("File '$source' could not be copied because it would overwrite itself.");
}
}
@@ -508,7 +468,6 @@ public function saveData($data, $destination, $replace = self::EXISTS_RENAME) {
// Write the data to a temporary file.
$temp_name = $this->tempnam('temporary://', 'file');
if (file_put_contents($temp_name, $data) === FALSE) {
- $this->logger->error("Temporary file '%temp_name' could not be created.", ['%temp_name' => $temp_name]);
throw new FileWriteException("Temporary file '$temp_name' could not be created.");
}
@@ -737,9 +696,6 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0)
}
closedir($handle);
}
- else {
- $this->logger->error('@dir can not be opened', ['@dir' => $dir]);
- }
// Give priority to files in this folder by merging them after
// any subdirectory files.
diff --git a/core/lib/Drupal/Core/File/FileSystemInterface.php b/core/lib/Drupal/Core/File/FileSystemInterface.php
index 19ab1c9f7787..7089ef0df8bd 100644
--- a/core/lib/Drupal/Core/File/FileSystemInterface.php
+++ b/core/lib/Drupal/Core/File/FileSystemInterface.php
@@ -90,7 +90,8 @@ public function moveUploadedFile($filename, $uri);
* more information.
*
* @return bool
- * TRUE for success, FALSE in the event of an error.
+ * TRUE for success, FALSE in the event of an error. Note, it is the
+ * caller's to log an error if necessary.
*
* @ingroup php_wrappers
*/
diff --git a/core/tests/Drupal/KernelTests/Core/File/FileSystemTempDirectoryTest.php b/core/tests/Drupal/KernelTests/Core/File/FileSystemTempDirectoryTest.php
index bfa8503b229b..19b22a41fe25 100644
--- a/core/tests/Drupal/KernelTests/Core/File/FileSystemTempDirectoryTest.php
+++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemTempDirectoryTest.php
@@ -34,9 +34,8 @@ class FileSystemTempDirectoryTest extends KernelTestBase {
protected function setUp(): void {
parent::setUp();
$stream_wrapper_manager = $this->container->get('stream_wrapper_manager');
- $logger = $this->container->get('logger.channel.file');
$settings = $this->container->get('settings');
- $this->fileSystem = new FileSystem($stream_wrapper_manager, $settings, $logger);
+ $this->fileSystem = new FileSystem($stream_wrapper_manager, $settings);
}
/**
diff --git a/core/tests/Drupal/Tests/Core/File/FileSystemTest.php b/core/tests/Drupal/Tests/Core/File/FileSystemTest.php
index 4794b5fe44b7..12b049d9c703 100644
--- a/core/tests/Drupal/Tests/Core/File/FileSystemTest.php
+++ b/core/tests/Drupal/Tests/Core/File/FileSystemTest.php
@@ -45,8 +45,7 @@ protected function setUp(): void {
$settings = new Settings([]);
$this->streamWrapperManager = $this->createMock(StreamWrapperManagerInterface::class);
- $this->logger = $this->createMock('Psr\Log\LoggerInterface');
- $this->fileSystem = new FileSystem($this->streamWrapperManager, $settings, $this->logger);
+ $this->fileSystem = new FileSystem($this->streamWrapperManager, $settings);
}
/**
@@ -82,8 +81,6 @@ public function testChmodDir() {
*/
public function testChmodUnsuccessful() {
vfsStream::setup('dir');
- $this->logger->expects($this->once())
- ->method('error');
$this->assertFalse($this->fileSystem->chmod('vfs://dir/test.txt'));
}
--
GitLab