From 870b5646b2ec191aa674cddddf8eb5d47d33f0a8 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Fri, 26 Apr 2019 12:44:21 +1000 Subject: [PATCH] Issue #3021434 by voleger, Peter Majmesku, andypost, Berdir, kim.pepper: Properly deprecate drupal_unlink() --- core/includes/file.inc | 4 ++- core/includes/install.core.inc | 3 +- core/lib/Drupal/Core/File/FileSystem.php | 2 +- core/lib/Drupal/Core/FileTransfer/Local.php | 30 +++++++++++++++++-- .../Drupal/Core/StreamWrapper/LocalStream.php | 12 +++++++- core/modules/color/color.module | 3 +- .../config/src/Form/ConfigImportForm.php | 22 ++++++++++++-- .../file/tests/src/Kernel/ValidatorTest.php | 2 +- .../tests/src/Functional/LocaleExportTest.php | 9 +++--- .../Functional/LocaleImportFunctionalTest.php | 5 ++-- .../src/Functional/LocalePluralFormatTest.php | 5 ++-- .../src/Functional/Theme/TwigTransTest.php | 5 ++-- .../update/src/Form/UpdateManagerInstall.php | 2 +- core/modules/update/src/Form/UpdateReady.php | 2 +- .../src/TestFileTransferWithSettingsForm.php | 2 +- core/modules/update/update.manager.inc | 3 +- .../KernelTests/Core/File/DirectoryTest.php | 4 +-- .../Core/File/FileSystemDeprecationTest.php | 16 ++++++++++ .../Core/File/ReadOnlyStreamWrapperTest.php | 3 +- 19 files changed, 105 insertions(+), 29 deletions(-) diff --git a/core/includes/file.inc b/core/includes/file.inc index 178d7f61f6e..9493b9dcded 100644 --- a/core/includes/file.inc +++ b/core/includes/file.inc @@ -1091,12 +1091,14 @@ function drupal_chmod($uri, $mode = NULL) { /** * Deletes a file. * - * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0. + * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. * Use \Drupal\Core\File\FileSystem::unlink(). * + * @see \Drupal\Core\File\FileSystem::unlink() * @see https://www.drupal.org/node/2418133 */ function drupal_unlink($uri, $context = NULL) { + @trigger_error('drupal_unlink() is deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemInterface::unlink(). See https://www.drupal.org/node/2418133.', E_USER_DEPRECATED); return \Drupal::service('file_system')->unlink($uri, $context); } diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc index 03230a0519c..70aea0d9dfb 100644 --- a/core/includes/install.core.inc +++ b/core/includes/install.core.inc @@ -2085,6 +2085,7 @@ function install_check_requirements($install_state) { 'description_default' => t('The default settings file does not exist.'), 'title' => t('Settings file'), ]; + $file_system = \Drupal::service('file_system'); foreach ($default_files as $default_file_info) { $readable = FALSE; @@ -2146,7 +2147,7 @@ function install_check_requirements($install_state) { // file we just created and force the administrator to log on to the // server and create it manually. else { - $deleted = @drupal_unlink($file); + $deleted = @$file_system->unlink($file); // We expect deleting the file to be successful (since we just // created it ourselves above), but if it fails somehow, we set a // variable so we can display a one-time error message to the diff --git a/core/lib/Drupal/Core/File/FileSystem.php b/core/lib/Drupal/Core/File/FileSystem.php index ebb113c3804..b974592f576 100644 --- a/core/lib/Drupal/Core/File/FileSystem.php +++ b/core/lib/Drupal/Core/File/FileSystem.php @@ -411,7 +411,7 @@ class FileSystem implements FileSystemInterface { if (!@rename($real_source, $real_destination)) { // Fall back to slow copy and unlink procedure. This is necessary for // renames across schemes that are not local, or where rename() has not - // been implemented. It's not necessary to use drupal_unlink() as the + // 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'.", [ diff --git a/core/lib/Drupal/Core/FileTransfer/Local.php b/core/lib/Drupal/Core/FileTransfer/Local.php index 82ee421ba82..0d10d5a2088 100644 --- a/core/lib/Drupal/Core/FileTransfer/Local.php +++ b/core/lib/Drupal/Core/FileTransfer/Local.php @@ -2,11 +2,35 @@ namespace Drupal\Core\FileTransfer; +use Drupal\Core\DependencyInjection\DependencySerializationTrait; +use Drupal\Core\File\FileSystemInterface; + /** * Defines the local connection class for copying files as the httpd user. */ class Local extends FileTransfer implements ChmodInterface { + use DependencySerializationTrait; + + /** + * The file system service. + * + * @var \Drupal\Core\File\FileSystemInterface + */ + protected $fileSystem; + + /** + * {@inheritdoc} + */ + 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.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'); + } + $this->fileSystem = $file_system; + } + /** * {@inheritdoc} */ @@ -18,7 +42,7 @@ class Local extends FileTransfer implements ChmodInterface { * {@inheritdoc} */ public static function factory($jail, $settings) { - return new Local($jail); + return new Local($jail, \Drupal::service('file_system')); } /** @@ -56,7 +80,7 @@ class Local extends FileTransfer implements ChmodInterface { } } elseif ($file->isFile()) { - if (@!drupal_unlink($filename)) { + if (@!$this->fileSystem->unlink($filename)) { throw new FileTransferException('Cannot remove file %file.', NULL, ['%file' => $filename]); } } @@ -70,7 +94,7 @@ class Local extends FileTransfer implements ChmodInterface { * {@inheritdoc} */ protected function removeFileJailed($file) { - if (@!drupal_unlink($file)) { + if (@!$this->fileSystem->unlink($file)) { throw new FileTransferException('Cannot remove file %file.', NULL, ['%file' => $file]); } } diff --git a/core/lib/Drupal/Core/StreamWrapper/LocalStream.php b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php index b139b7825ea..67ab36a56a3 100644 --- a/core/lib/Drupal/Core/StreamWrapper/LocalStream.php +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php @@ -374,7 +374,7 @@ abstract class LocalStream implements StreamWrapperInterface { */ public function unlink($uri) { $this->uri = $uri; - return drupal_unlink($this->getLocalPath()); + return $this->getFileSystem()->unlink($this->getLocalPath()); } /** @@ -572,4 +572,14 @@ abstract class LocalStream implements StreamWrapperInterface { return TRUE; } + /** + * Returns file system service. + * + * @return \Drupal\Core\File\FileSystemInterface + * The file system service. + */ + private function getFileSystem() { + return \Drupal::service('file_system'); + } + } diff --git a/core/modules/color/color.module b/core/modules/color/color.module index 92c6110b08e..5f1d9375e6f 100644 --- a/core/modules/color/color.module +++ b/core/modules/color/color.module @@ -424,11 +424,12 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) { } } + $file_system = \Drupal::service('file_system'); // Delete old files. $files = $config->get('files'); if (isset($files)) { foreach ($files as $file) { - @drupal_unlink($file); + @$file_system->unlink($file); } } if (isset($file) && $file = dirname($file)) { diff --git a/core/modules/config/src/Form/ConfigImportForm.php b/core/modules/config/src/Form/ConfigImportForm.php index 28852f24eaa..4911d98c689 100644 --- a/core/modules/config/src/Form/ConfigImportForm.php +++ b/core/modules/config/src/Form/ConfigImportForm.php @@ -4,6 +4,7 @@ namespace Drupal\config\Form; use Drupal\Core\Archiver\ArchiveTar; use Drupal\Core\Config\StorageInterface; +use Drupal\Core\File\FileSystemInterface; use Drupal\Core\Form\FormBase; use Drupal\Core\Form\FormStateInterface; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -22,14 +23,28 @@ class ConfigImportForm extends FormBase { */ protected $configStorage; + /** + * The file system service. + * + * @var \Drupal\Core\File\FileSystemInterface + */ + protected $fileSystem; + /** * Constructs a new ConfigImportForm. * * @param \Drupal\Core\Config\StorageInterface $config_storage * The configuration storage. + * @param \Drupal\Core\File\FileSystemInterface $file_system + * The file system service. */ - public function __construct(StorageInterface $config_storage) { + public function __construct(StorageInterface $config_storage, FileSystemInterface $file_system = NULL) { $this->configStorage = $config_storage; + if (!isset($file_system)) { + @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'); + } + $this->fileSystem = $file_system; } /** @@ -37,7 +52,8 @@ class ConfigImportForm extends FormBase { */ public static function create(ContainerInterface $container) { return new static( - $container->get('config.storage.sync') + $container->get('config.storage.sync'), + $container->get('file_system') ); } @@ -106,7 +122,7 @@ class ConfigImportForm extends FormBase { catch (\Exception $e) { $this->messenger()->addError($this->t('Could not extract the contents of the tar file. The error message is @message', ['@message' => $e->getMessage()])); } - drupal_unlink($path); + $this->fileSystem->unlink($path); } } diff --git a/core/modules/file/tests/src/Kernel/ValidatorTest.php b/core/modules/file/tests/src/Kernel/ValidatorTest.php index 9f6dde314bd..e8f2c3709fe 100644 --- a/core/modules/file/tests/src/Kernel/ValidatorTest.php +++ b/core/modules/file/tests/src/Kernel/ValidatorTest.php @@ -106,7 +106,7 @@ class ValidatorTest extends FileManagedUnitTestBase { $errors = file_validate_image_resolution($this->image, '-10x-5'); $this->assertEqual(count($errors), 1, 'An error reported for an oversized image that can not be scaled down.', 'File'); - drupal_unlink('temporary://druplicon.png'); + \Drupal::service('file_system')->unlink('temporary://druplicon.png'); } else { // TODO: should check that the error is returned if no toolkit is available. diff --git a/core/modules/locale/tests/src/Functional/LocaleExportTest.php b/core/modules/locale/tests/src/Functional/LocaleExportTest.php index 3d0d1f416e1..313c8e17324 100644 --- a/core/modules/locale/tests/src/Functional/LocaleExportTest.php +++ b/core/modules/locale/tests/src/Functional/LocaleExportTest.php @@ -41,15 +41,16 @@ class LocaleExportTest extends BrowserTestBase { * Test exportation of translations. */ public function testExportTranslation() { + $file_system = \Drupal::service('file_system'); // First import some known translations. // This will also automatically add the 'fr' language. - $name = \Drupal::service('file_system')->tempnam('temporary://', "po_") . '.po'; + $name = $file_system->tempnam('temporary://', "po_") . '.po'; file_put_contents($name, $this->getPoFile()); $this->drupalPostForm('admin/config/regional/translate/import', [ 'langcode' => 'fr', 'files[file]' => $name, ], t('Import')); - drupal_unlink($name); + $file_system->unlink($name); // Get the French translations. $this->drupalPostForm('admin/config/regional/translate/export', [ @@ -62,14 +63,14 @@ class LocaleExportTest extends BrowserTestBase { $this->assertRaw('msgstr "lundi"', 'French translations present in exported file.'); // Import some more French translations which will be marked as customized. - $name = \Drupal::service('file_system')->tempnam('temporary://', "po2_") . '.po'; + $name = $file_system->tempnam('temporary://', "po2_") . '.po'; file_put_contents($name, $this->getCustomPoFile()); $this->drupalPostForm('admin/config/regional/translate/import', [ 'langcode' => 'fr', 'files[file]' => $name, 'customized' => 1, ], t('Import')); - drupal_unlink($name); + $file_system->unlink($name); // Create string without translation in the locales_source table. $this->container diff --git a/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php index b8701b6c349..ab0935f253f 100644 --- a/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php @@ -374,11 +374,12 @@ class LocaleImportFunctionalTest extends BrowserTestBase { * (optional) Additional options to pass to the translation import form. */ public function importPoFile($contents, array $options = []) { - $name = \Drupal::service('file_system')->tempnam('temporary://', "po_") . '.po'; + $file_system = \Drupal::service('file_system'); + $name = $file_system->tempnam('temporary://', "po_") . '.po'; file_put_contents($name, $contents); $options['files[file]'] = $name; $this->drupalPostForm('admin/config/regional/translate/import', $options, t('Import')); - drupal_unlink($name); + $file_system->unlink($name); } /** diff --git a/core/modules/locale/tests/src/Functional/LocalePluralFormatTest.php b/core/modules/locale/tests/src/Functional/LocalePluralFormatTest.php index 6af46e5327a..edd2344bb09 100644 --- a/core/modules/locale/tests/src/Functional/LocalePluralFormatTest.php +++ b/core/modules/locale/tests/src/Functional/LocalePluralFormatTest.php @@ -352,11 +352,12 @@ class LocalePluralFormatTest extends BrowserTestBase { * Additional options to pass to the translation import form. */ public function importPoFile($contents, array $options = []) { - $name = \Drupal::service('file_system')->tempnam('temporary://', "po_") . '.po'; + $file_system = \Drupal::service('file_system'); + $name = $file_system->tempnam('temporary://', "po_") . '.po'; file_put_contents($name, $contents); $options['files[file]'] = $name; $this->drupalPostForm('admin/config/regional/translate/import', $options, t('Import')); - drupal_unlink($name); + $file_system->unlink($name); } /** diff --git a/core/modules/system/tests/src/Functional/Theme/TwigTransTest.php b/core/modules/system/tests/src/Functional/Theme/TwigTransTest.php index aeafe7aeb11..0bd3919dd1a 100644 --- a/core/modules/system/tests/src/Functional/Theme/TwigTransTest.php +++ b/core/modules/system/tests/src/Functional/Theme/TwigTransTest.php @@ -192,6 +192,7 @@ class TwigTransTest extends BrowserTestBase { * Helper function: install languages. */ protected function installLanguages() { + $file_system = \Drupal::service('file_system'); foreach ($this->languages as $langcode => $name) { // Generate custom .po contents for the language. $contents = $this->poFileContents($langcode); @@ -209,7 +210,7 @@ class TwigTransTest extends BrowserTestBase { $this->assertRaw('"edit-languages-' . $langcode . '-weight"', 'Language code found.'); // Import the custom .po contents for the language. - $filename = \Drupal::service('file_system')->tempnam('temporary://', "po_") . '.po'; + $filename = $file_system->tempnam('temporary://', "po_") . '.po'; file_put_contents($filename, $contents); $options = [ 'files[file]' => $filename, @@ -217,7 +218,7 @@ class TwigTransTest extends BrowserTestBase { 'customized' => TRUE, ]; $this->drupalPostForm('admin/config/regional/translate/import', $options, t('Import')); - drupal_unlink($filename); + $file_system->unlink($filename); } } $this->container->get('language_manager')->reset(); diff --git a/core/modules/update/src/Form/UpdateManagerInstall.php b/core/modules/update/src/Form/UpdateManagerInstall.php index c3fed384827..9ff5690633c 100644 --- a/core/modules/update/src/Form/UpdateManagerInstall.php +++ b/core/modules/update/src/Form/UpdateManagerInstall.php @@ -234,7 +234,7 @@ class UpdateManagerInstall extends FormBase { // update_authorize_run_install() directly. if (fileowner($project_real_location) == fileowner($this->sitePath) && !$test_authorize) { $this->moduleHandler->loadInclude('update', 'inc', 'update.authorize'); - $filetransfer = new Local($this->root); + $filetransfer = new Local($this->root, \Drupal::service('file_system')); $response = call_user_func_array('update_authorize_run_install', array_merge([$filetransfer], $arguments)); if ($response instanceof Response) { $form_state->setResponse($response); diff --git a/core/modules/update/src/Form/UpdateReady.php b/core/modules/update/src/Form/UpdateReady.php index 15a9ddd3f52..83739146d28 100644 --- a/core/modules/update/src/Form/UpdateReady.php +++ b/core/modules/update/src/Form/UpdateReady.php @@ -153,7 +153,7 @@ class UpdateReady extends FormBase { // and invoke update_authorize_run_update() directly. if (fileowner($project_real_location) == fileowner($this->sitePath)) { $this->moduleHandler->loadInclude('update', 'inc', 'update.authorize'); - $filetransfer = new Local($this->root); + $filetransfer = new Local($this->root, \Drupal::service('file_system')); $response = update_authorize_run_update($filetransfer, $updates); if ($response instanceof Response) { $form_state->setResponse($response); diff --git a/core/modules/update/tests/modules/update_test/src/TestFileTransferWithSettingsForm.php b/core/modules/update/tests/modules/update_test/src/TestFileTransferWithSettingsForm.php index 4afcade707e..5b0d6c303f4 100644 --- a/core/modules/update/tests/modules/update_test/src/TestFileTransferWithSettingsForm.php +++ b/core/modules/update/tests/modules/update_test/src/TestFileTransferWithSettingsForm.php @@ -22,7 +22,7 @@ class TestFileTransferWithSettingsForm extends Local { * A new Drupal\update_test\TestFileTransferWithSettingsForm object. */ public static function factory($jail, $settings) { - return new static($jail); + return new static($jail, \Drupal::service('file_system')); } /** diff --git a/core/modules/update/update.manager.inc b/core/modules/update/update.manager.inc index cbe1c5f9128..40a65563edf 100644 --- a/core/modules/update/update.manager.inc +++ b/core/modules/update/update.manager.inc @@ -309,6 +309,7 @@ function update_manager_batch_project_get($project, $url, &$context) { * @see install_check_requirements() */ function update_manager_local_transfers_allowed() { + $file_system = \Drupal::service('file_system'); // Compare the owner of a webserver-created temporary file to the owner of // the configuration directory to determine if local transfers will be // allowed. @@ -318,7 +319,7 @@ function update_manager_local_transfers_allowed() { // Clean up. If this fails, we can ignore it (since this is just a temporary // file anyway). - @drupal_unlink($temporary_file); + @$file_system->unlink($temporary_file); return $local_transfers_allowed; } diff --git a/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php index ca2d873cd87..c62a8191a48 100644 --- a/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php @@ -72,7 +72,7 @@ class DirectoryTest extends FileTestBase { // Make sure directory actually exists. $this->assertTrue(is_dir($directory), 'Directory actually exists.', 'File'); - + $file_system = \Drupal::service('file_system'); if (substr(PHP_OS, 0, 3) != 'WIN') { // PHP on Windows doesn't support any kind of useful read-only mode for // directories. When executing a chmod() on a directory, PHP only sets the @@ -92,7 +92,7 @@ class DirectoryTest extends FileTestBase { $this->assertDirectoryPermissions($directory, 0777, 'file_chmod_directory setting is respected.'); // Remove .htaccess file to then test that it gets re-created. - @drupal_unlink(file_default_scheme() . '://.htaccess'); + @$file_system->unlink(file_default_scheme() . '://.htaccess'); $this->assertFalse(is_file(file_default_scheme() . '://.htaccess'), 'Successfully removed the .htaccess file in the files directory.', 'File'); file_ensure_htaccess(); $this->assertTrue(is_file(file_default_scheme() . '://.htaccess'), 'Successfully re-created the .htaccess file in the files directory.', 'File'); diff --git a/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php index bc18bd3f7ce..3b148cc10dc 100644 --- a/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php @@ -3,6 +3,7 @@ namespace Drupal\KernelTests\Core\File; use Drupal\KernelTests\KernelTestBase; +use org\bovigo\vfs\vfsStream; /** * Tests deprecations in file.inc. @@ -146,4 +147,19 @@ class FileSystemDeprecationTest extends KernelTestBase { $this->assertNotNull(drupal_tempnam('temporary://', 'file')); } + /** + * Tests deprecation of the drupal_unlink() function. + * + * @expectedDeprecation drupal_unlink() is deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemInterface::unlink(). See https://www.drupal.org/node/2418133. + */ + public function testUnlink() { + vfsStream::setup('dir'); + vfsStream::create(['test.txt' => 'asdf']); + $uri = 'vfs://dir/test.txt'; + + $this->assertFileExists($uri); + drupal_unlink($uri); + $this->assertFileNotExists($uri); + } + } diff --git a/core/tests/Drupal/KernelTests/Core/File/ReadOnlyStreamWrapperTest.php b/core/tests/Drupal/KernelTests/Core/File/ReadOnlyStreamWrapperTest.php index 8dd9de8965c..d313cd6a4f5 100644 --- a/core/tests/Drupal/KernelTests/Core/File/ReadOnlyStreamWrapperTest.php +++ b/core/tests/Drupal/KernelTests/Core/File/ReadOnlyStreamWrapperTest.php @@ -46,6 +46,7 @@ class ReadOnlyStreamWrapperTest extends FileTestBase { $uri = $this->scheme . '://' . $filename; \Drupal::service('stream_wrapper_manager')->getViaScheme($this->scheme); + $file_system = \Drupal::service('file_system'); // Attempt to open a file in read/write mode $handle = @fopen($uri, 'r+'); $this->assertFalse($handle, 'Unable to open a file for reading and writing with the read-only stream wrapper.'); @@ -79,7 +80,7 @@ class ReadOnlyStreamWrapperTest extends FileTestBase { // Test the rename() function $this->assertFalse(@rename($uri, $this->scheme . '://newname.txt'), 'Unable to rename files using the read-only stream wrapper.'); // Test the unlink() function - $this->assertTrue(@drupal_unlink($uri), 'Able to unlink file using read-only stream wrapper.'); + $this->assertTrue(@$file_system->unlink($uri), 'Able to unlink file using read-only stream wrapper.'); $this->assertTrue(file_exists($filepath), 'Unlink File was not actually deleted.'); // Test the mkdir() function by attempting to create a directory.