#443286: clean-up of files fail on Windows, leading to a lot of test failures. From: Damien Tournoud --- file.inc | 72 +++++++++++++++++++++++++++++++++++++++-- filetransfer/ftp.inc | 4 +- filetransfer/local.inc | 8 ++--- stream_wrappers.inc | 6 ++- color/color.module | 4 +- locale/locale.install | 2 + locale/locale.test | 4 +- simpletest/simpletest.install | 2 + simpletest/tests/file.test | 53 ++++++++++++++++++++++++------ system/system.install | 4 +- system/system.tar.inc | 12 +++---- 11 files changed, 135 insertions(+), 36 deletions(-) diff --git includes/file.inc includes/file.inc index cef2b15..3d83172 100644 --- includes/file.inc +++ includes/file.inc @@ -921,6 +921,10 @@ function file_create_filename($basename, $directory) { // Strip control characters (ASCII value < 32). Though these are allowed in // some filesystems, not many applications handle them well. $basename = preg_replace('/[\x00-\x1F]/u', '_', $basename); + if (substr(PHP_OS, 0, 3) == 'WIN') { + // These characters are not allowed in Windows filenames + $basename = str_replace(array(':', '*', '?', '"', '<', '>', '|'), '_', $basename); + } // A URI or path may already have a trailing slash or look like "public://". if (substr($directory, -1) == '/') { @@ -1018,7 +1022,7 @@ function file_unmanaged_delete($path) { return FALSE; } if (is_file($path)) { - return unlink($path); + return drupal_unlink($path); } // Return TRUE for non-existent file, but log that nothing was actually // deleted, as the current state is the intended result. @@ -1065,7 +1069,8 @@ function file_unmanaged_delete_recursive($path) { file_unmanaged_delete_recursive($entry_path); } $dir->close(); - return rmdir($path); + + return drupal_rmdir($path); } return file_unmanaged_delete($path); } @@ -1817,6 +1822,37 @@ function drupal_chmod($uri, $mode = NULL) { } /** + * Deletes a file. + * + * PHP's unlink() is broken on Windows, as it can fail to remove a file + * when it has a read-only flag set. + * + * @param $uri + * A URI or pathname. + * @param $context + * Refer to http://php.net/manual/en/ref.stream.php + * + * @return + * Boolean TRUE on success, or FALSE on failure. + * + * @see unlink() + * @ingroup php_wrappers + */ +function drupal_unlink($uri, $context = NULL) { + $scheme = file_uri_scheme($uri); + if ((!$scheme || !file_stream_wrapper_valid_scheme($scheme)) + && (substr(PHP_OS, 0, 3) == 'WIN')) { + chmod($uri, 0600); + } + if ($context) { + return unlink($uri, $context); + } + else { + return unlink($uri); + } +} + +/** * Returns the absolute path of a file or directory * * PHP's realpath() does not properly support streams, so this function @@ -1919,7 +1955,6 @@ function drupal_dirname($uri) { * @ingroup php_wrappers */ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) { - if (is_null($mode)) { $mode = variable_get('file_chmod_directory', 0775); } @@ -1933,6 +1968,37 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) { } /** + * Remove a directory. + * + * PHP's rmdir() is broken on Windows, as it can fail to remove a directory + * when it has a read-only flag set. + * + * @param $uri + * A URI or pathname. + * @param $context + * Refer to http://php.net/manual/en/ref.stream.php + * + * @return + * Boolean TRUE on success, or FALSE on failure. + * + * @see rmdir() + * @ingroup php_wrappers + */ +function drupal_rmdir($uri, $context = NULL) { + $scheme = file_uri_scheme($uri); + if ((!$scheme || !file_stream_wrapper_valid_scheme($scheme)) + && (substr(PHP_OS, 0, 3) == 'WIN')) { + chmod($uri, 0700); + } + if ($context) { + return rmdir($uri, $context); + } + else { + return rmdir($uri); + } +} + +/** * Creates a file with a unique filename in the specified directory. * * PHP's tempnam() does not return a URI like we want. This function diff --git includes/filetransfer/ftp.inc includes/filetransfer/ftp.inc index c3ee39b..ead835f 100644 --- includes/filetransfer/ftp.inc +++ includes/filetransfer/ftp.inc @@ -77,7 +77,7 @@ class FileTransferFTPWrapper extends FileTransferFTP { } } closedir($dh); - if (!rmdir($this->connection . $directory)) { + if (!drupal_rmdir($this->connection . $directory)) { $exception = new FileTransferException('Cannot remove @directory.', NULL, array('@directory' => $directory)); throw $exception; } @@ -91,7 +91,7 @@ class FileTransferFTPWrapper extends FileTransferFTP { } function removeFileJailed($destination) { - if (!@unlink($this->connection . '/' .$destination)) { + if (!@drupal_unlink($this->connection . '/' .$destination)) { throw new FileTransferException('Cannot remove @destination', NULL, array('@destination' => $destination)); } } diff --git includes/filetransfer/local.inc includes/filetransfer/local.inc index 18b8c24..bfcad3d 100644 --- includes/filetransfer/local.inc +++ includes/filetransfer/local.inc @@ -33,23 +33,23 @@ class FileTransferLocal extends FileTransfer implements FileTransferChmodInterfa } foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($directory), RecursiveIteratorIterator::CHILD_FIRST) as $filename => $file) { if ($file->isDir()) { - if (@!rmdir($filename)) { + if (@!drupal_rmdir($filename)) { throw new FileTransferException('Cannot remove directory %directory.', NULL, array('%directory' => $filename)); } } elseif ($file->isFile()) { - if (@!unlink($filename)) { + if (@!drupal_unlink($filename)) { throw new FileTransferException('Cannot remove file %file.', NULL, array('%file' => $filename)); } } } - if (@!rmdir($directory)) { + if (@!drupal_rmdir($directory)) { throw new FileTransferException('Cannot remove directory %directory.', NULL, array('%directory' => $directory)); } } protected function removeFileJailed($file) { - if (@!unlink($file)) { + if (@!drupal_unlink($file)) { throw new FileTransferException('Cannot remove file %file.', NULL, array('%file' => $file)); } } diff --git includes/stream_wrappers.inc includes/stream_wrappers.inc index 61784fe..30c24ce 100644 --- includes/stream_wrappers.inc +++ includes/stream_wrappers.inc @@ -473,7 +473,7 @@ abstract class DrupalLocalStreamWrapper implements DrupalStreamWrapperInterface */ public function unlink($uri) { $this->uri = $uri; - return unlink($this->getLocalPath()); + return drupal_unlink($this->getLocalPath()); } /** @@ -537,10 +537,10 @@ abstract class DrupalLocalStreamWrapper implements DrupalStreamWrapperInterface public function rmdir($uri, $options) { $this->uri = $uri; if ($options & STREAM_REPORT_ERRORS) { - return rmdir($this->getLocalPath()); + return drupal_rmdir($this->getLocalPath()); } else { - return @rmdir($this->getLocalPath()); + return @drupal_rmdir($this->getLocalPath()); } } diff --git modules/color/color.module modules/color/color.module index 0b5a889..5837576 100644 --- modules/color/color.module +++ modules/color/color.module @@ -312,10 +312,10 @@ function color_scheme_form_submit($form, &$form_state) { // Delete old files. foreach (variable_get('color_' . $theme . '_files', array()) as $file) { - @unlink($file); + @drupal_unlink($file); } if (isset($file) && $file = dirname($file)) { - @rmdir($file); + @drupal_rmdir($file); } // Don't render the default colorscheme, use the standard theme instead. diff --git modules/locale/locale.install modules/locale/locale.install index 73d3537..1ff8577 100644 --- modules/locale/locale.install +++ modules/locale/locale.install @@ -106,7 +106,7 @@ function locale_uninstall() { } // Delete the JavaScript translations directory if empty. if (!file_scan_directory($locale_js_directory, '/.*/')) { - rmdir($locale_js_directory); + drupal_rmdir($locale_js_directory); } // Clear variables. diff --git modules/locale/locale.test modules/locale/locale.test index b4833d1..2b8b186 100644 --- modules/locale/locale.test +++ modules/locale/locale.test @@ -762,7 +762,7 @@ class LocaleImportFunctionalTest extends DrupalWebTestCase { file_put_contents($name, $contents); $options['files[file]'] = $name; $this->drupalPost('admin/config/regional/translate/import', $options, t('Import')); - unlink($name); + drupal_unlink($name); } /** @@ -911,7 +911,7 @@ class LocaleExportFunctionalTest extends DrupalWebTestCase { 'langcode' => 'fr', 'files[file]' => $name, ), t('Import')); - unlink($name); + drupal_unlink($name); // Get the French translations. $this->drupalPost('admin/config/regional/translate/export', array( diff --git modules/simpletest/simpletest.install modules/simpletest/simpletest.install index 2805a9d..f194b37 100644 --- modules/simpletest/simpletest.install +++ modules/simpletest/simpletest.install @@ -25,7 +25,7 @@ function simpletest_uninstall() { foreach ($files as $file) { file_unmanaged_delete($file->uri); } - rmdir($path); + drupal_rmdir($path); } /** diff --git modules/simpletest/tests/file.test modules/simpletest/tests/file.test index dd4270b..3108500 100644 --- modules/simpletest/tests/file.test +++ modules/simpletest/tests/file.test @@ -108,7 +108,20 @@ class FileTestCase extends DrupalWebTestCase { clearstatcache(); // Mask out all but the last three octets. - $actual_mode = fileperms($filepath) & 511; + $actual_mode = fileperms($filepath) & 0777; + + // PHP on Windows has limited support for file permissions. Usually each of + // "user", "group" and "other" use one octal digit (3 bits) to represent the + // read/write/execute bits. On Windows, chmod() ignores the "group" and + // "other" bits, and fileperms() returns the "user" bits in all three + // positions. $expected_mode is updated to reflect this. + if (substr(PHP_OS, 0, 3) == 'WIN') { + // Reset the "group" and "other" bits. + $expected_mode = $expected_mode & 0700; + // Shift the "user" bits to the "group" and "other" positions also. + $expected_mode = $expected_mode | $expected_mode >> 3 | $expected_mode >> 6; + } + if (!isset($message)) { $message = t('Expected file permission to be %expected, actually were %actual.', array('%actual' => decoct($actual_mode), '%expected' => decoct($expected_mode))); } @@ -130,7 +143,20 @@ class FileTestCase extends DrupalWebTestCase { clearstatcache(); // Mask out all but the last three octets. - $actual_mode = fileperms($directory) & 511; + $actual_mode = fileperms($directory) & 0777; + + // PHP on Windows has limited support for file permissions. Usually each of + // "user", "group" and "other" use one octal digit (3 bits) to represent the + // read/write/execute bits. On Windows, chmod() ignores the "group" and + // "other" bits, and fileperms() returns the "user" bits in all three + // positions. $expected_mode is updated to reflect this. + if (substr(PHP_OS, 0, 3) == 'WIN') { + // Reset the "group" and "other" bits. + $expected_mode = $expected_mode & 0700; + // Shift the "user" bits to the "group" and "other" positions also. + $expected_mode = $expected_mode | $expected_mode >> 3 | $expected_mode >> 6; + } + if (!isset($message)) { $message = t('Expected directory permission to be %expected, actually were %actual.', array('%actual' => decoct($actual_mode), '%expected' => decoct($expected_mode))); } @@ -424,7 +450,7 @@ class FileValidatorTest extends DrupalWebTestCase { $this->assertTrue($info['width'] <= 10, t('Image scaled to correct width.'), 'File'); $this->assertTrue($info['height'] <= 5, t('Image scaled to correct height.'), 'File'); - unlink(drupal_realpath($temp_dir . '/druplicon.png')); + drupal_unlink(drupal_realpath($temp_dir . '/druplicon.png')); } else { // TODO: should check that the error is returned if no toolkit is available. @@ -716,19 +742,26 @@ class FileDirectoryTest extends FileTestCase { // Make sure directory actually exists. $this->assertTrue(is_dir($directory), t('Directory actually exists.'), 'File'); - // Make directory read only. - @chmod($directory, 0444); - $this->assertFalse(file_prepare_directory($directory, 0), t('Error reported for a non-writeable directory.'), 'File'); + 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 + // read-only flag, which doesn't prevent files to actually be written + // in the directory on any recent version of Windows. + + // Make directory read only. + @chmod($directory, 0444); + $this->assertFalse(file_prepare_directory($directory, 0), t('Error reported for a non-writeable directory.'), 'File'); - // Test directory permission modification. - $this->assertTrue(file_prepare_directory($directory, FILE_MODIFY_PERMISSIONS), t('No error reported when making directory writeable.'), 'File'); + // Test directory permission modification. + $this->assertTrue(file_prepare_directory($directory, FILE_MODIFY_PERMISSIONS), t('No error reported when making directory writeable.'), 'File'); + } - // Test directory permission modification actually set correct permissions. + // Test that the directory has the correct permissions. $this->assertDirectoryPermissions($directory, variable_get('file_chmod_directory', 0775)); // Remove .htaccess file to then test that it gets re-created. $directory = file_directory_path(); - @unlink($directory . '/.htaccess'); + @drupal_unlink($directory . '/.htaccess'); $this->assertFalse(is_file($directory . '/.htaccess'), t('Successfully removed the .htaccess file in the files directory.'), 'File'); file_ensure_htaccess(); $this->assertTrue(is_file($directory . '/.htaccess'), t('Successfully re-created the .htaccess file in the files directory.'), 'File'); diff --git modules/system/system.install modules/system/system.install index 923be4e..c57e7ba 100644 --- modules/system/system.install +++ modules/system/system.install @@ -2288,10 +2288,10 @@ function system_update_7046() { variable_set('theme_garland_settings', $settings); // Remove Garland's color files since they won't match Minnelli's. foreach (variable_get('color_garland_files', array()) as $file) { - @unlink($file); + @drupal_unlink($file); } if (isset($file) && $file = dirname($file)) { - @rmdir($file); + @drupal_rmdir($file); } variable_del('color_garland_palette'); variable_del('color_garland_stylesheets'); diff --git modules/system/system.tar.inc modules/system/system.tar.inc index dbf933c..18f9331 100644 --- modules/system/system.tar.inc +++ modules/system/system.tar.inc @@ -202,7 +202,7 @@ class Archive_Tar $this->_close(); // ----- Look for a local copy to delete if ($this->_temp_tarname != '') - @unlink($this->_temp_tarname); + @drupal_unlink($this->_temp_tarname); } // }}} @@ -766,7 +766,7 @@ class Archive_Tar // ----- Look if a local copy need to be erase // Note that it might be interesting to keep the url for a time : ToDo if ($this->_temp_tarname != '') { - @unlink($this->_temp_tarname); + @drupal_unlink($this->_temp_tarname); $this->_temp_tarname = ''; } @@ -782,11 +782,11 @@ class Archive_Tar // ----- Look for a local copy if ($this->_temp_tarname != '') { // ----- Remove the local copy but not the remote tarname - @unlink($this->_temp_tarname); + @drupal_unlink($this->_temp_tarname); $this->_temp_tarname = ''; } else { // ----- Remove the local tarname file - @unlink($this->_tarname); + @drupal_unlink($this->_tarname); } $this->_tarname = ''; @@ -1590,7 +1590,7 @@ class Archive_Tar } } elseif ($v_header['typeflag'] == "2") { if (@file_exists($v_header['filename'])) { - @unlink($v_header['filename']); + @drupal_unlink($v_header['filename']); } if (!@symlink($v_header['link'], $v_header['filename'])) { $this->_error('Unable to extract symbolic link {' @@ -1727,7 +1727,7 @@ class Archive_Tar @bzclose($v_temp_tar); } - if (!@unlink($this->_tarname.".tmp")) { + if (!@drupal_unlink($this->_tarname.".tmp")) { $this->_error('Error while deleting temporary file \'' .$this->_tarname.'.tmp\''); }