Skip to content

Commit 91b8ec3

Browse files
magento-engcom-teammagento-engcom-team
committed
ENGCOM-6529: reduce reset data actions on DeploymentConfig #24648
- Merge Pull Request #24648 from georgebabarus/magento2:deployment-config-unnecessary-reset-data - Merged commits: 1. d56f3c6 2. 43abaec 3. 662035b
2 parents 3811867 + 662035b commit 91b8ec3

File tree

6 files changed

+124
-74
lines changed

6 files changed

+124
-74
lines changed

‎dev/tests/integration/testsuite/Magento/Developer/Model/Logger/Handler/DebugTest.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ private function reinitDeploymentConfig()
173173
{
174174
$this->etcDirectory->delete(self::$configFile);
175175
$this->etcDirectory->copyFile(self::$backupFile, self::$configFile);
176+
$this->deploymentConfig->resetData();
176177
}
177178

178179
/**

‎lib/internal/Magento/Framework/App/DeploymentConfig.php

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@
77
namespace Magento\Framework\App;
88

99
use Magento\Framework\Config\ConfigOptionsListConstants;
10+
use Magento\Framework\Exception\FileSystemException;
11+
use Magento\Framework\Exception\RuntimeException;
12+
use Magento\Framework\Phrase;
1013

1114
/**
1215
* Application deployment configuration
@@ -63,6 +66,8 @@ public function __construct(DeploymentConfig\Reader $reader, $overrideData = [])
6366
* @param string $key
6467
* @param mixed $defaultValue
6568
* @return mixed|null
69+
* @throws FileSystemException
70+
* @throws RuntimeException
6671
*/
6772
public function get($key = null, $defaultValue = null)
6873
{
@@ -82,10 +87,11 @@ public function get($key = null, $defaultValue = null)
8287
* Checks if data available
8388
*
8489
* @return bool
90+
* @throws FileSystemException
91+
* @throws RuntimeException
8592
*/
8693
public function isAvailable()
8794
{
88-
$this->data = null;
8995
$this->load();
9096
return isset($this->flatData[ConfigOptionsListConstants::CONFIG_PATH_INSTALL_DATE]);
9197
}
@@ -95,6 +101,8 @@ public function isAvailable()
95101
*
96102
* @param string $key
97103
* @return null|mixed
104+
* @throws FileSystemException
105+
* @throws RuntimeException
98106
*/
99107
public function getConfigData($key = null)
100108
{
@@ -104,11 +112,7 @@ public function getConfigData($key = null)
104112
return null;
105113
}
106114

107-
if (isset($this->data[$key])) {
108-
return $this->data[$key];
109-
}
110-
111-
return $this->data;
115+
return $this->data[$key] ?? $this->data;
112116
}
113117

