-
Notifications
You must be signed in to change notification settings - Fork 15
MC-5465: Changing Console and WebSetupPluginInstaller classes to not be static #4
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
MC-5465: Changing Console and WebSetupPluginInstaller classes to not be static #4
Conversation
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.
Pease review the following comments.
|
|
||
| /** | ||
| * @var bool|string $mageComposerBackup | ||
| * @var boolean|string $mageComposerBackup |
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.
Why this was changed? I think we should always use bool. There are more occurrences like this in this PR.
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 will change back to bool; other projects have both, so I just picked one to standardize here.
| use Symfony\Component\Console\Helper\HelperSet; | ||
| use Symfony\Component\Console\Helper\ProcessHelper; | ||
| use Symfony\Component\Console\Helper\QuestionHelper; | ||
| use Symfony\Component\Console\Input\ArrayInput; |
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.
This class now has a lot of dependencies, please consider splitting it. Same issue with other classes in this PR.
BTW, did we check this composer plugin with our sniffer and mess detector?
| use Magento\ComposerRootUpdatePlugin\Plugin\PluginDefinition; | ||
|
|
||
| /** | ||
| * Class WebSetupWizardPluginInstaller |
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.
Please do not keep auto-generated docs, either write good docs or remove the stubs.
PWA-2165: Move functionality to require-commerce and drop web setup w…
Console and WebSetupPluginInstaller are now instantiated classes rather than static methods or singleton objects.