From cb9df6f04399ac2efd143be539658c4264e449e3 Mon Sep 17 00:00:00 2001 From: drugan Date: Wed, 22 Feb 2017 14:40:48 +0200 Subject: [PATCH] Issue #2745123 by drugan, mondrake, Mile23, benjifisher, penyaskito, hgoto, othermachines, Jaypan: Simpletest module crashes on cleanup, uninstall --- core/modules/simpletest/simpletest.module | 69 ++++++++++++++++++++++----- core/modules/simpletest/src/TestBase.php | 19 +------- core/scripts/run-tests.sh | 8 ++-- core/tests/Drupal/Tests/BrowserTestBase.php | 22 +-------- 4 files changed, 65 insertions(+), 53 deletions(-) diff --git a/core/modules/simpletest/simpletest.module b/core/modules/simpletest/simpletest.module index c9eed3b..70f75d6 100644 --- a/core/modules/simpletest/simpletest.module +++ b/core/modules/simpletest/simpletest.module @@ -684,26 +684,73 @@ function simpletest_clean_database() { /** * Finds all leftover temporary directories and removes them. + * + * @param string $directory + * (optional) The relative path to directory of a particular test site. Must + * to be of the following pattern: sites/simpletest/12345678. If ommited, all + * the test sites' directories found in sites/simpletest will be removed. + * + * @return bool + * TRUE if directory is removed, FALSE otherwise. Additionally watchdog + * messages will be emitted if any path attempted to remove fails. + * + * @see file_unmanaged_delete() + * @see http://php.net/manual/en/function.unlink.php */ -function simpletest_clean_temporary_directories() { +function simpletest_clean_temporary_directories($directory = NULL) { + $directories = []; $count = 0; - if (is_dir(DRUPAL_ROOT . '/sites/simpletest')) { - $files = scandir(DRUPAL_ROOT . '/sites/simpletest'); - foreach ($files as $file) { + $result = FALSE; + $simpletest_root = \Drupal::root() . '/sites/simpletest/'; + + if (is_dir($simpletest_root)) { + // If the $directory is not valid string or NULL then get the type of it for + // debugging purposes in the watchdog error message below. + $path = is_string($directory) && !empty($directory) ? $directory : gettype($directory); + // Do not recognize any, except expected directory pattern as wrong + // directory being passed accidentally may cause catastrophic consequences. + preg_match('/^(sites\/simpletest\/)(.*)/', $path, $matches); + + if (!empty($matches[2]) && is_dir($simpletest_root . $matches[2])) { + $directories[] = $matches[2]; + } + elseif ($directory === NULL) { + $directories = scandir($simpletest_root); + } + else { + $logger = \Drupal::logger('Testing'); + $logger->error('The %path is not eligible for removing in %func().', ['%path' => $path, '%func' => __FUNCTION__]); + } + + foreach ($directories as $file) { if ($file[0] != '.') { - $path = DRUPAL_ROOT . '/sites/simpletest/' . $file; - file_unmanaged_delete_recursive($path, array('Drupal\simpletest\TestBase', 'filePreDeleteCallback')); + $path = $simpletest_root . $file; + // When the webserver runs with the same system user as the test + // runner, we can make read-only files writable again. If not, chmod + // will fail while the file deletion still works if file permissions + // have been configured correctly. Thus, we ignore any chmod errors. + $result = file_unmanaged_delete_recursive($path, function ($any_path) { + // @see http://php.net/manual/en/function.chmod.php + @chmod($any_path, 0700); + }); $count++; } } } - if ($count > 0) { - drupal_set_message(\Drupal::translation()->formatPlural($count, 'Removed 1 temporary directory.', 'Removed @count temporary directories.')); - } - else { - drupal_set_message(t('No temporary directories to remove.')); + // To prevent some tests failure do not display any messages if this function + // was called from within a test instead of using "Clean environment" button. + // @see https://www.drupal.org/node/2745123#comment-11837053 + if ($directory === NULL) { + if ($count > 0) { + drupal_set_message(\Drupal::translation()->formatPlural($count, 'Removed 1 temporary directory.', 'Removed @count temporary directories.')); + } + else { + drupal_set_message(t('No temporary directories to remove.')); + } } + + return $result; } /** diff --git a/core/modules/simpletest/src/TestBase.php b/core/modules/simpletest/src/TestBase.php index 14d7909..b6e2c3e 100644 --- a/core/modules/simpletest/src/TestBase.php +++ b/core/modules/simpletest/src/TestBase.php @@ -1231,8 +1231,8 @@ private function restoreEnvironment() { $this->container = $this->originalContainer; \Drupal::setContainer($this->originalContainer); - // Delete test site directory. - file_unmanaged_delete_recursive($this->siteDirectory, array($this, 'filePreDeleteCallback')); + // Delete the current test site directory. + simpletest_clean_temporary_directories($this->siteDirectory); // Restore original database connection. Database::removeConnection('default'); @@ -1393,21 +1393,6 @@ public static function generatePermutations($parameters) { } /** - * Ensures test files are deletable within file_unmanaged_delete_recursive(). - * - * Some tests chmod generated files to be read only. During - * TestBase::restoreEnvironment() and other cleanup operations, these files - * need to get deleted too. - */ - public static function filePreDeleteCallback($path) { - // When the webserver runs with the same system user as the test runner, we - // can make read-only files writable again. If not, chmod will fail while - // the file deletion still works if file permissions have been configured - // correctly. Thus, we ignore any problems while running chmod. - @chmod($path, 0700); - } - - /** * Configuration accessor for tests. Returns non-overridden configuration. * * @param $name diff --git a/core/scripts/run-tests.sh b/core/scripts/run-tests.sh index 9e832a5..b1ae138 100755 --- a/core/scripts/run-tests.sh +++ b/core/scripts/run-tests.sh @@ -911,11 +911,9 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) { $messages[] = $errors; } } - // Delete the test site directory. - // simpletest_clean_temporary_directories() cannot be used here, since it - // would also delete file directories of other tests that are potentially - // running concurrently. - file_unmanaged_delete_recursive($test_directory, array('Drupal\simpletest\TestBase', 'filePreDeleteCallback')); + // Delete the current test site directory leaving alone other tests that + // are potentially running concurrently. + simpletest_clean_temporary_directories($test_db->getTestSitePath()); $messages[] = "- Removed test site directory."; } diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php index d4968f4..554d8f1 100644 --- a/core/tests/Drupal/Tests/BrowserTestBase.php +++ b/core/tests/Drupal/Tests/BrowserTestBase.php @@ -457,24 +457,6 @@ protected function setUp() { } /** - * Ensures test files are deletable within file_unmanaged_delete_recursive(). - * - * Some tests chmod generated files to be read only. During - * BrowserTestBase::cleanupEnvironment() and other cleanup operations, - * these files need to get deleted too. - * - * @param string $path - * The file path. - */ - public static function filePreDeleteCallback($path) { - // When the webserver runs with the same system user as phpunit, we can - // make read-only files writable again. If not, chmod will fail while the - // file deletion still works if file permissions have been configured - // correctly. Thus, we ignore any problems while running chmod. - @chmod($path, 0700); - } - - /** * Clean up the Simpletest environment. */ protected function cleanupEnvironment() { @@ -492,8 +474,8 @@ protected function cleanupEnvironment() { } } - // Delete test site directory. - file_unmanaged_delete_recursive($this->siteDirectory, array($this, 'filePreDeleteCallback')); + // Delete the current test site directory. + simpletest_clean_temporary_directories($this->siteDirectory); } /** -- 1.7.9.5