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

renttek · 2017-09-30T08:55:05Z

Fix the command "setup:config:set --key=..." to replace existing crypt key instead of append it.

Description

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.

Fixed Issues (if relevant)

setup:config:set --key append instead of replace #11089: setup:config:set --key append instead of replace

Manual testing scenarios

Install Magento & get sure, that there is a crypt key defined in env.php
Call "bin/magento setup:config:set --key="
New crypt key in env.php should be your new key, instead of "\n"

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

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.

dmanners · 2017-09-30T08:58:35Z

setup/src/Magento/Setup/Model/ConfigGenerator.php

@@ -50,15 +51,25 @@ class ConfigGenerator
    protected $deploymentConfig;

    /**
+     * @var ConfigDataFactory
+     */
+    protected $configDataFactory;


I think this can be made private rather than protected to start with.

dmanners · 2017-09-30T08:59:55Z

setup/src/Magento/Setup/Model/ConfigGenerator.php

@@ -99,7 +109,7 @@ public function createCryptConfig(array $data)
     */
    public function createSessionConfig(array $data)
    {
-        $configData = new ConfigData(ConfigFilePool::APP_ENV);


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

dmanners · 2017-09-30T09:03:08Z

setup/src/Magento/Setup/Model/ConfigGenerator.php

@@ -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


I would suggest leaving the create as it is rather than moving it into the factory. You could always make a getConfigData method that checks for $this->configData being null and if so calls the factory create.

Something like https://github.com/magento/magento2/pull/11152/files#diff-dfb3ee5a7f863458afea75f5524cd2e5R233 might work in this case also.

dmanners · 2017-09-30T09:20:37Z

setup/src/Magento/Setup/Model/ConfigGenerator.php

+    public function __construct(
+        Random $random,
+        DeploymentConfig $deploymentConfig,
+        ConfigDataFactory $configDataFactory


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

dmanners

Add the optional dependency and I think we are good to go here.

Add CryptKeyGeneratorInterface and a default implementation "CryptKeyGenerator". Refactor ConfigGenerator to use CryptKeyGeneratorInterface

to make this code easier to understand, the chained ?? was refactored to 2 simple uses of '.. ?? ..' with some comments

Random class is now optional in constructor. If no class is given on construct, the objectmanager will be used to create an instance

Fixed Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php

renttek · 2017-09-30T10:54:00Z

I made the dependency optional.

Also, I refactored the createCryptConfig method a little bit. (Introduced a new Interface on the way)

Make it a little more simple

Put "ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT . '/'" into a variable to shorten some function calls

One test reported an undefined "offset 1" for data, that has no port. Simple solution: remove list() and just work with the array.

It has to stay there, so that no older code breaks

dmanners · 2017-09-30T12:10:36Z

lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php

+     * @param string $fileKey
+     * @return ConfigData
+     */
+    public function create(string $fileKey)


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

dmanners · 2017-09-30T12:11:22Z

setup/src/Magento/Setup/Model/ConfigGenerator.php

-    {
-        $this->random = $random;
+    public function __construct(
+        DeploymentConfig $deploymentConfig,


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

dmanners · 2017-09-30T12:11:59Z

setup/src/Magento/Setup/Model/CryptKeyGenerator.php

+     */
+    private $random;
+
+    public function __construct(Random $random = null)


Since this is a new class we do not need random to be defaulted to null.

renttek · 2017-09-30T12:12:26Z

hey @dmanners I pushed the requested changes

dmanners · 2017-09-30T12:14:12Z

setup/src/Magento/Setup/Model/ConfigGenerator.php

+        DeploymentConfig $deploymentConfig,
+        ConfigDataFactory $configDataFactory = null,
+        CryptKeyGeneratorInterface $cryptKeyGenerator = null
+    ) {
        $this->deploymentConfig = $deploymentConfig;


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

Aah, I thought I added id back.

I Think a shorter variable naming in tests would not only shorten the code, but also make it harder to understand.

- Remove scalar type and return hints - Inline mapHostData method & get rid of SupressWarning - Remove @api on new interface & implementation

dmanners · 2017-10-01T09:59:49Z

lib/internal/Magento/Framework/Config/Data/ConfigDataFactory.php

+
+/**
+ * Factory for ConfigData
+ * @api


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.

orlangur · 2017-10-02T11:13:38Z

setup/src/Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php

 {
    /**
     * @var ConfigGenerator
     */
    private $configGeneratorObject;

+    /**
+     * @SuppressWarnings(PHPMD.LongVariable)


I believe such rule was removed from Magento 2 PHPMD configuration. If it's just Codacy complains, simply ignore this and don't add suppressing annotations. Please remove both @SuppressWarnings(PHPMD.LongVariable) introduced.

Okay, I will remove them. Can such Issues be hidden/ignored on Codacy?

Don't know, you can simply ignore it overall. There are some other dumb rules like "never use else" :)

@SuppresssWarning should not be used

renttek · 2017-10-04T09:03:28Z

Hey @orlangur
I removed the SupressWarnings now

dmanners · 2017-10-06T12:36:59Z

@orlangur are you ok with the changes made? If so I will continue to process this.

orlangur

Thanks for changes.

orlangur · 2017-10-06T12:54:43Z

@dmanners yeah, please proceed, I didn't do a full review, just small note on phpmd suppression.

Looks like "Changes requested" cannot be changed to neutral resolution (and "Approved" is not suitable if I didn't dig into details).

…ead of append #11155

renttek · 2017-10-11T08:31:07Z

yeah \o/ first code in core

dmanners · 2017-10-11T08:34:42Z

@renttek congrats on the first PR. I have a t-shirt and some stickers for you from the event.

okorshenko · 2017-10-26T19:32:40Z

Hi @renttek could you please accept the invite on the GitHub to join magento team?

renttek · 2017-10-27T08:15:51Z

Hi @okorshenko I just accepted

dmanners reviewed Sep 30, 2017

View reviewed changes

okorshenko added the MageHackMuc17 label Sep 30, 2017

renttek added 2 commits September 30, 2017 11:10

Change visibility of configDataFactory-member to private

f50ad05

Remove unnecessary/old comments

4f3e981

dmanners reviewed Sep 30, 2017

View reviewed changes

Make ConfigDataFactory-parameter optional

2718be3

dmanners suggested changes Sep 30, 2017

View reviewed changes

renttek added 5 commits September 30, 2017 11:55

Refactoring of createCryptConfig

b517623

Introduce CryptKeyGgeneratorInterface

345a7f3

Add CryptKeyGeneratorInterface and a default implementation "CryptKeyGenerator". Refactor ConfigGenerator to use CryptKeyGeneratorInterface

Remove/refactor chained null coalescing operator

ae06955

to make this code easier to understand, the chained ?? was refactored to 2 simple uses of '.. ?? ..' with some comments

Make 'Random' optional

43e9751

Random class is now optional in constructor. If no class is given on construct, the objectmanager will be used to create an instance

Fix Unit/Module/ConfigGeneratorTest for Setup

4b748cf

Fixed Magento/Setup/Test/Unit/Module/ConfigGeneratorTest.php

renttek added 7 commits September 30, 2017 13:07

Refactor createCacheHostsConfig

010097d

Make it a little more simple

Reformat some code to make it consistent

2316cd8

Reformat the big array on the top

c7ca261

Shorten some function calls in createDbConfig

eb33d60

Put "ConfigOptionsListConstants::CONFIG_PATH_DB_CONNECTION_DEFAULT . '/'" into a variable to shorten some function calls

Remove list() to fix some tests

61d9767

One test reported an undefined "offset 1" for data, that has no port. Simple solution: remove list() and just work with the array.

Put "random" parameter back in ConfigGenerator

aec956f

It has to stay there, so that no older code breaks

Make "random" required CryptKeyGenerator

e440946

dmanners reviewed Sep 30, 2017

View reviewed changes

Add random as constructor param in test

885ecf8

dmanners reviewed Sep 30, 2017

View reviewed changes

dmanners added the Progress: needs update label Sep 30, 2017

dmanners added Progress: accept Release Line: 2.2 and removed Progress: needs update labels Oct 1, 2017

Suppress warnings of long var names in testcases

c638fd7

I Think a shorter variable naming in tests would not only shorten the code, but also make it harder to understand.

okorshenko added this to the October 2017 milestone Oct 1, 2017

Remove type hints and @api annotation

62eff1d

- Remove scalar type and return hints - Inline mapHostData method & get rid of SupressWarning - Remove @api on new interface & implementation

dmanners reviewed Oct 1, 2017

View reviewed changes

Some more fixes for static analysis

58aab5e

orlangur requested changes Oct 2, 2017

View reviewed changes

magento-team added the Progress: needs update label Oct 4, 2017

Remove @SuppressWarning annotations form tests

e8fe7ed

@SuppresssWarning should not be used

magento-team removed the Progress: needs update label Oct 4, 2017

magento-team added the Progress: needs update label Oct 6, 2017

orlangur reviewed Oct 6, 2017

View reviewed changes

magento-team added Progress: needs update and removed Progress: needs update labels Oct 6, 2017

dmanners added the Component: Config label Oct 10, 2017

magento-team pushed a commit that referenced this pull request Oct 11, 2017

MAGETWO-80514: Refactor ConfigGenerator to replace/set crypt key inst…

fc9e07a

…ead of append #11155

magento-team merged commit e8fe7ed into magento:2.2-develop Oct 11, 2017

renttek deleted the issue/11089 branch October 11, 2017 08:30

okorshenko mentioned this pull request Oct 11, 2017

setup:config:set --key append instead of replace #11089

Closed

nmalevanec mentioned this pull request Feb 13, 2018

[Forwardport] magento/magento2#:11155 Refactor ConfigGenerator to replace/set crypt key instead of append #13630

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

renttek commented Sep 30, 2017 •

edited by dmanners

Loading

dmanners Sep 30, 2017

dmanners Sep 30, 2017

dmanners Sep 30, 2017

dmanners Sep 30, 2017

dmanners Sep 30, 2017

dmanners left a comment

renttek commented Sep 30, 2017

dmanners Sep 30, 2017

dmanners Sep 30, 2017

dmanners Sep 30, 2017

renttek commented Sep 30, 2017

dmanners Sep 30, 2017

renttek Sep 30, 2017

dmanners Oct 1, 2017

orlangur Oct 2, 2017 •

edited

Loading

renttek Oct 4, 2017

orlangur Oct 4, 2017

renttek commented Oct 4, 2017

dmanners commented Oct 6, 2017

orlangur left a comment

orlangur commented Oct 6, 2017

renttek commented Oct 11, 2017

dmanners commented Oct 11, 2017

okorshenko commented Oct 26, 2017

renttek commented Oct 27, 2017

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

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

Conversation

renttek commented Sep 30, 2017 • edited by dmanners Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanners left a comment

Choose a reason for hiding this comment

renttek commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renttek commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renttek commented Oct 4, 2017

dmanners commented Oct 6, 2017

orlangur left a comment

Choose a reason for hiding this comment

orlangur commented Oct 6, 2017

renttek commented Oct 11, 2017

dmanners commented Oct 11, 2017

okorshenko commented Oct 26, 2017

renttek commented Oct 27, 2017

renttek commented Sep 30, 2017 •

edited by dmanners

Loading

orlangur Oct 2, 2017 •

edited

Loading