Skip to content
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.

Sign up for GitHub

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

Refactor ConfigGenerator to replace/set crypt key instead of append #11155

Merged
merged 29 commits into from
Oct 11, 2017
Merged

Refactor ConfigGenerator to replace/set crypt key instead of append #11155

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
13daa74
Refactor ConfigGenerator to replace crypt key instead of append
renttek Sep 30, 2017
f50ad05
Change visibility of configDataFactory-member to private
renttek Sep 30, 2017
4f3e981
Remove unnecessary/old comments
renttek Sep 30, 2017
2718be3
Make ConfigDataFactory-parameter optional
renttek Sep 30, 2017
b517623
Refactoring of createCryptConfig
renttek Sep 30, 2017
345a7f3
Introduce CryptKeyGgeneratorInterface
renttek Sep 30, 2017
ae06955
Remove/refactor chained null coalescing operator
renttek Sep 30, 2017
43e9751
Make 'Random' optional
renttek Sep 30, 2017
4b748cf
Fix Unit/Module/ConfigGeneratorTest for Setup
renttek Sep 30, 2017
010097d
Refactor createCacheHostsConfig
renttek Sep 30, 2017
2316cd8
Reformat some code to make it consistent
renttek Sep 30, 2017
c7ca261
Reformat the big array on the top
renttek Sep 30, 2017
eb33d60
Shorten some function calls in createDbConfig
renttek Sep 30, 2017
61d9767
Remove list() to fix some tests
renttek Sep 30, 2017
aec956f
Put "random" parameter back in ConfigGenerator
renttek Sep 30, 2017
e440946
Make "random" required CryptKeyGenerator
renttek Sep 30, 2017
885ecf8
Add random as constructor param in test
renttek Sep 30, 2017
91ee875
Assign deprecated random parameter to property
renttek Sep 30, 2017
3faf8b5
Add type hints for new interface and impementation
renttek Sep 30, 2017
1fd5b0e
Add type hint for ConfigDataFactory->create()
renttek Sep 30, 2017
a2ae382
Add some type hints
renttek Sep 30, 2017
c868fa2
Add a empty string return to satisfy return hints
renttek Sep 30, 2017
d78ae56
Add ObjectManager to ConfigDataFactory
renttek Sep 30, 2017
b2146db
Refactor test after modifying factory
renttek Sep 30, 2017
bab5e31
Fix issues of static code checks
renttek Oct 1, 2017
c638fd7
Suppress warnings of long var names in testcases
renttek Oct 1, 2017
62eff1d
Remove type hints and @api annotation
renttek Oct 1, 2017
58aab5e
Some more fixes for static analysis
renttek Oct 1, 2017
e8fe7ed
Remove @SuppressWarning annotations form tests
Oct 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Framework\Config\Data;

/**
* Factory for ConfigData
* @api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So since it is a patch release we cannot add these tag currently. @renttek Can you check all the classes and remove the @api tag if you added it in.

*/
class ConfigDataFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this before but this factory should either be updated to match the standard defined in http://devdocs.magento.com/guides/v2.2/extension-dev-guide/factories.html or actually it might be possible to use the autogenerated factory here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autogenerated factories are not allowed in Magento\Framework as far as I know. But, the best option is to commit autogenerated factory here.

{
/**
* Returns a new instance of ConfigData on every call.
*
* @param string $fileKey
* @return ConfigData
*/
public function create(string $fileKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can update this to use PHP 7's return type declaration if you would like. http://php.net/manual/en/migration70.new-features.php#migration70.new-features.return-type-declarations

{
return new ConfigData($fileKey);
}
}
42 changes: 25 additions & 17 deletions setup/src/Magento/Setup/Model/ConfigGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@

namespace Magento\Setup\Model;

use Magento\Framework\App\ObjectManager;
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;
use Magento\Framework\App\ObjectManagerFactory;

/**
* Creates deployment config data based on user input array
Expand Down Expand Up @@ -49,16 +50,26 @@ class ConfigGenerator
*/
protected $deploymentConfig;

/**
* @var ConfigDataFactory
*/
private $configDataFactory;

/**
* Constructor
*
* @param Random $random
* @param DeploymentConfig $deploymentConfig
* @param ConfigDataFactory $configDataFactory
*/
public function __construct(Random $random, DeploymentConfig $deploymentConfig)
{
public function __construct(
Random $random,
DeploymentConfig $deploymentConfig,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned we need to keep the random parameter here but can mark the property as deprecated.

ConfigDataFactory $configDataFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional dependency like http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/#adding-a-constructor-parameter

) {
$this->random = $random;
$this->deploymentConfig = $deploymentConfig;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we still need to assign $this->random = $random here in-case someone is using it in an extension to this class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah, I thought I added id back.

$this->configDataFactory = $configDataFactory;
}

/**
Expand All @@ -70,15 +81,12 @@ public function createCryptConfig(array $data)
{
$currentKey = $this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_CRYPT_KEY);

$configData = new ConfigData(ConfigFilePool::APP_ENV);
$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(
Expand All @@ -99,7 +107,7 @@ public function createCryptConfig(array $data)
*/
public function createSessionConfig(array $data)
{
$configData = new ConfigData(ConfigFilePool::APP_ENV);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work, good to see this being moved out to a factory.

$configData = $this->configDataFactory->create(ConfigFilePool::APP_ENV);

if (isset($data[ConfigOptionsListConstants::INPUT_KEY_SESSION_SAVE])) {
$configData->set(
Expand Down Expand Up @@ -132,7 +140,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,
Expand Down Expand Up @@ -182,7 +190,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');
Expand All @@ -198,7 +206,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');
}
Expand All @@ -212,7 +220,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);
}
Expand All @@ -227,7 +235,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 = [];
Expand Down
86 changes: 75 additions & 11 deletions setup/src/Magento/Setup/Test/Unit/Model/ConfigGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]
);
}

Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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);

}
}