Resolve variadic parameters.#883
Conversation
|
Solves #881 |
|
Any significant feature addition like this requires a new integration test, can you add it? I'm not sure how it behaves with the compiled container. |
|
@mnapoli You're right. Added test and handling of variadic parameter for |
| 'blank_line_between_import_groups' => false, | ||
| 'global_namespace_import' => false, | ||
| 'nullable_type_declaration_for_default_null_value' => false, | ||
| 'nullable_type_declaration' => false, |
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh you mean that fixes master?
A bit more clarity would help here 🙂
There was a problem hiding this comment.
Yes, this change disables the PhpCsFixer nullable_type_declaration check, preventing the master build from failing. Since PHP 8.4 will deprecate this kind of declaration, I suggest creating a separate Issue/PR to address it. You can then decide whether to standardize on union types or the question mark operator (whichever you think is preferable).
|
|
||
| final class Issue881Test extends BaseContainerTest | ||
| { | ||
| public function testContainerWithVariadicParameters(): void |
There was a problem hiding this comment.
Can you move this to DI\Test\IntegrationTest\Definitions\CreateDefinitionTest and change the signature like the other test methods to merge both tests in 1:
| public function testContainerWithVariadicParameters(): void | |
| /** | |
| * @dataProvider provideContainer | |
| */ | |
| public function test_with_variadics(ContainerBuilder $builder) |
|
|
||
| namespace DI\Test\IntegrationTest\Issues\Issue881; | ||
|
|
||
| final class ClassWithVariadicParameter |
There was a problem hiding this comment.
This class can be moved to DI\Test\IntegrationTest\Definitions\ObjectDefinition
|
@mnapoli Please don't merge it yet. After digging into issues #619 and #802, and considering Ben's comment I added some edge case tests. Turns out, there are still a few things I need to work through. I'll dive deeper and get this sorted as soon as I can. |
|
Summary of changes:
|
|
I can't comment on the code in general as I don't know this codebase at all, but I can confirm that this PR fixes #881 for me. |
ParameterResolver::resolveParameters()was iterating through the reflection parameters and fetching appropriate values from either named parameters or definition parameters. This meant the number of resolved arguments matched the number of reflection parameters, even if a parameter was variadic. This change adds a check for variadic parameters and adds the rest of the parameters to theargsarray accordingly.