Skip to content

Conversation

@pdohogne-magento
Copy link
Contributor

@pdohogne-magento pdohogne-magento commented Apr 9, 2019

Console and WebSetupPluginInstaller are now instantiated classes rather than static methods or singleton objects.

Copy link

@paliarush paliarush left a 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

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.

Copy link
Contributor Author

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;

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

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.

@pdohogne-magento pdohogne-magento deleted the MC-5465-static-refactor branch May 23, 2019 21:07
magento-devops-reposync-svc pushed a commit that referenced this pull request Oct 18, 2021
PWA-2165: Move functionality to require-commerce and drop web setup w…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants