Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve variadic parameters. #883

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

serge-kvashnin
Copy link

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 the args array accordingly.

@serge-kvashnin
Copy link
Author

Solves #881

@mnapoli
Copy link
Member

mnapoli commented Apr 17, 2024

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.

@serge-kvashnin
Copy link
Author

@mnapoli You're right. Added test and handling of variadic parameter for ObjectCreationCompiler.

@@ -58,6 +58,7 @@
'blank_line_between_import_groups' => false,
'global_namespace_import' => false,
'nullable_type_declaration_for_default_null_value' => false,
'nullable_type_declaration' => false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you mean that fixes master?

A bit more clarity would help here 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

tests/IntegrationTest/CompiledContainerTest.php Outdated Show resolved Hide resolved

final class Issue881Test extends BaseContainerTest
{
public function testContainerWithVariadicParameters(): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
public function testContainerWithVariadicParameters(): void
/**
* @dataProvider provideContainer
*/
public function test_with_variadics(ContainerBuilder $builder)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 8854fe6.


namespace DI\Test\IntegrationTest\Issues\Issue881;

final class ClassWithVariadicParameter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class can be moved to DI\Test\IntegrationTest\Definitions\ObjectDefinition

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 8854fe6.

@serge-kvashnin
Copy link
Author

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

@serge-kvashnin
Copy link
Author

Summary of changes:

  1. I introduced ksort because for definitions where specific parameters are defined and not ordered, the array might look like [1 => 'bar', 0 => 'foo']. This ensures array_slice will properly fetch the rest of the variadic parameters.
  2. I also made isVariadic check the first priority check so that it works properly when variadic parameters are not passed.
  3. I also enhanced the ReflectionBasedAutowiring::autowire() method with iteration through defined method injections. This involves completing method injections with parameters taken from reflection but not defined explicitly (was already done for constructor). It has nothing to do with variadic parameters, but I spotted this issue when I was defining only parameter b while skipping a (which was type-hinted).

@joachim-n
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants