MAGECLOUD-1600: Merge ece-patches into ece-tools #158
Conversation
config/patches.json
Outdated
| @@ -0,0 +1,28 @@ | |||
| { | |||
There was a problem hiding this comment.
Why we need config folder? I think it's better to put this file in root.
src/Patch/Manager.php
Outdated
| $this->directoryList = $directoryList; | ||
| } | ||
|
|
||
| public function apply() |
There was a problem hiding this comment.
Please add description to this method and @throw tags
There was a problem hiding this comment.
And I think applyAll or applyAllPatches is better name for this method
src/Patch/Manager.php
Outdated
| * | ||
| * @return void | ||
| */ | ||
| private function copyStaticFile() |
There was a problem hiding this comment.
This method should be moved from this class because the logic of this method is not related to patching.
Let's move it to some build or deploy process/
There was a problem hiding this comment.
Will be moved within next tasks
src/Patch/Manager.php
Outdated
|
|
||
| $this->logger->info('Applying hot-fixes.'); | ||
|
|
||
| $files = glob($hotFixesDir . '/*.patch', GLOB_NOSORT); |
There was a problem hiding this comment.
Looks like GLOB_NOSORT is not needed here. When this flag is not used, the pathnames are sorted alphabetically and next line can be removed.
src/Patch/Manager.php
Outdated
| /** | ||
| * Applies all needed patches. | ||
| */ | ||
| public function applyAll() |
There was a problem hiding this comment.
Please add throw tags, this method can throw RuntimeException and FileSystemException.
src/Patch/Manager.php
Outdated
| use Psr\Log\LoggerInterface; | ||
|
|
||
| /** | ||
| * Class Manager. |
There was a problem hiding this comment.
Please remove or change this description
|
QA approved |
andriyShevtsov
added
Progress: accept
src/Command/ApplyPatches.php
Outdated
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc |
src/Command/ApplyPatches.php
Outdated
| /** | ||
| * {@inheritdoc | ||
| * | ||
| * @throws \RuntimeException |
There was a problem hiding this comment.
Patch manager may throw at least 2 types of exception.
| } catch (\Exception $exception) { | ||
| $this->logger->critical($exception->getMessage()); | ||
|
|
||
| throw $exception; |
There was a problem hiding this comment.
why do you need to throw the exception again?
There was a problem hiding this comment.
It's standard flow curretly
| * Applies all needed patches. | ||
| * | ||
| * @throws \RuntimeException | ||
| * @throws FileSystemException |
There was a problem hiding this comment.
Why do you need 2 types of exceptions here?
Isn't it better to add try catch and throw only one exception?
There was a problem hiding this comment.
It's being caught in command itself
YPyltiai
added
Release: 2002.0.8
https://magento2.atlassian.net/browse/MAGECLOUD-1600
Description
Integrates patches into ECE-Tools
Zephyr Tests
Contribution checklist
MAGECLOUD-1600: Merge ece-patches into ece-tools #158