Skip to content

Commit 539f2f2

Browse files
theofidrytheofidry
authored
Fix child command generation (#158)
- A obscure piece of code was used for excluding the Symfony application `command` argument. This is now done in a more transparent manner and also has the benefit of working regardless of whether the command argument was here (e.g. outside of a Symfony app) or the order different for some reasons - The actual raw arguments are picked instead of `Input::getArguments()` which also returns the default values - Do not forward any of the parallelization input: only the child flag is necessary and is manually added after
1 parent b24d500 commit 539f2f2

File tree

4 files changed

+221
-77
lines changed

4 files changed

+221
-77
lines changed

‎infection.json5

-10
Original file line numberDiff line numberDiff line change
@@ -58,16 +58,6 @@
5858
"ignoreSourceCodeByRegex": [
5959
".*ResettableContainerInterface.*"
6060
]
61-
},
62-
"UnwrapArrayFilter": {
63-
"ignore": [
64-
"Webmozarts\\Console\\Parallelization\\Input\\ChildCommandFactory::createBaseCommand"
65-
]
66-
},
67-
"UnwrapArrayMap": {
68-
"ignore": [
69-
"Webmozarts\\Console\\Parallelization\\Input\\ChildCommandFactory::createBaseCommand"
70-
]
7161
}
7262
}
7363
}

‎src/Input/ChildCommandFactory.php

+34-23
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@
1919
use function array_filter;
2020
use function array_map;
2121
use function array_merge;
22-
use function array_slice;
23-
use function implode;
2422
use function Safe\getcwd;
2523
use function sprintf;
2624

@@ -55,14 +53,7 @@ public function createChildCommand(InputInterface $input): array
5553
{
5654
return array_merge(
5755
$this->createBaseCommand($input),
58-
// Forward all the options except for "processes" to the children
59-
// this way the children can inherit the options such as env
60-
// or no-debug.
61-
InputOptionsSerializer::serialize(
62-
$this->commandDefinition,
63-
$input,
64-
['child', 'processes'],
65-
),
56+
$this->getForwardedOptions($input),
6657
);
6758
}
6859

@@ -76,23 +67,43 @@ private function createBaseCommand(
7667
$this->phpExecutable,
7768
$this->scriptPath,
7869
$this->commandName,
79-
implode(
80-
' ',
81-
// TODO: this looks suspicious: why do we need to take the first arg?
82-
// why is this not a specific arg?
83-
// why do we include optional arguments? (cf. options)
84-
// maybe has to do with the item arg but in that case it is incorrect...
85-
array_filter(
86-
array_slice(
87-
array_map('strval', $input->getArguments()),
88-
1,
89-
),
90-
),
91-
),
70+
...array_map('strval', self::getArguments($input)),
9271
'--child',
9372
]);
9473
}
9574

