From d2adf4f923b64441de97898f013eb82da02efa1f Mon Sep 17 00:00:00 2001 From: MGatner Date: Mon, 24 May 2021 15:51:16 -0400 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Mostafa Khudair <59371810+mostafakhudair@users.noreply.github.com> --- system/Language/en/Publisher.php | 4 +- .../Exceptions/PublisherException.php | 10 +- system/Publisher/Publisher.php | 125 ++++++++---------- 3 files changed, 60 insertions(+), 79 deletions(-) diff --git a/system/Language/en/Publisher.php b/system/Language/en/Publisher.php index ada599074efe..cb5ce81f10a4 100644 --- a/system/Language/en/Publisher.php +++ b/system/Language/en/Publisher.php @@ -11,7 +11,7 @@ // Publisher language settings return [ - 'expectedFile' => 'Publisher::{0} expects a valid file.', - 'expectedDirectory' => 'Publisher::{0} expects a valid directory.', 'collision' => 'Publisher encountered an unexpected {0} while copying {1} to {2}.', + 'expectedDirectory' => 'Publisher::{0} expects a valid directory.', + 'expectedFile' => 'Publisher::{0} expects a valid file.', ]; diff --git a/system/Publisher/Exceptions/PublisherException.php b/system/Publisher/Exceptions/PublisherException.php index 23a4b54d3e57..81f9bb4b7174 100644 --- a/system/Publisher/Exceptions/PublisherException.php +++ b/system/Publisher/Exceptions/PublisherException.php @@ -15,6 +15,11 @@ class PublisherException extends FrameworkException { + public static function forCollision(string $from, string $to) + { + return new static(lang('Publisher.collision', [filetype($to), $from, $to])); + } + public static function forExpectedDirectory(string $caller) { return new static(lang('Publisher.expectedDirectory', [$caller])); @@ -24,9 +29,4 @@ public static function forExpectedFile(string $caller) { return new static(lang('Publisher.expectedFile', [$caller])); } - - public static function forCollision(string $from, string $to) - { - return new static(lang('Publisher.collision', [filetype($to), $from, $to])); - } } diff --git a/system/Publisher/Publisher.php b/system/Publisher/Publisher.php index d7f797e00312..b2e9de8d577b 100644 --- a/system/Publisher/Publisher.php +++ b/system/Publisher/Publisher.php @@ -20,24 +20,18 @@ /** * Publisher Class * - * Publishers read in file paths from a variety - * of sources and copy the files out to different - * destinations. - * This class acts both as a base for individual - * publication directives as well as the mode of - * discovery for said instances. - * In this class a "file" is a full path to a - * verified file while a "path" is relative to - * to its source or destination and may indicate - * either a file or directory fo unconfirmed - * existence. - * Class failures throw a PublisherException, - * but some underlying methods may percolate - * different exceptions, like FileException, - * FileNotFoundException, InvalidArgumentException. - * Write operations will catch all errors in the - * file-specific $errors property to minimize - * impact of partial batch operations. + * Publishers read in file paths from a variety of sources and copy + * the files out to different destinations. This class acts both as + * a base for individual publication directives as well as the mode + * of discovery for said instances. In this class a "file" is a full + * path to a verified file while a "path" is relative to its source + * or destination and may indicate either a file or directory of + * unconfirmed existence. + * class failures throw the PublisherException, but some underlying + * methods may percolate different exceptions, like FileException, + * FileNotFoundException or InvalidArgumentException. + * Write operations will catch all errors in the file-specific + * $errors property to minimize impact of partial batch operations. */ class Publisher { @@ -90,14 +84,17 @@ class Publisher * Discovers and returns all Publishers * in the specified namespace directory. * + * @param string $directory + * * @return self[] */ - public static function discover($directory = 'Publishers'): array + public static function discover(string $directory = 'Publishers'): array { if (isset(self::$discovered[$directory])) { return self::$discovered[$directory]; } + self::$discovered[$directory] = []; /** @var FileLocator $locator */ @@ -145,8 +142,7 @@ private static function resolveDirectory(string $directory): string } /** - * Resolves a full path and verifies - * it is an actual file. + * Resolves a full path and verifies it is an actual file. * * @param string $file * @@ -184,11 +180,10 @@ private static function filterFiles(array $files, string $directory): array } /** - * Returns any files whose basename matches - * the given pattern. + * Returns any files whose `basename` matches the given pattern. * - * @param string[] $files - * @param string $pattern Regex or pseudo-regex string + * @param array $files + * @param string $pattern Regex or pseudo-regex string * * @return string[] */ @@ -239,13 +234,12 @@ private static function wipeDirectory(string $directory) } /** - * Copies a file with directory creation - * and identical file awareness. + * Copies a file with directory creation and identical file awareness. * Intentionally allows errors. * - * @param string $from - * @param string $to - * @param bool $replace + * @param string $from + * @param string $to + * @param boolean $replace * * @return void * @@ -301,8 +295,7 @@ public function __construct(string $source = null, string $destination = null) } /** - * Cleans up any temporary files - * in the scratch space. + * Cleans up any temporary files in the scratch space. */ public function __destruct() { @@ -315,14 +308,13 @@ public function __destruct() } /** - * Reads in file sources and copies out - * the files to their destinations. - * This method should be reimplemented by - * child classes intended for discovery. + * Reads files in the sources and copies them out to their destinations. + * This method should be reimplemented by child classes intended for + * discovery. * - * @return bool + * @return boolean */ - public function publish() + public function publish(): bool { return $this->addPath('/')->merge(true); } @@ -347,8 +339,7 @@ protected function getScratch(): string } /** - * Returns any errors from the last - * write operation. + * Returns errors from the last write operation if any. * * @return array */ @@ -358,8 +349,7 @@ public function getErrors(): array } /** - * Optimizes and returns the - * current file list. + * Optimizes and returns the current file list. * * @return string[] */ @@ -372,11 +362,10 @@ public function getFiles(): array } /** - * Sets the file list directly. - * Files are still subject to verification. + * Sets the file list directly, files are still subject to verification. * This works as a "reset" method with []. * - * @param string[] $files A new file list to use + * @param string[] $files The new file list to use * * @return $this */ @@ -407,8 +396,7 @@ public function addFiles(array $files) } /** - * Verifies and adds a single file - * to the file list. + * Verifies and adds a single file to the file list. * * @param string $file * @@ -469,11 +457,10 @@ public function addDirectories(array $directories, bool $recursive = false) } /** - * Verifies and adds all files - * from a directory. + * Verifies and adds all files from a directory. * - * @param string $directory - * @param bool $recursive + * @param string $directory + * @param boolean $recursive * * @return $this */ @@ -520,8 +507,8 @@ public function addPaths(array $paths, bool $recursive = true) /** * Adds a single path to the file list. * - * @param string $path - * @param bool $recursive + * @param string $path + * @param boolean $recursive * * @return $this */ @@ -563,8 +550,7 @@ public function addUris(array $uris) } /** - * Downloads a file from the URI - * and adds it to the file list. + * Downloads a file from the URI, and adds it to the file list. * * @param string $uri Because HTTP\URI is stringable it will still be accepted * @@ -576,8 +562,7 @@ public function addUri(string $uri) $file = $this->getScratch() . basename((new URI($uri))->getPath()); // Get the content and write it to the scratch space - $response = service('curlrequest')->get($uri); - write_file($file, $response->getBody()); + write_file($file, service('curlrequest')->get($uri)->getBody()); return $this->addFile($file); } @@ -585,11 +570,11 @@ public function addUri(string $uri) //-------------------------------------------------------------------- /** - * Removes any files from the list that match - * the supplied pattern (within the optional scope). + * Removes any files from the list that match the supplied pattern + * (within the optional scope). * - * @param string $pattern Regex or pseudo-regex string - * @param string|null $scope A directory to limit the scope + * @param string $pattern Regex or pseudo-regex string + * @param string|null $scope The directory to limit the scope * * @return $this */ @@ -601,9 +586,7 @@ public function removePattern(string $pattern, string $scope = null) } // Start with all files or those in scope - $files = is_null($scope) - ? $this->files - : self::filterFiles($this->files, $scope); + $files = is_null($scope) ? $this->files : self::filterFiles($this->files, $scope); // Remove any files that match the pattern return $this->removeFiles(self::matchFiles($files, $pattern)); @@ -611,9 +594,9 @@ public function removePattern(string $pattern, string $scope = null) /** * Keeps only the files from the list that match - * the supplied pattern (within the optional scope). + * (within the optional scope). * - * @param string $pattern Regex or pseudo-regex string + * @param string $pattern Regex or pseudo-regex string * @param string|null $scope A directory to limit the scope * * @return $this @@ -654,10 +637,9 @@ public function wipe() //-------------------------------------------------------------------- /** - * Copies all files into the destination. - * Does not create directory structure. + * Copies all files into the destination, does not create directory structure. * - * @param bool $replace Whether to overwrite existing files. + * @param boolean $replace Whether to overwrite existing files. * * @return boolean Whether all files were copied successfully */ @@ -684,10 +666,9 @@ public function copy(bool $replace = true): bool /** * Merges all files into the destination. - * Creates a mirrored directory structure - * only for files from source. + * Creates a mirrored directory structure only for files from source. * - * @param bool $replace Whether to overwrite existing files. + * @param boolean $replace Whether to overwrite existing files. * * @return boolean Whether all files were copied successfully */