114118
/**
@@ -125,6 +129,8 @@ public function resetData()
125129
* Check if data from deploy files is available
126130
*
127131
* @return bool
132+
* @throws FileSystemException
133+
* @throws RuntimeException
128134
* @since 100.1.3
129135
*/
130136
public function isDbAvailable()
@@ -137,6 +143,8 @@ public function isDbAvailable()
137143
* Loads the configuration data
138144
*
139145
* @return void
146+
* @throws FileSystemException
147+
* @throws RuntimeException
140148
*/
141149
private function load()
142150
{
@@ -158,28 +166,31 @@ private function load()
158166
*
159167
* @param array $params
160168
* @param string $path
169+
* @param array $flattenResult
161170
* @return array
162-
* @throws \Exception
171+
* @throws RuntimeException
163172
*/
164-
private function flattenParams(array $params, $path = null)
173+
private function flattenParams(array $params, $path = null, array &$flattenResult = null) : array
165174
{
166-
$cache = [];
175+
if (null === $flattenResult) {
176+
$flattenResult = [];
177+
}
167178

168179
foreach ($params as $key => $param) {
169180
if ($path) {
170181
$newPath = $path . '/' . $key;
171182
} else {
172183
$newPath = $key;
173184
}
174-
if (isset($cache[$newPath])) {
175-
throw new \Exception("Key collision {$newPath} is already defined.");
185+
if (isset($flattenResult[$newPath])) {
186+
throw new RuntimeException(new Phrase("Key collision '%1' is already defined.", [$newPath]));
176187
}
177-
$cache[$newPath] = $param;
188+
$flattenResult[$newPath] = $param;
178189
if (is_array($param)) {
179-
$cache = array_merge($cache, $this->flattenParams($param, $newPath));
190+
$this->flattenParams($param, $newPath, $flattenResult);
180191
}
181192
}
182193

183-
return $cache;
194+
return $flattenResult;
184195
}
185196
}

‎lib/internal/Magento/Framework/App/Test/Unit/DeploymentConfigTest.php

Lines changed: 71 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -14,27 +14,33 @@ class DeploymentConfigTest extends \PHPUnit\Framework\TestCase
1414
/**
1515
* @var array
1616
*/
17-
private static $fixture = [
18-
'configData1' => 'scalar_value',
19-
'configData2' => [
20-
'foo' => 1,
21-
'bar' => ['baz' => 2],
22-
],
23-
];
17+
private static $fixture
18+
= [
19+
'configData1' => 'scalar_value',
20+
'configData2' => [
21+
'foo' => 1,
22+
'bar' => ['baz' => 2],
23+
],
24+
'configData3' => null,
25+
'test_override' => 'original',
26+
];
2427

2528
/**
2629
* @var array
2730
*/
28-
private static $flattenedFixture = [
29-
'configData1' => 'scalar_value',
30-
'configData2' => [
31-
'foo' => 1,
32-
'bar' => ['baz' => 2],
33-
],
34-
'configData2/foo' => 1,
35-
'configData2/bar' => ['baz' => 2],
36-
'configData2/bar/baz' => 2,
37-
];
31+
private static $flattenedFixture
32+
= [
33+
'configData1' => 'scalar_value',
34+
'configData2' => [
35+
'foo' => 1,
36+
'bar' => ['baz' => 2],
37+
],
38+
'configData2/foo' => 1,
39+
'configData2/bar' => ['baz' => 2],
40+
'configData2/bar/baz' => 2,
41+
'configData3' => null,
42+
'test_override' => 'overridden',
43+
];
3844

3945
/**
4046
* @var array
@@ -63,55 +69,65 @@ class DeploymentConfigTest extends \PHPUnit\Framework\TestCase
6369

6470
public static function setUpBeforeClass()
6571
{
66-
self::$fixtureConfig = require __DIR__ . '/_files/config.php';
67-
self::$fixtureConfigMerged = require __DIR__ . '/_files/other/local_developer_merged.php';
72+
self::$fixtureConfig = require __DIR__.'/_files/config.php';
73+
self::$fixtureConfigMerged = require __DIR__.'/_files/other/local_developer_merged.php';
6874
}
6975

7076
protected function setUp()
7177
{
72-
$this->reader = $this->createMock(\Magento\Framework\App\DeploymentConfig\Reader::class);
73-
$this->_deploymentConfig = new \Magento\Framework\App\DeploymentConfig($this->reader, []);
78+
$this->reader = $this->createMock(\Magento\Framework\App\DeploymentConfig\Reader::class);
79+
$this->_deploymentConfig = new \Magento\Framework\App\DeploymentConfig(
80+
$this->reader,
81+
['test_override' => 'overridden']
82+
);
7483
$this->_deploymentConfigMerged = new \Magento\Framework\App\DeploymentConfig(
7584
$this->reader,
76-
require __DIR__ . '/_files/other/local_developer.php'
85+
require __DIR__.'/_files/other/local_developer.php'
7786
);
7887
}
7988

80-
public function testGetters()
89+
public function testGetters(): void
8190
{
8291
$this->reader->expects($this->once())->method('load')->willReturn(self::$fixture);
8392
$this->assertSame(self::$flattenedFixture, $this->_deploymentConfig->get());
8493
// second time to ensure loader will be invoked only once
8594
$this->assertSame(self::$flattenedFixture, $this->_deploymentConfig->get());
8695
$this->assertSame('scalar_value', $this->_deploymentConfig->getConfigData('configData1'));
8796
$this->assertSame(self::$fixture['configData2'], $this->_deploymentConfig->getConfigData('configData2'));
97+
$this->assertSame(self::$fixture['configData3'], $this->_deploymentConfig->getConfigData('configData3'));
98+
$this->assertSame('', $this->_deploymentConfig->get('configData3'));
99+
$this->assertSame('defaultValue', $this->_deploymentConfig->get('invalid_key', 'defaultValue'));
100+
$this->assertNull($this->_deploymentConfig->getConfigData('invalid_key'));
101+
$this->assertSame('overridden', $this->_deploymentConfig->get('test_override'));
88102
}
89103

90-
public function testIsAvailable()
104+
public function testIsAvailable(): void
91105
{
92-
$this->reader->expects($this->once())->method('load')->willReturn([
93-
ConfigOptionsListConstants::CONFIG_PATH_INSTALL_DATE => 1
94-
]);
106+
$this->reader->expects($this->once())->method('load')->willReturn(
107+
[
108+
ConfigOptionsListConstants::CONFIG_PATH_INSTALL_DATE => 1,
109+
]
110+
);
95111
$object = new DeploymentConfig($this->reader);
96112
$this->assertTrue($object->isAvailable());
97113
}
98114

99-
public function testNotAvailable()
115+
public function testNotAvailable(): void
100116
{
101117
$this->reader->expects($this->once())->method('load')->willReturn([]);
102118
$object = new DeploymentConfig($this->reader);
103119
$this->assertFalse($object->isAvailable());
104120
}
105121

106-
public function testNotAvailableThenAvailable()
122+
/**
123+
* test if the configuration changes during the same request, the configuration remain the same
124+
*/
125+
public function testNotAvailableThenAvailable(): void
107126
{
108-
$this->reader->expects($this->at(0))->method('load')->willReturn([]);
109-
$this->reader->expects($this->at(1))->method('load')->willReturn([
110-
ConfigOptionsListConstants::CONFIG_PATH_INSTALL_DATE => 1
111-
]);
127+
$this->reader->expects($this->once())->method('load')->willReturn([]);
112128
$object = new DeploymentConfig($this->reader);
113129
$this->assertFalse($object->isAvailable());
114-
$this->assertTrue($object->isAvailable());
130+
$this->assertFalse($object->isAvailable());
115131
}
116132

117133
/**
@@ -120,7 +136,7 @@ public function testNotAvailableThenAvailable()
120136
* @expectedExceptionMessage Key collision
121137
* @dataProvider keyCollisionDataProvider
122138
*/
123-
public function testKeyCollision(array $data)
139+
public function testKeyCollision(array $data): void
124140
{
125141
$this->reader->expects($this->once())->method('load')->willReturn($data);
126142
$object = new DeploymentConfig($this->reader);
@@ -130,14 +146,32 @@ public function testKeyCollision(array $data)
130146
/**
131147
* @return array
132148
*/
133-
public function keyCollisionDataProvider()
149+
public function keyCollisionDataProvider(): array
134150
{
135151
return [
136152
[
137153
['foo' => ['bar' => '1'], 'foo/bar' => '2'],
138154
['foo/bar' => '1', 'foo' => ['bar' => '2']],
139155
['foo' => ['subfoo' => ['subbar' => '1'], 'subfoo/subbar' => '2'], 'bar' => '3'],
140-
]
156+
],
141157
];
142158
}
159+
160+
public function testResetData(): void
161+
{
162+
$this->reader->expects($this->exactly(2))->method('load')->willReturn(self::$fixture);
163+
$this->assertSame(self::$flattenedFixture, $this->_deploymentConfig->get());
164+
$this->_deploymentConfig->resetData();
165+
// second time to ensure loader will be invoked only once after reset
166+
$this->assertSame(self::$flattenedFixture, $this->_deploymentConfig->get());
167+
$this->assertSame(self::$flattenedFixture, $this->_deploymentConfig->get());
168+
}
169+
170+
public function testIsDbAvailable(): void
171+
{
172+
$this->reader->expects($this->exactly(2))->method('load')->willReturnOnConsecutiveCalls([], ['db' => []]);
173+
$this->assertFalse($this->_deploymentConfig->isDbAvailable());
174+
$this->_deploymentConfig->resetData();
175+
$this->assertTrue($this->_deploymentConfig->isDbAvailable());
176+
}
143177
}