75+
/**
76+
* @return list<string>
77+
*/
78+
private function getForwardedOptions(InputInterface $input): array
79+
{
80+
// Forward all the options except for "processes" to the children
81+
// this way the children can inherit the options such as env
82+
// or no-debug.
83+
return InputOptionsSerializer::serialize(
84+
$this->commandDefinition,
85+
$input,
86+
ParallelizationInput::OPTIONS,
87+
);
88+
}
89+
90+
/**
91+
* @return list<string|bool|int|float|null|array<string|bool|int|float|null>>
92+
*/
93+
private static function getArguments(InputInterface $input): array
94+
{
95+
$arguments = RawInput::getRawArguments($input);
96+
97+
// Remove the item: we do not want it to be passed to child processes
98+
// ever.
99+
unset(
100+
$arguments['command'],
101+
$arguments[ParallelizationInput::ITEM_ARGUMENT],
102+
);
103+
104+
return array_values($arguments);
105+
}
106+
96107
private static function validateScriptPath(string $scriptPath): void
97108
{
98109
Assert::fileExists(

‎src/Input/ParallelizationInput.php

+10-4
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,16 @@
2828

2929
final class ParallelizationInput
3030
{
31-
private const ITEM_ARGUMENT = 'item';
32-
private const PROCESSES_OPTION = 'processes';
33-
private const MAIN_PROCESS_OPTION = 'main-process';
34-
private const CHILD_OPTION = 'child';
31+
public const ITEM_ARGUMENT = 'item';
32+
public const PROCESSES_OPTION = 'processes';
33+
public const MAIN_PROCESS_OPTION = 'main-process';
34+
public const CHILD_OPTION = 'child';
35+
36+
public const OPTIONS = [
37+
self::PROCESSES_OPTION,
38+
self::MAIN_PROCESS_OPTION,
39+
self::CHILD_OPTION,
40+
];
3541

3642
private bool $mainProcess;
3743

‎tests/Input/ChildCommandFactoryTest.php

+177-40
Original file line numberDiff line numberDiff line change
@@ -63,46 +63,46 @@ public static function childProvider(): iterable
6363
$scriptPath,
6464
$commandName
6565
) {
66-
$input = new ArrayInput([
67-
'item' => 'item3',
68-
'groupId' => 'group2',
69-
'--child' => null,
70-
'--processes' => '2',
71-
'--opt' => 'val',
72-
]);
73-
74-
$commandDefinition = new InputDefinition([
75-
new InputArgument(
76-
'item',
77-
InputArgument::REQUIRED,
78-
),
79-
new InputArgument(
80-
'groupId',
81-
InputArgument::REQUIRED,
82-
),
83-
new InputArgument(
84-
'optArg',
85-
InputArgument::OPTIONAL,
86-
'',
87-
'',
88-
),
89-
new InputOption(
90-
'opt',
91-
null,
92-
InputOption::VALUE_REQUIRED,
93-
),
94-
new InputOption(
95-
'child',
96-
null,
97-
InputOption::VALUE_NONE,
98-
),
99-
new InputOption(
100-
'processes',
101-
null,
102-
InputOption::VALUE_REQUIRED,
103-
),
104-
]);
105-
$input->bind($commandDefinition);
66+
[$input, $commandDefinition] = self::createInput(
67+
[
68+
'item' => 'item3',
69+
'groupId' => 'group2',
70+
'--child' => null,
71+
'--processes' => '2',
72+
'--opt' => 'val',
73+
],
74+
[
75+
new InputArgument(
76+
'item',
77+
InputArgument::REQUIRED,
78+
),
79+
new InputArgument(
80+
'groupId',
81+
InputArgument::REQUIRED,
82+
),
83+
new InputArgument(
84+
'optArg',
85+
InputArgument::OPTIONAL,
86+
'',
87+
'',
88+
),
89+
new InputOption(
90+
'opt',
91+
null,
92+
InputOption::VALUE_REQUIRED,
93+
),
94+
new InputOption(
95+
'child',
96+
null,
97+
InputOption::VALUE_NONE,
98+
),
99+
new InputOption(
100+
'processes',
101+
null,
102+
InputOption::VALUE_REQUIRED,
103+
),
104+
],
105+
);
106106

107107
return [
108108
$phpExecutable,
@@ -120,6 +120,131 @@ public static function childProvider(): iterable
120120
],
121121
];
122122
})();
123+
124+
yield 'it removes the command & item argument' => (static function () use (
125+
$phpExecutable,
126+
$scriptPath,
127+
$commandName
128+
) {
129+
[$input, $commandDefinition] = self::createInput(
130+
[
131+
'command' => 'import:something',
132+
'groupId' => 'group2',
133+
'categoryId' => 10,
134+
'item' => 'item3',
135+
'tagIds' => 'tag1 tag2 tag3',
136+
],
137+
[
138+
new InputArgument(
139+
'groupId',
140+
InputArgument::REQUIRED,
141+
),
142+
new InputArgument(
143+
'categoryId',
144+
InputArgument::REQUIRED,
145+
),
146+
// Inverse the order to break with the old code which was
147+
// relying on item/command being the first argument.
148+
new InputArgument(
149+
'item',
150+
InputArgument::REQUIRED,
151+
),
152+
new InputArgument(
153+
'command',
154+
InputArgument::OPTIONAL,
155+
),
156+
new InputArgument(
157+
'tagIds',
158+
InputArgument::IS_ARRAY,
159+
),
160+
],
161+
);
162+
163+
return [
164+
$phpExecutable,
165+
$scriptPath,
166+
$commandName,
167+
$commandDefinition,
168+
$input,
169+
[
170+
$phpExecutable,
171+
$scriptPath,
172+
$commandName,
173+
'group2',
174+
'10',
175+
'tag1 tag2 tag3',
176+
'--child',
177+
],
178+
];
179+
})();
180+
181+
yield 'it does not forward the parallel input' => (static function () use (
182+
$phpExecutable,
183+
$scriptPath,
184+
$commandName
185+
) {
186+
[$input, $commandDefinition] = self::createInput(
187+
[
188+
'--processes' => '10',
189+
'--main-process' => null,
190+
'--child' => null,
191+
],
192+
[
193+
new InputOption(
194+
ParallelizationInput::PROCESSES_OPTION,
195+
null,
196+
InputOption::VALUE_OPTIONAL,
197+
),
198+
new InputOption(
199+
ParallelizationInput::MAIN_PROCESS_OPTION,
200+
null,
201+
InputOption::VALUE_NONE,
202+
),
203+
new InputOption(
204+
ParallelizationInput::CHILD_OPTION,
205+
null,
206+
InputOption::VALUE_NONE,
207+
),
208+
],
209+
);
210+
211+
return [
212+
$phpExecutable,
213+
$scriptPath,
214+
$commandName,
215+
$commandDefinition,
216+
$input,
217+
[
218+
$phpExecutable,
219+
$scriptPath,
220+
$commandName,
221+
'--child',
222+
],
223+
];
224+
})();
225+
226+
yield 'no PHP executable' => (static function () use (
227+
$scriptPath,
228+
$commandName
229+
) {
230+
[$input, $commandDefinition] = self::createInput(
231+
[],
232+
[],
233+
);
234+
235+
return [
236+
'',
237+
$scriptPath,
238+
$commandName,
239+
$commandDefinition,
240+
$input,
241+
[
242+
$scriptPath,
243+
$commandName,
244+
'--child',
245+
],
246+
];
247+
})();
123248
}
124249

125250
public function test_it_cannot_create_a_factory_with_an_invalid_script_path(): void
@@ -136,4 +261,16 @@ public function test_it_cannot_create_a_factory_with_an_invalid_script_path(): v
136261
new InputDefinition(),
137262
);
138263
}
264+
265+
private static function createInput(
266+
array $input,
267+
array $definition
268+
): array {
269+
$input = new ArrayInput($input);
270+
$inputDefinition = new InputDefinition($definition);
271+
272+
$input->bind($inputDefinition);
273+
274+
return [$input, $inputDefinition];
275+
}
139276
}

0 commit comments

Comments
 (0)