Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve file system error feedback #16479

Merged
merged 2 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions core/lexicon/en/file.inc.php
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
<?php

/**
* File English lexicon topic
*
* @language en
* @package modx
* @subpackage lexicon
*/

$_lang['directory'] = 'Directory';
$_lang['file_create'] = 'Create File';
$_lang['file_download'] = 'Download File';
Expand All @@ -14,17 +16,22 @@
$_lang['file_edit'] = 'Edit File';
$_lang['file_open'] = 'Open File Url';
$_lang['file_err_ae'] = 'File %s already exists';
$_lang['file_err_create'] = 'An unknown error occurred while trying to create the file.';
$_lang['file_err_create_general_exception'] = 'An unknown error occurred while trying to create the file. Please check the MODX and/or server error logs for more information.';
$_lang['file_err_create_write_exception'] = 'The file could not be created. Please verify you have write permissions for its target directory and try again.';
$_lang['file_err_ext_not_allowed'] = 'File extension `[[+ext]]` is not permitted.';
$_lang['file_err_filter'] = 'No files match the specified filter.';
$_lang['file_err_invalid'] = 'The file is not a regular file and cannot be deleted.';
$_lang['file_err_move_general_exception'] = 'An unknown error occurred while trying to move the file. Please check the MODX and/or server error logs for more information.';
$_lang['file_err_move_write_exception'] = 'The file could not be moved. Please verify you have write permissions for both the file and its target directory and try again.';
$_lang['file_err_nf'] = 'File does not exist!';
$_lang['file_err_ns'] = 'Please specify a valid file.';
$_lang['file_err_open'] = 'Cannot open file: ';
$_lang['file_err_rename'] = 'MODX failed to rename the file. Please make sure your permissions are set correctly.';
$_lang['file_err_remove'] = 'MODX failed to delete the file. Please make sure your permissions are set correctly.';
$_lang['file_err_too_large'] = 'Uploaded file is too large at [[+size]] bytes. Please ensure your files are less than [[+allowed]] bytes.';
$_lang['file_err_unzip'] = 'Unzip Failed!';
$_lang['file_err_update_general_exception'] = 'An unknown system error occurred while trying to update this file. Please check the MODX and/or server error logs for more information.';
$_lang['file_err_update_write_exception'] = 'The file could not be updated. Please verify you have write permissions for it and try again.';
$_lang['file_err_upload'] = 'An error occurred while trying to upload the files.';
$_lang['file_extensions'] = 'File Extensions';
$_lang['file_folder_path'] = 'Path';
Expand All @@ -40,9 +47,12 @@
$_lang['file_folder_err_ae'] = 'A directory already exists with that name in that location.';
$_lang['file_folder_err_create'] = 'An unknown error occurred while trying to create the directory.';
$_lang['file_folder_err_invalid'] = 'The specified directory is not a directory.';
$_lang['file_folder_err_move_general_exception'] = 'An unknown error occurred while trying to move the directory. Please check the MODX and/or server error logs for more information.';
$_lang['file_folder_err_move_write_exception'] = 'The directory could not be moved. Please verify you have write permissions for both this directory and its target directory and try again.';
$_lang['file_folder_err_ns'] = 'Please specify a valid directory.';
$_lang['file_folder_err_ns_name'] = 'Please specify a valid name for the directory.';
$_lang['file_folder_err_rename'] = 'An unknown error occurred while trying to rename the directory.';
$_lang['file_folder_err_rename_general_exception'] = 'An unknown error occurred while trying to rename the directory. Please check the MODX and/or server error logs for more information.';
$_lang['file_folder_err_rename_write_exception'] = 'The directory could not be renamed. Please verify you have write permissions for it and try again.';
$_lang['file_folder_err_rename_protected'] = 'Renaming the protected system directory is not permitted.';
$_lang['file_folder_err_remove'] = 'An error occurred while trying to delete the directory.';
$_lang['file_folder_err_remove_protected'] = 'Deleting the protected system directory is not permitted.';
Expand Down Expand Up @@ -115,3 +125,7 @@
$_lang['upload.clear_list.notpermitted'] = 'Delete not permitted only';
$_lang['upload.msg.title.error'] = 'Error';
$_lang['upload.upload.success'] = 'Upload successful';

/** Deprecated keys */
$_lang['file_err_create'] = $_lang['file_err_create_general_exception'];
$_lang['file_folder_err_rename'] = $_lang['file_folder_err_rename_general_exception'];
93 changes: 57 additions & 36 deletions core/src/Revolution/Sources/modMediaSource.php
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