‎lib/internal/Magento/Framework/Module/ModuleList.php

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public function __construct(DeploymentConfig $config, ModuleList\Loader $loader)
5959
}
6060

6161
/**
62-
* {@inheritdoc}
62+
* @inheritdoc
6363
*
6464
* Note that this triggers loading definitions of all existing modules in the system.
6565
* Use this method only when you actually need modules' declared meta-information.
@@ -84,8 +84,7 @@ public function getAll()
8484
}
8585

8686
/**
87-
* {@inheritdoc}
88-
* @see has()
87+
* @inheritdoc
8988
*/
9089
public function getOne($name)
9190
{
@@ -94,7 +93,7 @@ public function getOne($name)
9493
}
9594

9695
/**
97-
* {@inheritdoc}
96+
* @inheritdoc
9897
*/
9998
public function getNames()
10099
{
@@ -107,7 +106,7 @@ public function getNames()
107106
}
108107

109108
/**
110-
* {@inheritdoc}
109+
* @inheritdoc
111110
*/
112111
public function has($name)
113112
{
@@ -136,12 +135,16 @@ public function isModuleInfoAvailable()
136135
* Loads configuration data only
137136
*
138137
* @return void
138+
* @throws \Magento\Framework\Exception\FileSystemException
139+
* @throws \Magento\Framework\Exception\RuntimeException
139140
*/
140141
private function loadConfigData()
141142
{
142-
$this->config->resetData();
143-
if (null === $this->configData && null !== $this->config->get(ConfigOptionsListConstants::KEY_MODULES)) {
144-
$this->configData = $this->config->get(ConfigOptionsListConstants::KEY_MODULES);
143+
if (null === $this->configData) {
144+
$this->config->resetData();
145+
if (null !== $this->config->get(ConfigOptionsListConstants::KEY_MODULES)) {
146+
$this->configData = $this->config->get(ConfigOptionsListConstants::KEY_MODULES);
147+
}
145148
}
146149
}
147150
}

‎lib/internal/Magento/Framework/Module/Test/Unit/ModuleListTest.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ protected function setUp()
4747

4848
public function testGetAll()
4949
{
50-
$this->config->expects($this->exactly(2))->method('resetData');
50+
$this->config->expects($this->once())->method('resetData');
5151
$this->setLoadAllExpectation();
5252
$this->setLoadConfigExpectation();
5353
$expected = ['foo' => self::$allFixture['foo']];
@@ -65,7 +65,7 @@ public function testGetAllNoData()
6565

6666
public function testGetOne()
6767
{
68-
$this->config->expects($this->exactly(2))->method('resetData');
68+
$this->config->expects($this->once())->method('resetData');
6969
$this->setLoadAllExpectation();
7070
$this->setLoadConfigExpectation();
7171
$this->assertSame(['key' => 'value'], $this->model->getOne('foo'));
@@ -74,7 +74,7 @@ public function testGetOne()
7474

7575
public function testGetNames()
7676
{
77-
$this->config->expects($this->exactly(2))->method('resetData');
77+
$this->config->expects($this->once())->method('resetData');
7878
$this->setLoadAllExpectation(false);
7979
$this->setLoadConfigExpectation();
8080
$this->assertSame(['foo'], $this->model->getNames());
@@ -83,7 +83,7 @@ public function testGetNames()
8383

8484
public function testHas()
8585
{
86-
$this->config->expects($this->exactly(2))->method('resetData');
86+
$this->config->expects($this->once())->method('resetData');
8787
$this->setLoadAllExpectation(false);
8888
$this->setLoadConfigExpectation();
8989
$this->assertTrue($this->model->has('foo'));

0 commit comments

Comments
 (0)