-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added more logging and some code refactor #7
base: master
Are you sure you want to change the base?
Conversation
*/ | ||
public function delete($path) | ||
private function ensureWeCanWriteDestFile($newpath, $overwrite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have exactly the same check in src/Cmp/Storage/Adapter/FileSystemAdapter.php
maybe in future common checks between those two classes we can create a common class for check it, and avoid duplicate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some doubts about this because it's only one function, but yes, may be is a good idea prepare a trait to afford this duplicate function and future duplicate checks if any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* @param \Exception $e | ||
*/ | ||
private function logAdapterException($adapter, $e) | ||
private function logAdapterException(AdapterInterface $adapter, $e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add \Exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
* | ||
* @throws InvalidStorageAdapterException | ||
*/ | ||
public function __construct(array $config = [], $bucket = '', $pathPrefix = '') | ||
public function __construct(array $config = [], $bucket = '', $pathPrefix = '', $options = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add array or [] to $option declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
$e = new FileExistsException($newpath); | ||
$this->logger->log( | ||
LogLevel::ERROR, | ||
'Adapter "'.$this->getName().'" fails. Des file {path} already exists.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Des means destination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, fixed!
No description provided.