namespace MODX\Revolution\Sources;

use Exception;
Expand Down Expand Up @@ -361,7 +362,6 @@
$directories = $dirNames = $files = $fileNames = [];

if (!empty($path)) {

// Ensure the provided path can be read.
try {
$mimeType = $this->filesystem->mimeType($path);
Expand All @@ -375,16 +375,15 @@
$this->addError('path', $this->xpdo->lexicon('file_folder_err_invalid'));
return [];
}

}

try {
$re = '#^(.*?/|)(' . implode('|', array_map('preg_quote', $skipFiles)) . ')/?$#';
$contents = $this->filesystem->listContents($path)
->filter(function(StorageAttributes $attributes) use ($re) {
->filter(function (StorageAttributes $attributes) use ($re) {
return !preg_match($re, $attributes->path());
})
->filter(function(StorageAttributes $attributes) use ($properties) {
->filter(function (StorageAttributes $attributes) use ($properties) {
if ($attributes->isDir()) {
return $this->hasPermission('directory_list');
} elseif ($attributes->isFile()) {
Expand Down Expand Up @@ -420,9 +419,7 @@
$directories[$file_name]['menu'] = [
'items' => $this->getListDirContextMenu(),
];

}
elseif ($object instanceof FileAttributes) {
} elseif ($object instanceof FileAttributes) {
// @TODO review/refactor extension and mime_type would be better for filesystems that
// may not always have an extension on it. For example would be S3 and you have an HTML file
// but the name is just myPage - $this->filesystem->getMimetype($object['path']);
Expand All @@ -449,9 +446,7 @@
foreach ($files as $file) {
$ls[] = $file;
}

return $ls;

} catch (FilesystemException $e) {
$this->addError('path', $e->getMessage());
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
Expand Down Expand Up @@ -486,14 +481,12 @@
$files = $fileNames = [];

if (!empty($path) && $path != DIRECTORY_SEPARATOR) {

try {
$mimeType = $this->filesystem->mimeType($path);
} catch (FilesystemException | UnableToRetrieveMetadata $e) {
$this->addError('path', $e->getMessage());
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
return [];

}

// Ensure this is a directory.
Expand All @@ -511,7 +504,11 @@
return [];
}
foreach ($contents as $object) {
if (in_array($object['path'], $skipFiles) || in_array(trim($object['path'], DIRECTORY_SEPARATOR), $skipFiles) || (in_array($fullPath . $object['path'], $skipFiles))) {
if (
in_array($object['path'], $skipFiles) ||
in_array(trim($object['path'], DIRECTORY_SEPARATOR), $skipFiles) ||
in_array($fullPath . $object['path'], $skipFiles)

Check warning on line 510 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L508-L510

Added lines #L508 - L510 were not covered by tests
) {
continue;
}
if ($object instanceof DirectoryAttributes && !$this->hasPermission('directory_list')) {
Expand Down Expand Up @@ -726,7 +723,11 @@
try {
$this->filesystem->write($path, $content, $config);
} catch (FilesystemException | UnableToWriteFile $e) {
$this->addError('name', $this->xpdo->lexicon('file_err_create'));
$messageKey = $e instanceof UnableToWriteFile
? 'file_err_create_write_exception'
: 'file_err_create_general_exception'
;
$this->addError('name', $this->xpdo->lexicon($messageKey));

Check warning on line 730 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L726-L730

Added lines #L726 - L730 were not covered by tests
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
return false;
}
Expand Down Expand Up @@ -812,7 +813,13 @@
try {
$this->filesystem->move($path, $newPath);
} catch (FilesystemException | UnableToMoveFile $e) {
$this->addError('from', $this->xpdo->lexicon('file_err_rename'));
// $this->addError('from', $this->xpdo->lexicon('file_err_rename'));
$prefix = $mimeType === 'directory' ? 'file_folder_' : 'file_' ;
$messageKey = $e instanceof UnableToMoveFile
? $prefix . 'err_move_write_exception'
: $prefix . 'err_move_write_general'
;
$this->addError('name', $this->xpdo->lexicon($messageKey));

Check warning on line 822 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L817-L822

Added lines #L817 - L822 were not covered by tests
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
return false;
}
Expand Down Expand Up @@ -960,7 +967,11 @@
try {
$this->filesystem->move($oldPath, $newPath);
} catch (FilesystemException | UnableToMoveFile $e) {
$this->addError('name', $this->xpdo->lexicon('file_folder_err_rename'));
$messageKey = $e instanceof UnableToMoveFile
? 'file_folder_err_rename_write_exception'
: 'file_folder_err_rename_general_exception'
;
$this->addError('name', $this->xpdo->lexicon($messageKey));

Check warning on line 974 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L970-L974

Added lines #L970 - L974 were not covered by tests
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
return false;
}
Expand Down Expand Up @@ -999,12 +1010,10 @@
if (!$this->filesystem->fileExists($oldPath)) {
$this->addError('name', $this->xpdo->lexicon('file_err_invalid'));
return false;
}
elseif ($this->checkFileExists() && $this->filesystem->fileExists($newPath)) {
} elseif ($this->checkFileExists() && $this->filesystem->fileExists($newPath)) {

Check warning on line 1013 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1013

Added line #L1013 was not covered by tests
$this->addError('name', sprintf($this->xpdo->lexicon('file_err_ae'), $newName));
return false;
}
elseif (!$this->checkFileType($newName)) {
} elseif (!$this->checkFileType($newName)) {

Check warning on line 1016 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1016

Added line #L1016 was not covered by tests
return false;
}
} catch (FilesystemException | UnableToRetrieveMetadata $e) {
Expand Down Expand Up @@ -1052,8 +1061,7 @@
try {
if (!$this->checkFileType($path)) {
return false;
}
elseif (!$this->filesystem->fileExists($path)) {
} elseif (!$this->filesystem->fileExists($path)) {

Check warning on line 1064 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1064

Added line #L1064 was not covered by tests
$this->addError('file', $this->xpdo->lexicon('file_err_nf') . ': ' . $path);
return false;
}
Expand All @@ -1071,7 +1079,11 @@
try {
$this->filesystem->write($path, $content, $config);
} catch (FilesystemException | UnableToWriteFile $e) {
$this->addError('name', $this->xpdo->lexicon('file_folder_err_update'));
$messageKey = $e instanceof UnableToWriteFile
? 'file_err_update_write_exception'
: 'file_err_update_write_general'
;
$this->addError('name', $this->xpdo->lexicon($messageKey));

Check warning on line 1086 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1082-L1086

Added lines #L1082 - L1086 were not covered by tests
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
return false;
}
Expand Down Expand Up @@ -1135,7 +1147,7 @@
continue;
}

if ((boolean)$this->xpdo->getOption('upload_translit')) {
if ((bool)$this->xpdo->getOption('upload_translit')) {

Check warning on line 1150 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1150

Added line #L1150 was not covered by tests
$newName = $this->xpdo->filterPathSegment($file['name']);

// If the file name is different after filtering, call OnFileManagerFileRename
Expand Down Expand Up @@ -1176,7 +1188,7 @@
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
}
}

$objects[$key] = $file;
}

Expand Down Expand Up @@ -1228,7 +1240,6 @@
$this->filesystem->setVisibility($path, $visibility);
return true;
}

} catch (FilesystemException | UnableToSetVisibility $e) {
$this->addError('path', $this->xpdo->lexicon('file_err_nf') . ' ' . $e->getMessage());
}
Expand Down Expand Up @@ -1528,13 +1539,15 @@

if (!empty($propertyArray['options'])) {
foreach ($propertyArray['options'] as $optionKey => &$option) {
if (empty($option['text']) && !empty($option['name'])) $option['text'] = $option['name'];
if (empty($option['text']) && !empty($option['name'])) {
$option['text'] = $option['name'];

Check warning on line 1543 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1542-L1543

Added lines #L1542 - L1543 were not covered by tests
}
unset($option['menu'], $option['name']);
}
}

if ($propertyArray['type'] == 'combo-boolean' && is_numeric($propertyArray['value'])) {
$propertyArray['value'] = (boolean)$propertyArray['value'];
$propertyArray['value'] = (bool)$propertyArray['value'];

Check warning on line 1550 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1550

Added line #L1550 was not covered by tests
}

$propertiesArray[$key] = $propertyArray;
Expand Down Expand Up @@ -1567,7 +1580,7 @@
return '';
}
} catch (FilesystemException | UnableToRetrieveMetadata $e) {
$this->xpdo->log(modX::LOG_LEVEL_ERROR,$e->getMessage());
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());

Check warning on line 1583 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1583

Added line #L1583 was not covered by tests
return '';
}

Expand Down Expand Up @@ -1630,9 +1643,9 @@
$enabled = true;
$context = 'mgr';
if ($context === $this->xpdo->context->get('key')) {
$enabled = (boolean)$this->xpdo->getOption('access_media_source_enabled', null, true);
$enabled = (bool)$this->xpdo->getOption('access_media_source_enabled', null, true);

Check warning on line 1646 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1646

Added line #L1646 was not covered by tests
} elseif ($obj = $this->xpdo->getContext($context)) {
$enabled = (boolean)$obj->getOption('access_media_source_enabled', true);
$enabled = (bool)$obj->getOption('access_media_source_enabled', true);

Check warning on line 1648 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1648

Added line #L1648 was not covered by tests
}
if ($enabled) {
if (empty($this->_policies) || !isset($this->_policies[$context])) {
Expand Down Expand Up @@ -1729,9 +1742,9 @@

$options[xPDO::OPT_CACHE_KEY] = $this->getOption('cache_media_sources_key', $options, 'media_sources');
$options[xPDO::OPT_CACHE_HANDLER] = $this->getOption('cache_media_sources_handler', $options, $this->getOption(xPDO::OPT_CACHE_HANDLER, $options));
$options[xPDO::OPT_CACHE_FORMAT] = (integer)$this->getOption('cache_media_sources_format', $options, $this->getOption(xPDO::OPT_CACHE_FORMAT, $options, xPDOCacheManager::CACHE_PHP));
$options[xPDO::OPT_CACHE_ATTEMPTS] = (integer)$this->getOption('cache_media_sources_attempts', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPTS, $options, 10));
$options[xPDO::OPT_CACHE_ATTEMPT_DELAY] = (integer)$this->getOption('cache_media_sources_attempt_delay', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPT_DELAY, $options, 1000));
$options[xPDO::OPT_CACHE_FORMAT] = (int)$this->getOption('cache_media_sources_format', $options, $this->getOption(xPDO::OPT_CACHE_FORMAT, $options, xPDOCacheManager::CACHE_PHP));
$options[xPDO::OPT_CACHE_ATTEMPTS] = (int)$this->getOption('cache_media_sources_attempts', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPTS, $options, 10));
$options[xPDO::OPT_CACHE_ATTEMPT_DELAY] = (int)$this->getOption('cache_media_sources_attempt_delay', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPT_DELAY, $options, 1000));

Check warning on line 1747 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L1745-L1747

Added lines #L1745 - L1747 were not covered by tests

if ($c->prepare() && $c->stmt->execute()) {
while ($row = $c->stmt->fetch(PDO::FETCH_ASSOC)) {
Expand Down Expand Up @@ -1833,7 +1846,7 @@
$editAction = $this->getEditActionId();
$canSave = $this->checkPolicy('save');
$canRemove = $this->checkPolicy('remove');
$id = rawurlencode(htmlspecialchars_decode($path));
$id = rawurlencode(htmlspecialchars_decode($path, ENT_COMPAT));

$cls = [];

Expand Down Expand Up @@ -1894,7 +1907,15 @@
$imageWidth = $this->ctx->getOption('filemanager_image_width', 400);
$imageHeight = $this->ctx->getOption('filemanager_image_height', 300);
$preview_image = $this->buildManagerImagePreview($path, $ext, $imageWidth, $imageHeight, $bases, $properties);
$file_list['qtip'] = '<img src="' . $preview_image['src'] . '" width="' . $preview_image['width'] . '" height="' . $preview_image['height'] . '" alt="' . $path . '" />';
// Once minimum php requirement is brought up to 7.4+, heredoc closing can be indented
$file_list['qtip'] = <<<QTIP
<img
src="{$preview_image['src']}"
width="{$preview_image['width']}"
height="{$preview_image['height']}"
alt="{$path}"
>
QTIP;
}
}

Expand Down Expand Up @@ -2343,7 +2364,7 @@
try {
$file = $this->filesystem->read($path);
} catch (FilesystemException $e) {
$this->addError('file',$e->getMessage());
$this->addError('file', $e->getMessage());

Check warning on line 2367 in core/src/Revolution/Sources/modMediaSource.php

View check run for this annotation

Codecov / codecov/patch

core/src/Revolution/Sources/modMediaSource.php#L2367

Added line #L2367 was not covered by tests
}
$size = @getimagesize($this->getBasePath() . $path);
if (is_array($size)) {
Expand Down
Loading