Skip to content

Commit acd9e39

Browse files
fabpotfabpot
committed
bug #942 Fixing bug where removed parameters were not handled correctly during recipes update (weaverryan)
This PR was merged into the 1.x branch. Discussion ---------- Fixing bug where removed parameters were not handled correctly during recipes update Hi again! Reported at https://symfonycasts.com/screencast/symfony6-upgrade/upgrade-recipes2#comment-5935619544 Just an oversight - the original implementation was a bit naive, not taking into account a situation where the `container` configurator in the OLD recipe contained a parameter, but this parameter was *removed* in the updated recipe. Cheers! Commits ------- 3032d76 Fixing bug where removed parameters were not handled correctly during recipes update
2 parents d1a6923 + 3032d76 commit acd9e39

File tree

2 files changed

+94
-26
lines changed

2 files changed

+94
-26
lines changed

‎src/Configurator/ContainerConfigurator.php

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -34,31 +34,32 @@ public function unconfigure(Recipe $recipe, $parameters, Lock $lock)
3434
{
3535
$this->write('Unsetting parameters');
3636
$target = $this->options->get('root-dir').'/'.$this->getServicesPath();
37-
$lines = [];
38-
foreach (file($target) as $line) {
39-
if ($this->removeParameters(1, $parameters, $line)) {
40-
continue;
41-
}
42-
$lines[] = $line;
43-
}
37+
$lines = $this->removeParametersFromLines(file($target), $parameters);
4438
file_put_contents($target, implode('', $lines));
4539
}
4640

4741
public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
4842
{
49-
if ($originalConfig) {
50-
$recipeUpdate->setOriginalFile(
51-
$this->getServicesPath(),
52-
$this->configureParameters($originalConfig, true)
53-
);
43+
$recipeUpdate->setOriginalFile(
44+
$this->getServicesPath(),
45+
$this->configureParameters($originalConfig, true)
46+
);
47+
48+
// for the new file, we need to update any values *and* remove any removed values
49+
$removedParameters = [];
50+
foreach ($originalConfig as $name => $value) {
51+
if (!isset($newConfig[$name])) {
52+
$removedParameters[$name] = $value;
53+
}
5454
}
5555

56-
if ($newConfig) {
57-
$recipeUpdate->setNewFile(
58-
$this->getServicesPath(),
59-
$this->configureParameters($newConfig, true)
60-
);
61-
}
56+
$updatedFile = $this->configureParameters($newConfig, true);
57+
$lines = $this->removeParametersFromLines(explode("\n", $updatedFile), $removedParameters);
58+
59+
$recipeUpdate->setNewFile(
60+
$this->getServicesPath(),
61+
implode("\n", $lines)
62+
);
6263
}
6364

6465
private function configureParameters(array $parameters, bool $update = false): string
@@ -114,6 +115,19 @@ private function configureParameters(array $parameters, bool $update = false): s
114115
return implode('', $lines);
115116
}
116117

118+
private function removeParametersFromLines(array $sourceLines, array $parameters): array
119+
{
120+
$lines = [];
121+
foreach ($sourceLines as $line) {
122+
if ($this->removeParameters(1, $parameters, $line)) {
123+
continue;
124+
}
125+
$lines[] = $line;
126+
}
127+
128+
return $lines;
129+
}
130+
117131
private function removeParameters($level, $params, $line)
118132
{
119133
foreach ($params as $key => $value) {

‎tests/Configurator/ContainerConfiguratorTest.php

Lines changed: 62 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public function testConfigure()
5757
services:
5858
5959
EOF
60-
, file_get_contents($config));
60+
, file_get_contents($config));
6161

6262
$configurator->unconfigure($recipe, ['locale' => 'en'], $lock);
6363
$this->assertEquals(<<<EOF
@@ -67,7 +67,7 @@ public function testConfigure()
6767
services:
6868
6969
EOF
70-
, file_get_contents($config));
70+
, file_get_contents($config));
7171
}
7272

7373
public function testConfigureWithoutParametersKey()
@@ -95,7 +95,7 @@ public function testConfigureWithoutParametersKey()
9595
services:
9696
9797
EOF
98-
, file_get_contents($config));
98+
, file_get_contents($config));
9999

100100
$configurator->unconfigure($recipe, ['locale' => 'en'], $lock);
101101
$this->assertEquals(<<<EOF
@@ -104,7 +104,7 @@ public function testConfigureWithoutParametersKey()
104104
services:
105105
106106
EOF
107-
, file_get_contents($config));
107+
, file_get_contents($config));
108108
}
109109

110110
public function testConfigureWithoutDuplicated()
@@ -135,7 +135,7 @@ public function testConfigureWithoutDuplicated()
135135
services:
136136
137137
EOF
138-
, file_get_contents($config));
138+
, file_get_contents($config));
139139

140140
$configurator->unconfigure($recipe, ['locale' => 'en'], $lock);
141141
$this->assertEquals(<<<EOF
@@ -144,7 +144,7 @@ public function testConfigureWithoutDuplicated()
144144
services:
145145
146146
EOF
147-
, file_get_contents($config));
147+
, file_get_contents($config));
148148
}
149149

150150
public function testConfigureWithComplexContent()
@@ -184,7 +184,7 @@ public function testConfigureWithComplexContent()
184184
services:
185185
186186
EOF
187-
, file_get_contents($config));
187+
, file_get_contents($config));
188188

189189
$configurator->unconfigure($recipe, ['locale' => 'en', 'foobar' => 'baz'], $lock);
190190
$this->assertEquals(<<<EOF
@@ -197,7 +197,7 @@ public function testConfigureWithComplexContent()
197197
services:
198198
199199
EOF
200-
, file_get_contents($config));
200+
, file_get_contents($config));
201201
}
202202

203203
public function testConfigureWithComplexContent2()
@@ -356,6 +356,60 @@ public function testUpdate()
356356
357357
services:
358358
359+
EOF
360+
], $recipeUpdate->getNewFiles());
361+
}
362+
363+
public function testUpdateWithNoRemovedKeysInUpdate()
364+
{
365+
$configurator = new ContainerConfigurator(
366+
$this->createMock(Composer::class),
367+
$this->createMock(IOInterface::class),
368+
new Options(['config-dir' => 'config', 'root-dir' => FLEX_TEST_DIR])
369+
);
370+
371+
$recipeUpdate = new RecipeUpdate(
372+
$this->createMock(Recipe::class),
373+
$this->createMock(Recipe::class),
374+
$this->createMock(Lock::class),
375+
FLEX_TEST_DIR
376+
);
377+
378+
@mkdir(FLEX_TEST_DIR.'/config');
379+
file_put_contents(
380+
FLEX_TEST_DIR.'/config/services.yaml',
381+
<<<EOF
382+
parameters:
383+
locale: es
384+
something: else
385+
386+
services:
387+
foo_router: '@router'
388+
EOF
389+
);
390+
391+
$configurator->update(
392+
$recipeUpdate,
393+
['locale' => 'en'],
394+
[]
395+
);
396+
397+
$this->assertSame(['config/services.yaml' => <<<EOF
398+
parameters:
399+
locale: en
400+
something: else
401+
402+
services:
403+
foo_router: '@router'
404+
EOF
405+
], $recipeUpdate->getOriginalFiles());
406+
407+
$this->assertSame(['config/services.yaml' => <<<EOF
408+
parameters:
409+
something: else
410+
411+
services:
412+
foo_router: '@router'
359413
EOF
360414
], $recipeUpdate->getNewFiles());
361415
}

0 commit comments

Comments
 (0)