From 13daa74260759b3e8dd8d36f25374daf2a069bb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 10:45:08 +0200 Subject: [PATCH 01/29] Refactor ConfigGenerator to replace crypt key instead of append Originally a call so "bin/magento setup:config:set --key='foobar'" did not replace the crypt key, but append it with a newline. This does not only "break" handling of values, encryptwed with the old key, but also don't allow use of new key, because the new key is composed of the old one, a newline and the new one. --- .../Config/Data/ConfigDataFactory.php | 25 ++++++ .../Magento/Setup/Model/ConfigGenerator.php | 44 ++++++---- .../Test/Unit/Model/ConfigGeneratorTest.php | 86 ++++++++++++++++--- 3 files changed, 127 insertions(+), 28 deletions(-) create mode 100644 lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php diff --git a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php new file mode 100644 index 0000000000000..2422a0dd7022b --- /dev/null +++ b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php @@ -0,0 +1,25 @@ +random = $random; $this->deploymentConfig = $deploymentConfig; + $this->configDataFactory = $configDataFactory; } /** @@ -70,15 +81,14 @@ public function createCryptConfig(array $data) { $currentKey = $this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY); - $configData = new ConfigData(ConfigFilePool::APP_ENV); + // TODO: refactor configData to factory in constructor + // TODO: test method 'set' with mock + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if (isset($data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY])) { - if ($currentKey !== null) { - $key = $currentKey . "\n" . $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY]; - } else { - $key = $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY]; - } - - $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, $key); + $configData->set( + ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, + $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY] + ); } else { if ($currentKey === null) { $configData->set( @@ -99,7 +109,7 @@ public function createCryptConfig(array $data) */ public function createSessionConfig(array $data) { - $configData = new ConfigData(ConfigFilePool::APP_ENV); + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if (isset($data[ConfigOptionsListConstants::INPUT_KEY_SESSION_SAVE])) { $configData->set( @@ -132,7 +142,7 @@ public function createDefinitionsConfig(array $data) */ public function createDbConfig(array $data) { - $configData = new ConfigData(ConfigFilePool::APP_ENV); + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); $optional = [ ConfigOptionsListConstants::INPUT_KEY_DB_HOST, @@ -182,7 +192,7 @@ public function createDbConfig(array $data) */ public function createResourceConfig() { - $configData = new ConfigData(ConfigFilePool::APP_ENV); + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if ($this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_RESOURCE_DEFAULT_SETUP) === null) { $configData->set(ConfigOptionsListConstants::CONFIG_PATH_RESOURCE_DEFAULT_SETUP, 'default'); @@ -198,7 +208,7 @@ public function createResourceConfig() */ public function createXFrameConfig() { - $configData = new ConfigData(ConfigFilePool::APP_ENV); + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if ($this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT) === null) { $configData->set(ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT, 'SAMEORIGIN'); } @@ -212,7 +222,7 @@ public function createXFrameConfig() */ public function createModeConfig() { - $configData = new ConfigData(ConfigFilePool::APP_ENV); + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if ($this->deploymentConfig->get(State::PARAM_MODE) === null) { $configData->set(State::PARAM_MODE, State::MODE_DEFAULT); } @@ -227,7 +237,7 @@ public function createModeConfig() */ public function createCacheHostsConfig(array $data) { - $configData = new ConfigData(ConfigFilePool::APP_ENV); + $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if (isset($data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS])) { $hostData = explode(',', $data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS]); $hosts = []; diff --git a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php index d5bce391f4cd8..35da6863e342e 100644 --- a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php @@ -5,27 +5,57 @@ */ namespace Magento\Setup\Test\Unit\Model; +use Magento\Framework\App\DeploymentConfig; use Magento\Framework\Config\ConfigOptionsListConstants; use Magento\Framework\App\State; +use Magento\Framework\Config\Data\ConfigData; +use Magento\Framework\Config\Data\ConfigDataFactory; +use Magento\Setup\Model\ConfigGenerator; class ConfigGeneratorTest extends \PHPUnit\Framework\TestCase { - /** @var \Magento\Framework\App\DeploymentConfig | \PHPUnit_Framework_MockObject_MockObject */ + /** + * @var DeploymentConfig | \PHPUnit_Framework_MockObject_MockObject + */ private $deploymentConfigMock; - /** @var \Magento\Setup\Model\ConfigGenerator | \PHPUnit_Framework_MockObject_MockObject */ + /** + * @var ConfigGenerator | \PHPUnit_Framework_MockObject_MockObject + */ private $model; + /** + * @var ConfigData|\PHPUnit_Framework_MockObject_MockObject + */ + private $configDataMock; + public function setUp() { $objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); - $this->deploymentConfigMock = $this->getMockBuilder(\Magento\Framework\App\DeploymentConfig::class) + $this->deploymentConfigMock = $this->getMockBuilder(DeploymentConfig::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->configDataMock = $this->getMockBuilder(ConfigData::class) + ->disableOriginalConstructor() + ->setMethods(['set']) + ->getMock(); + + $configDataFactoryMock = $this->getMockBuilder(ConfigDataFactory::class) ->disableOriginalConstructor() + ->setMethods(['create']) ->getMock(); + + $configDataFactoryMock->method('create') + ->willReturn($this->configDataMock); + $this->model = $objectManager->getObject( - \Magento\Setup\Model\ConfigGenerator::class, - ['deploymentConfig' => $this->deploymentConfigMock] + ConfigGenerator::class, + [ + 'deploymentConfig' => $this->deploymentConfigMock, + 'configDataFactory' => $configDataFactoryMock, + ] ); } @@ -35,8 +65,13 @@ public function testCreateXFrameConfig() ->method('get') ->with(ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT) ->willReturn(null); - $configData = $this->model->createXFrameConfig(); - $this->assertSame('SAMEORIGIN', $configData->getData()[ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT]); + + $this->configDataMock + ->expects($this->once()) + ->method('set') + ->with(ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT, 'SAMEORIGIN'); + + $this->model->createXFrameConfig(); } public function testCreateCacheHostsConfig() @@ -55,8 +90,13 @@ public function testCreateCacheHostsConfig() 'port' => '90', ], ]; - $configData = $this->model->createCacheHostsConfig($data); - $this->assertEquals($expectedData, $configData->getData()[ConfigOptionsListConstants::CONFIG_PATH_CACHE_HOSTS]); + + $this->configDataMock + ->expects($this->once()) + ->method('set') + ->with(ConfigOptionsListConstants::CONFIG_PATH_CACHE_HOSTS, $expectedData); + + $this->model->createCacheHostsConfig($data); } public function testCreateModeConfig() @@ -65,8 +105,13 @@ public function testCreateModeConfig() ->method('get') ->with(State::PARAM_MODE) ->willReturn(null); - $configData = $this->model->createModeConfig(); - $this->assertSame(State::MODE_DEFAULT, $configData->getData()[State::PARAM_MODE]); + + $this->configDataMock + ->expects($this->once()) + ->method('set') + ->with(State::PARAM_MODE, State::MODE_DEFAULT); + + $this->model->createModeConfig(); } public function testCreateModeConfigIfAlreadySet() @@ -78,4 +123,23 @@ public function testCreateModeConfigIfAlreadySet() $configData = $this->model->createModeConfig(); $this->assertSame([], $configData->getData()); } + + public function testCreateCryptKeyConfig() + { + $key = 'my-new-key'; + $data = [ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY => $key]; + + $this->deploymentConfigMock + ->method('get') + ->with(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY) + ->willReturn(null); + + $this->configDataMock + ->expects($this->once()) + ->method('set') + ->with(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, $key); + + $this->model->createCryptConfig($data); + + } } From f50ad05a3f8153df3bc21408d4e92545344f512d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 11:10:00 +0200 Subject: [PATCH 02/29] Change visibility of configDataFactory-member to private --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index d461ff5722d24..3947f57a39909 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -53,7 +53,7 @@ class ConfigGenerator /** * @var ConfigDataFactory */ - protected $configDataFactory; + private $configDataFactory; /** * Constructor From 4f3e981d7824c1e5df46497c2e0910ceb96a0109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 11:14:08 +0200 Subject: [PATCH 03/29] Remove unnecessary/old comments --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 3947f57a39909..cbf06fc8c8f81 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -81,8 +81,6 @@ public function createCryptConfig(array $data) { $currentKey = $this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY); - // TODO: refactor configData to factory in constructor - // TODO: test method 'set' with mock $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); if (isset($data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY])) { $configData->set( From 2718be371b84613008c21d07ffc89175a5507c47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 11:25:49 +0200 Subject: [PATCH 04/29] Make ConfigDataFactory-parameter optional --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index cbf06fc8c8f81..7f42c4b378fe9 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -65,11 +65,11 @@ class ConfigGenerator public function __construct( Random $random, DeploymentConfig $deploymentConfig, - ConfigDataFactory $configDataFactory + ConfigDataFactory $configDataFactory = null ) { $this->random = $random; $this->deploymentConfig = $deploymentConfig; - $this->configDataFactory = $configDataFactory; + $this->configDataFactory = $deploymentConfig ?? ObjectManager::getInstance()->get(ConfigDataFactory::class); } /** From b5176238babbdf37aec8c0845deff672092f3ed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 11:55:55 +0200 Subject: [PATCH 05/29] Refactoring of createCryptConfig --- .../Magento/Setup/Model/ConfigGenerator.php | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 7f42c4b378fe9..e79495579ae02 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -69,7 +69,7 @@ public function __construct( ) { $this->random = $random; $this->deploymentConfig = $deploymentConfig; - $this->configDataFactory = $deploymentConfig ?? ObjectManager::getInstance()->get(ConfigDataFactory::class); + $this->configDataFactory = $configDataFactory ?? ObjectManager::getInstance()->get(ConfigDataFactory::class); } /** @@ -82,23 +82,26 @@ public function createCryptConfig(array $data) $currentKey = $this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY); $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); - if (isset($data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY])) { - $configData->set( - ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, - $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY] - ); - } else { - if ($currentKey === null) { - $configData->set( - ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, - md5($this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE)) - ); - } - } + + $key = $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY] + ?? $currentKey + ?? $this->generateCryptKey(); + + $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, $key); return $configData; } + /** + * Generates a new crypt key. + * + * @return string + */ + private function generateCryptKey() + { + return md5($this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE)); + } + /** * Creates session config data * From 345a7f3b2ed8cdd93da35260a634e646277f01a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 12:08:38 +0200 Subject: [PATCH 06/29] Introduce CryptKeyGgeneratorInterface Add CryptKeyGeneratorInterface and a default implementation "CryptKeyGenerator". Refactor ConfigGenerator to use CryptKeyGeneratorInterface --- .../Magento/Setup/Model/ConfigGenerator.php | 33 ++++------- .../Magento/Setup/Model/CryptKeyGenerator.php | 51 ++++++++++++++++ .../Model/CryptKeyGeneratorInterface.php | 23 ++++++++ .../Test/Unit/Model/CryptKeyGeneratorTest.php | 58 +++++++++++++++++++ 4 files changed, 143 insertions(+), 22 deletions(-) create mode 100644 setup/src/Magento/Setup/Model/CryptKeyGenerator.php create mode 100644 setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php create mode 100644 setup/src/Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index e79495579ae02..9faa95ea07563 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -10,7 +10,6 @@ use Magento\Framework\Config\Data\ConfigData; use Magento\Framework\Config\Data\ConfigDataFactory; use Magento\Framework\Config\File\ConfigFilePool; -use Magento\Framework\Math\Random; use Magento\Framework\App\DeploymentConfig; use Magento\Framework\Config\ConfigOptionsListConstants; use Magento\Framework\App\State; @@ -40,11 +39,6 @@ class ConfigGenerator ConfigOptionsListConstants::INPUT_KEY_RESOURCE => ConfigOptionsListConstants::KEY_RESOURCE, ]; - /** - * @var Random - */ - protected $random; - /** * @var DeploymentConfig */ @@ -55,21 +49,26 @@ class ConfigGenerator */ private $configDataFactory; + /** + * @var CryptKeyGeneratorInterface + */ + private $cryptKeyGenerator; + /** * Constructor * - * @param Random $random * @param DeploymentConfig $deploymentConfig - * @param ConfigDataFactory $configDataFactory + * @param ConfigDataFactory|null $configDataFactory + * @param CryptKeyGeneratorInterface|null $cryptKeyGenerator */ public function __construct( - Random $random, DeploymentConfig $deploymentConfig, - ConfigDataFactory $configDataFactory = null + ConfigDataFactory $configDataFactory = null, + CryptKeyGeneratorInterface $cryptKeyGenerator = null ) { - $this->random = $random; $this->deploymentConfig = $deploymentConfig; $this->configDataFactory = $configDataFactory ?? ObjectManager::getInstance()->get(ConfigDataFactory::class); + $this->cryptKeyGenerator = $cryptKeyGenerator ?? ObjectManager::getInstance()->get(CryptKeyGenerator::class); } /** @@ -85,23 +84,13 @@ public function createCryptConfig(array $data) $key = $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY] ?? $currentKey - ?? $this->generateCryptKey(); + ?? $this->cryptKeyGenerator->generate(); $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, $key); return $configData; } - /** - * Generates a new crypt key. - * - * @return string - */ - private function generateCryptKey() - { - return md5($this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE)); - } - /** * Creates session config data * diff --git a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php new file mode 100644 index 0000000000000..5251d20a11a33 --- /dev/null +++ b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php @@ -0,0 +1,51 @@ +random = $random; + } + + /** + * Generates & returns a string to be used as crypt key. + * + * The key length is not a parameter, but an implementation detail. + * + * @return string + * + * @throws \Magento\Framework\Exception\LocalizedException + */ + public function generate() + { + return md5($this->getRandomString()); + } + + /** + * Returns a random string. + * + * @return string + */ + private function getRandomString() + { + return $this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE); + } +} diff --git a/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php b/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php new file mode 100644 index 0000000000000..e75bb6eb668af --- /dev/null +++ b/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php @@ -0,0 +1,23 @@ +randomMock = $this->getMockBuilder(Random::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->cryptKeyGenerator = new CryptKeyGenerator($this->randomMock); + } + + public function testStringForHashingIsReadFromRandom() + { + $this->randomMock + ->expects($this->once()) + ->method('getRandomString'); + + $this->cryptKeyGenerator->generate(); + } + + public function testReturnsMd5OfRandomString() + { + $expected = 'fdb7594e77f1ad5fbb8e6c917b6012ce'; // == 'magento2' + + $this->randomMock + ->method('getRandomString') + ->willReturn('magento2'); + + $actual = $this->cryptKeyGenerator->generate(); + + $this->assertEquals($expected, $actual); + } +} From ae069553295c6aa26c97c9525d786599d3576e7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 12:46:42 +0200 Subject: [PATCH 07/29] Remove/refactor chained null coalescing operator to make this code easier to understand, the chained ?? was refactored to 2 simple uses of '.. ?? ..' with some comments --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 9faa95ea07563..2345c361a5d2c 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -82,9 +82,13 @@ public function createCryptConfig(array $data) $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); - $key = $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY] - ?? $currentKey - ?? $this->cryptKeyGenerator->generate(); + // Use given key if set, else use current + $key = $data[ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY] ?? $currentKey; + + // If there is no key given or currently set, generate a new one + $key = $key ?? $this->cryptKeyGenerator->generate(); + + // Chaining of ".. ?? .." is not used to keep it simpler to understand $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, $key); From 43e975179e2da43b6b79c763c3d30f01b0eba531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 12:48:10 +0200 Subject: [PATCH 08/29] Make 'Random' optional Random class is now optional in constructor. If no class is given on construct, the objectmanager will be used to create an instance --- setup/src/Magento/Setup/Model/CryptKeyGenerator.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php index 5251d20a11a33..e3afc3eb492e0 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php @@ -6,6 +6,7 @@ namespace Magento\Setup\Model; +use Magento\Framework\App\ObjectManager; use Magento\Framework\Config\ConfigOptionsListConstants; use Magento\Framework\Math\Random; @@ -20,9 +21,9 @@ class CryptKeyGenerator implements CryptKeyGeneratorInterface */ private $random; - public function __construct(Random $random) + public function __construct(Random $random = null) { - $this->random = $random; + $this->random = $random ?? ObjectManager::getInstance()->get(Random::class); } /** From 4b748cf93db79e525bfb4d1d646db04e6354930a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 12:49:13 +0200 Subject: [PATCH 09/29] Fix Unit/Module/ConfigGeneratorTest for Setup Fixed Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php --- .../Test/Unit/Module/ConfigGeneratorTest.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php index 69f320bccd5ce..4a793bdc43ea8 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php @@ -5,9 +5,12 @@ */ namespace Magento\Setup\Test\Unit\Module; +use Magento\Framework\App\DeploymentConfig; use Magento\Framework\Config\File\ConfigFilePool; +use Magento\Framework\Math\Random; use Magento\Setup\Model\ConfigGenerator; use Magento\Framework\Config\ConfigOptionsListConstants; +use Magento\Setup\Model\CryptKeyGenerator; class ConfigGeneratorTest extends \PHPUnit\Framework\TestCase { @@ -18,11 +21,17 @@ class ConfigGeneratorTest extends \PHPUnit\Framework\TestCase protected function setUp() { - $random = $this->createMock(\Magento\Framework\Math\Random::class); - $random->expects($this->any())->method('getRandomString')->willReturn('key'); - $deployConfig= $this->createMock(\Magento\Framework\App\DeploymentConfig::class); + /** @var DeploymentConfig|\PHPUnit_Framework_MockObject_MockObject $deployConfig */ + $deployConfig = $this->createMock(DeploymentConfig::class); $deployConfig->expects($this->any())->method('isAvailable')->willReturn(false); - $this->configGeneratorObject = new ConfigGenerator($random, $deployConfig); + + /** @var Random|\PHPUnit_Framework_MockObject_MockObject $randomMock */ + $randomMock = $this->createMock(Random::class); + $randomMock->expects($this->any())->method('getRandomString')->willReturn('key'); + + $cryptKeyGenerator = new CryptKeyGenerator($randomMock); + + $this->configGeneratorObject = new ConfigGenerator($deployConfig, null, $cryptKeyGenerator); } public function testCreateCryptConfigWithInput() From 010097d9af1e32d37141444a012caeae22f9b5cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 13:07:10 +0200 Subject: [PATCH 10/29] Refactor createCacheHostsConfig Make it a little more simple --- .../Magento/Setup/Model/ConfigGenerator.php | 35 +++++++++++++------ 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 2345c361a5d2c..b128f3173bb0e 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -232,21 +232,34 @@ public function createModeConfig() public function createCacheHostsConfig(array $data) { $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); + if (isset($data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS])) { - $hostData = explode(',', $data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS]); - $hosts = []; - foreach ($hostData as $data) { - $dataArray = explode(':', trim($data)); - $host = []; - $host['host'] = $dataArray[0]; - if (isset($dataArray[1])) { - $host['port'] = $dataArray[1]; - } - $hosts[] = $host; - } + $hosts = explode(',', $data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS]); + $hosts = array_map([$this, 'mapHostData'], $hosts); $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CACHE_HOSTS, $hosts); } + $configData->setOverrideWhenSave(true); return $configData; } + + /** + * Splits host data to host and port and returns them as an assoc. array. + * + * @param string $hostData + * + * @return array + */ + private function mapHostData($hostData) + { + list($host, $port) = explode(':', trim($hostData)); + + $tmp = ['host' => $host]; + + if ($port !== null) { + $tmp['port'] = $port; + } + + return $tmp; + } } From 2316cd8b98c8eece915922ce29a679e463b2ace5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 13:08:13 +0200 Subject: [PATCH 11/29] Reformat some code to make it consistent --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index b128f3173bb0e..24ca59544a7e6 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -203,9 +203,11 @@ public function createResourceConfig() public function createXFrameConfig() { $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); + if ($this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT) === null) { $configData->set(ConfigOptionsListConstants::CONFIG_PATH_X_FRAME_OPT, 'SAMEORIGIN'); } + return $configData; } @@ -217,9 +219,11 @@ public function createXFrameConfig() public function createModeConfig() { $configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV); + if ($this->deploymentConfig->get(State::PARAM_MODE) === null) { $configData->set(State::PARAM_MODE, State::MODE_DEFAULT); } + return $configData; } From c7ca261312c22376a2227fec4b294bd84af5bfe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 13:09:30 +0200 Subject: [PATCH 12/29] Reformat the big array on the top --- .../Magento/Setup/Model/ConfigGenerator.php | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 24ca59544a7e6..7eec29730b644 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -26,17 +26,17 @@ class ConfigGenerator * @var array */ private static $paramMap = [ - ConfigOptionsListConstants::INPUT_KEY_DB_HOST => ConfigOptionsListConstants::KEY_HOST, - ConfigOptionsListConstants::INPUT_KEY_DB_NAME => ConfigOptionsListConstants::KEY_NAME, - ConfigOptionsListConstants::INPUT_KEY_DB_USER => ConfigOptionsListConstants::KEY_USER, - ConfigOptionsListConstants::INPUT_KEY_DB_PASSWORD => ConfigOptionsListConstants::KEY_PASSWORD, - ConfigOptionsListConstants::INPUT_KEY_DB_PREFIX => ConfigOptionsListConstants::KEY_PREFIX, - ConfigOptionsListConstants::INPUT_KEY_DB_MODEL => ConfigOptionsListConstants::KEY_MODEL, - ConfigOptionsListConstants::INPUT_KEY_DB_ENGINE => ConfigOptionsListConstants::KEY_ENGINE, + ConfigOptionsListConstants::INPUT_KEY_DB_HOST => ConfigOptionsListConstants::KEY_HOST, + ConfigOptionsListConstants::INPUT_KEY_DB_NAME => ConfigOptionsListConstants::KEY_NAME, + ConfigOptionsListConstants::INPUT_KEY_DB_USER => ConfigOptionsListConstants::KEY_USER, + ConfigOptionsListConstants::INPUT_KEY_DB_PASSWORD => ConfigOptionsListConstants::KEY_PASSWORD, + ConfigOptionsListConstants::INPUT_KEY_DB_PREFIX => ConfigOptionsListConstants::KEY_PREFIX, + ConfigOptionsListConstants::INPUT_KEY_DB_MODEL => ConfigOptionsListConstants::KEY_MODEL, + ConfigOptionsListConstants::INPUT_KEY_DB_ENGINE => ConfigOptionsListConstants::KEY_ENGINE, ConfigOptionsListConstants::INPUT_KEY_DB_INIT_STATEMENTS => ConfigOptionsListConstants::KEY_INIT_STATEMENTS, - ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY => ConfigOptionsListConstants::KEY_ENCRYPTION_KEY, - ConfigOptionsListConstants::INPUT_KEY_SESSION_SAVE => ConfigOptionsListConstants::KEY_SAVE, - ConfigOptionsListConstants::INPUT_KEY_RESOURCE => ConfigOptionsListConstants::KEY_RESOURCE, + ConfigOptionsListConstants::INPUT_KEY_ENCRYPTION_KEY => ConfigOptionsListConstants::KEY_ENCRYPTION_KEY, + ConfigOptionsListConstants::INPUT_KEY_SESSION_SAVE => ConfigOptionsListConstants::KEY_SAVE, + ConfigOptionsListConstants::INPUT_KEY_RESOURCE => ConfigOptionsListConstants::KEY_RESOURCE, ]; /** From eb33d6048e9bcc56199bba66eef45a628f7fae11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 13:19:47 +0200 Subject: [PATCH 13/29] Shorten some function calls in createDbConfig Put "ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT . '/'" into a variable to shorten some function calls --- .../src/Magento/Setup/Model/ConfigGenerator.php | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 7eec29730b644..1d2c504399dc6 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -155,25 +155,18 @@ public function createDbConfig(array $data) ); } + $dbConnectionPrefix = ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT . '/'; + foreach ($optional as $key) { if (isset($data[$key])) { - $configData->set( - ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT . '/' . self::$paramMap[$key], - $data[$key] - ); + $configData->set($dbConnectionPrefix . self::$paramMap[$key], $data[$key]); } } - $currentStatus = $this->deploymentConfig->get( - ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT . '/' . ConfigOptionsListConstants::KEY_ACTIVE - ); + $currentStatus = $this->deploymentConfig->get($dbConnectionPrefix . ConfigOptionsListConstants::KEY_ACTIVE); if ($currentStatus === null) { - $configData->set( - ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT - . '/' . ConfigOptionsListConstants::KEY_ACTIVE, - '1' - ); + $configData->set($dbConnectionPrefix . ConfigOptionsListConstants::KEY_ACTIVE, '1'); } return $configData; From 61d9767c9f2e62c5d4d5345566ae3d3316103baa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 13:24:09 +0200 Subject: [PATCH 14/29] Remove list() to fix some tests One test reported an undefined "offset 1" for data, that has no port. Simple solution: remove list() and just work with the array. --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 1d2c504399dc6..60236ca2f0a2b 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -249,12 +249,12 @@ public function createCacheHostsConfig(array $data) */ private function mapHostData($hostData) { - list($host, $port) = explode(':', trim($hostData)); + $hostDataParts = explode(':', trim($hostData)); - $tmp = ['host' => $host]; + $tmp = ['host' => $hostDataParts[0]]; - if ($port !== null) { - $tmp['port'] = $port; + if (isset($hostDataParts[1])) { + $tmp['port'] = $hostDataParts[1]; } return $tmp; From aec956f72243358d2d58a967a0e8f9cbcfe96dd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 14:08:00 +0200 Subject: [PATCH 15/29] Put "random" parameter back in ConfigGenerator It has to stay there, so that no older code breaks --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 60236ca2f0a2b..68c7f707302c7 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -13,6 +13,7 @@ use Magento\Framework\App\DeploymentConfig; use Magento\Framework\Config\ConfigOptionsListConstants; use Magento\Framework\App\State; +use Magento\Framework\Math\Random; /** * Creates deployment config data based on user input array @@ -44,6 +45,12 @@ class ConfigGenerator */ protected $deploymentConfig; + /** + * @var Random + * @deprecated since 100.2.0 + */ + protected $random; + /** * @var ConfigDataFactory */ @@ -57,11 +64,13 @@ class ConfigGenerator /** * Constructor * + * @param Random $random Deprecated since 100.2.0 * @param DeploymentConfig $deploymentConfig * @param ConfigDataFactory|null $configDataFactory * @param CryptKeyGeneratorInterface|null $cryptKeyGenerator */ public function __construct( + Random $random, DeploymentConfig $deploymentConfig, ConfigDataFactory $configDataFactory = null, CryptKeyGeneratorInterface $cryptKeyGenerator = null From e44094697e51fc51e97716bc2fe592a2da09ac2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 14:09:10 +0200 Subject: [PATCH 16/29] Make "random" required CryptKeyGenerator --- setup/src/Magento/Setup/Model/CryptKeyGenerator.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php index e3afc3eb492e0..5251d20a11a33 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php @@ -6,7 +6,6 @@ namespace Magento\Setup\Model; -use Magento\Framework\App\ObjectManager; use Magento\Framework\Config\ConfigOptionsListConstants; use Magento\Framework\Math\Random; @@ -21,9 +20,9 @@ class CryptKeyGenerator implements CryptKeyGeneratorInterface */ private $random; - public function __construct(Random $random = null) + public function __construct(Random $random) { - $this->random = $random ?? ObjectManager::getInstance()->get(Random::class); + $this->random = $random; } /** From 885ecf8527e1a4f6d5693672d592c142af450e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 14:10:46 +0200 Subject: [PATCH 17/29] Add random as constructor param in test --- .../src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php index 4a793bdc43ea8..a771b8d66d3f0 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php @@ -31,7 +31,7 @@ protected function setUp() $cryptKeyGenerator = new CryptKeyGenerator($randomMock); - $this->configGeneratorObject = new ConfigGenerator($deployConfig, null, $cryptKeyGenerator); + $this->configGeneratorObject = new ConfigGenerator($randomMock, $deployConfig, null, $cryptKeyGenerator); } public function testCreateCryptConfigWithInput() From 91ee8751185410203e7d20c49b89e8ae3aa252ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 14:17:16 +0200 Subject: [PATCH 18/29] Assign deprecated random parameter to property Has to be done to be backwards compatible --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 1 + 1 file changed, 1 insertion(+) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 68c7f707302c7..ba6dcf88d11c1 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -75,6 +75,7 @@ public function __construct( ConfigDataFactory $configDataFactory = null, CryptKeyGeneratorInterface $cryptKeyGenerator = null ) { + $this->random = $random; $this->deploymentConfig = $deploymentConfig; $this->configDataFactory = $configDataFactory ?? ObjectManager::getInstance()->get(ConfigDataFactory::class); $this->cryptKeyGenerator = $cryptKeyGenerator ?? ObjectManager::getInstance()->get(CryptKeyGenerator::class); From 3faf8b592932e1c3f8ab05ec2142fb509a85d29d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 14:18:56 +0200 Subject: [PATCH 19/29] Add type hints for new interface and impementation --- setup/src/Magento/Setup/Model/CryptKeyGenerator.php | 2 +- setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php index 5251d20a11a33..a5bed1ed66f52 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php @@ -34,7 +34,7 @@ public function __construct(Random $random) * * @throws \Magento\Framework\Exception\LocalizedException */ - public function generate() + public function generate() : string { return md5($this->getRandomString()); } diff --git a/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php b/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php index e75bb6eb668af..6ed14c664d489 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php @@ -19,5 +19,5 @@ interface CryptKeyGeneratorInterface * * @return string */ - public function generate(); + public function generate() : string; } From 1fd5b0e9ea91346af643c664847311b997836ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 14:20:04 +0200 Subject: [PATCH 20/29] Add type hint for ConfigDataFactory->create() --- .../Magento/Framework/Config/Data/ConfigDataFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php index 2422a0dd7022b..a1a06a572b5fe 100644 --- a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php +++ b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php @@ -18,7 +18,7 @@ class ConfigDataFactory * @param string $fileKey * @return ConfigData */ - public function create(string $fileKey) + public function create(string $fileKey) : ConfigData { return new ConfigData($fileKey); } From a2ae382a94f91ffbbb0efe9cc885c72962193d8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 15:01:40 +0200 Subject: [PATCH 21/29] Add some type hints --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 2 +- setup/src/Magento/Setup/Model/CryptKeyGenerator.php | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index ba6dcf88d11c1..9313ffcce0775 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -257,7 +257,7 @@ public function createCacheHostsConfig(array $data) * * @return array */ - private function mapHostData($hostData) + private function mapHostData(string $hostData) : array { $hostDataParts = explode(':', trim($hostData)); diff --git a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php index a5bed1ed66f52..15a3812dcb688 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php @@ -20,6 +20,11 @@ class CryptKeyGenerator implements CryptKeyGeneratorInterface */ private $random; + /** + * CryptKeyGenerator constructor. + * + * @param Random $random + */ public function __construct(Random $random) { $this->random = $random; @@ -44,7 +49,7 @@ public function generate() : string * * @return string */ - private function getRandomString() + private function getRandomString() : string { return $this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE); } From c868fa23365498f1938b01eb269b8e842ec935cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 15:20:16 +0200 Subject: [PATCH 22/29] Add a empty string return to satisfy return hints --- .../Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/setup/src/Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php index 5c6d964414a8b..d7a4d477de7b0 100644 --- a/setup/src/Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Model/CryptKeyGeneratorTest.php @@ -38,7 +38,8 @@ public function testStringForHashingIsReadFromRandom() { $this->randomMock ->expects($this->once()) - ->method('getRandomString'); + ->method('getRandomString') + ->willReturn(''); $this->cryptKeyGenerator->generate(); } From d78ae56ecd738e9370f0f214731a5a91635b3332 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 15:36:20 +0200 Subject: [PATCH 23/29] Add ObjectManager to ConfigDataFactory ObjectManager is used in ConfigDataFactory to create new instances of ConfigData --- .../Config/Data/ConfigDataFactory.php | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php index a1a06a572b5fe..ed2266b6cacb4 100644 --- a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php +++ b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php @@ -6,12 +6,30 @@ namespace Magento\Framework\Config\Data; +use Magento\Framework\App\ObjectManager; +use Magento\Framework\ObjectManagerInterface; + /** * Factory for ConfigData * @api */ class ConfigDataFactory { + /** + * @var ObjectManager + */ + private $objectManager; + + /** + * Factory constructor + * + * @param ObjectManagerInterface $objectManager + */ + public function __construct(ObjectManagerInterface $objectManager) + { + $this->objectManager = $objectManager; + } + /** * Returns a new instance of ConfigData on every call. * @@ -20,6 +38,6 @@ class ConfigDataFactory */ public function create(string $fileKey) : ConfigData { - return new ConfigData($fileKey); + return $this->objectManager->create(ConfigData::class, ['fileKey' => $fileKey]); } } From b2146dbc185fcae566835af9955d0a1356292d95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sat, 30 Sep 2017 21:42:45 +0200 Subject: [PATCH 24/29] Refactor test after modifying factory --- .../Test/Unit/Module/ConfigGeneratorTest.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php index a771b8d66d3f0..c7b2230c2c190 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php @@ -6,13 +6,17 @@ namespace Magento\Setup\Test\Unit\Module; use Magento\Framework\App\DeploymentConfig; +use Magento\Framework\Config\Data\ConfigData; +use Magento\Framework\Config\Data\ConfigDataFactory; use Magento\Framework\Config\File\ConfigFilePool; use Magento\Framework\Math\Random; +use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; use Magento\Setup\Model\ConfigGenerator; use Magento\Framework\Config\ConfigOptionsListConstants; use Magento\Setup\Model\CryptKeyGenerator; +use PHPUnit\Framework\TestCase; -class ConfigGeneratorTest extends \PHPUnit\Framework\TestCase +class ConfigGeneratorTest extends TestCase { /** * @var ConfigGenerator @@ -31,7 +35,16 @@ protected function setUp() $cryptKeyGenerator = new CryptKeyGenerator($randomMock); - $this->configGeneratorObject = new ConfigGenerator($randomMock, $deployConfig, null, $cryptKeyGenerator); + $objectManagerMock = $this->getMockBuilder(\Magento\Framework\App\ObjectManager::class) + ->disableOriginalConstructor() + ->getMock(); + + $objectManagerMock->method('create')->willReturn(new ConfigData('app_env')); + + $configDataFactoryMock = (new ObjectManager($this)) + ->getObject(ConfigDataFactory::class, ['objectManager' => $objectManagerMock]); + + $this->configGeneratorObject = new ConfigGenerator($randomMock, $deployConfig, $configDataFactoryMock, $cryptKeyGenerator); } public function testCreateCryptConfigWithInput() From bab5e3176550d402ac3cb00821c6fb566cdde748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sun, 1 Oct 2017 09:48:19 +0200 Subject: [PATCH 25/29] Fix issues of static code checks --- setup/src/Magento/Setup/Model/ConfigGenerator.php | 2 ++ .../Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php | 1 - .../Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php | 7 ++++++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 9313ffcce0775..12ac63ff755ed 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -256,6 +256,8 @@ public function createCacheHostsConfig(array $data) * @param string $hostData * * @return array + * + * @SuppressWarnings(PHPMD.UnusedPrivateMethod) */ private function mapHostData(string $hostData) : array { diff --git a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php index 35da6863e342e..be68be8bd8c2d 100644 --- a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php @@ -140,6 +140,5 @@ public function testCreateCryptKeyConfig() ->with(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY, $key); $this->model->createCryptConfig($data); - } } diff --git a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php index c7b2230c2c190..65fe4a5f47711 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php @@ -44,7 +44,12 @@ protected function setUp() $configDataFactoryMock = (new ObjectManager($this)) ->getObject(ConfigDataFactory::class, ['objectManager' => $objectManagerMock]); - $this->configGeneratorObject = new ConfigGenerator($randomMock, $deployConfig, $configDataFactoryMock, $cryptKeyGenerator); + $this->configGeneratorObject = new ConfigGenerator( + $randomMock, + $deployConfig, + $configDataFactoryMock, + $cryptKeyGenerator + ); } public function testCreateCryptConfigWithInput() From c638fd735992ec4404c99a3b42fe54dec456f302 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sun, 1 Oct 2017 10:56:52 +0200 Subject: [PATCH 26/29] Suppress warnings of long var names in testcases I Think a shorter variable naming in tests would not only shorten the code, but also make it harder to understand. --- .../src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php | 3 +++ .../src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php | 3 +++ 2 files changed, 6 insertions(+) diff --git a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php index be68be8bd8c2d..4aea7c8b25879 100644 --- a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php @@ -29,6 +29,9 @@ class ConfigGeneratorTest extends \PHPUnit\Framework\TestCase */ private $configDataMock; + /** + * @SuppressWarnings(PHPMD.LongVariable) + */ public function setUp() { $objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); diff --git a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php index 65fe4a5f47711..125cea25e79c4 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php @@ -23,6 +23,9 @@ class ConfigGeneratorTest extends TestCase */ private $configGeneratorObject; + /** + * @SuppressWarnings(PHPMD.LongVariable) + */ protected function setUp() { /** @var DeploymentConfig|\PHPUnit_Framework_MockObject_MockObject $deployConfig */ From 62eff1d5cf84b11f46541598d2ed26852fc9b071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sun, 1 Oct 2017 11:24:11 +0200 Subject: [PATCH 27/29] Remove type hints and @api annotation - Remove scalar type and return hints - Inline mapHostData method & get rid of SupressWarning - Remove @api on new interface & implementation --- .../Config/Data/ConfigDataFactory.php | 2 +- .../Magento/Setup/Model/ConfigGenerator.php | 38 ++++++++----------- .../Magento/Setup/Model/CryptKeyGenerator.php | 5 +-- .../Model/CryptKeyGeneratorInterface.php | 3 +- 4 files changed, 20 insertions(+), 28 deletions(-) diff --git a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php index ed2266b6cacb4..6ac6ad06568a2 100644 --- a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php +++ b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php @@ -36,7 +36,7 @@ public function __construct(ObjectManagerInterface $objectManager) * @param string $fileKey * @return ConfigData */ - public function create(string $fileKey) : ConfigData + public function create($fileKey) { return $this->objectManager->create(ConfigData::class, ['fileKey' => $fileKey]); } diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index 12ac63ff755ed..b357b67bae601 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -242,33 +242,27 @@ public function createCacheHostsConfig(array $data) if (isset($data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS])) { $hosts = explode(',', $data[ConfigOptionsListConstants::INPUT_KEY_CACHE_HOSTS]); - $hosts = array_map([$this, 'mapHostData'], $hosts); - $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CACHE_HOSTS, $hosts); - } - $configData->setOverrideWhenSave(true); - return $configData; - } + $hosts = array_map( + function ($hostData) { + $hostDataParts = explode(':', trim($hostData)); - /** - * Splits host data to host and port and returns them as an assoc. array. - * - * @param string $hostData - * - * @return array - * - * @SuppressWarnings(PHPMD.UnusedPrivateMethod) - */ - private function mapHostData(string $hostData) : array - { - $hostDataParts = explode(':', trim($hostData)); + $tmp = ['host' => $hostDataParts[0]]; - $tmp = ['host' => $hostDataParts[0]]; + if (isset($hostDataParts[1])) { + $tmp['port'] = $hostDataParts[1]; + } + + return $tmp; + }, + $hosts + ); - if (isset($hostDataParts[1])) { - $tmp['port'] = $hostDataParts[1]; + $configData->set(ConfigOptionsListConstants::CONFIG_PATH_CACHE_HOSTS, $hosts); } - return $tmp; + $configData->setOverrideWhenSave(true); + return $configData; } + } diff --git a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php index 15a3812dcb688..857a603631ed7 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGenerator.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGenerator.php @@ -11,7 +11,6 @@ /** * Generates a crypt - * @api */ class CryptKeyGenerator implements CryptKeyGeneratorInterface { @@ -39,7 +38,7 @@ public function __construct(Random $random) * * @throws \Magento\Framework\Exception\LocalizedException */ - public function generate() : string + public function generate() { return md5($this->getRandomString()); } @@ -49,7 +48,7 @@ public function generate() : string * * @return string */ - private function getRandomString() : string + private function getRandomString() { return $this->random->getRandomString(ConfigOptionsListConstants::STORE_KEY_RANDOM_STRING_SIZE); } diff --git a/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php b/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php index 6ed14c664d489..f96f9b37ac9a7 100644 --- a/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php +++ b/setup/src/Magento/Setup/Model/CryptKeyGeneratorInterface.php @@ -8,7 +8,6 @@ /** * Interface for crypt key generators. - * @api */ interface CryptKeyGeneratorInterface { @@ -19,5 +18,5 @@ interface CryptKeyGeneratorInterface * * @return string */ - public function generate() : string; + public function generate(); } From 58aab5e5b8ce4d9da72ee067677537f5c49aff3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Sun, 1 Oct 2017 12:26:21 +0200 Subject: [PATCH 28/29] Some more fixes for static analysis --- lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php | 1 - setup/src/Magento/Setup/Model/ConfigGenerator.php | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php index 6ac6ad06568a2..85232b35ac6f4 100644 --- a/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php +++ b/lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php @@ -11,7 +11,6 @@ /** * Factory for ConfigData - * @api */ class ConfigDataFactory { diff --git a/setup/src/Magento/Setup/Model/ConfigGenerator.php b/setup/src/Magento/Setup/Model/ConfigGenerator.php index b357b67bae601..e3ab070bd8fba 100644 --- a/setup/src/Magento/Setup/Model/ConfigGenerator.php +++ b/setup/src/Magento/Setup/Model/ConfigGenerator.php @@ -264,5 +264,4 @@ function ($hostData) { $configData->setOverrideWhenSave(true); return $configData; } - } From e8fe7ed1f3b4be7c9d3c89b19ae1463a6b553d70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20Nu=C3=9F?= Date: Wed, 4 Oct 2017 11:00:57 +0200 Subject: [PATCH 29/29] Remove @SuppressWarning annotations form tests @SuppresssWarning should not be used --- .../src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php | 3 --- .../src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php | 3 --- 2 files changed, 6 deletions(-) diff --git a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php index 4aea7c8b25879..be68be8bd8c2d 100644 --- a/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php @@ -29,9 +29,6 @@ class ConfigGeneratorTest extends \PHPUnit\Framework\TestCase */ private $configDataMock; - /** - * @SuppressWarnings(PHPMD.LongVariable) - */ public function setUp() { $objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); diff --git a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php index 125cea25e79c4..65fe4a5f47711 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php @@ -23,9 +23,6 @@ class ConfigGeneratorTest extends TestCase */ private $configGeneratorObject; - /** - * @SuppressWarnings(PHPMD.LongVariable) - */ protected function setUp() { /** @var DeploymentConfig|\PHPUnit_Framework_MockObject_MockObject $deployConfig */