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

Support named arguments when instantiating records via injector #11048

Open
GuySartorelli opened this issue Nov 12, 2023 · 3 comments
Open

Support named arguments when instantiating records via injector #11048

GuySartorelli opened this issue Nov 12, 2023 · 3 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Nov 12, 2023

Currently passing in named arguments to be used in use of injector (i.e. MyClass::create('normal-arg', namedArg: 'value');) will result in silently passing the named argument to the wrong parameter. This is done intentionally because we use the splat operator to unpack arguments to be passed to constructors, and that operator throws an error for any string keys - so we have to pass through only the values (see InjectionCreator::create()).

We could change this to support named arguments in injection like so:

-// Ensure there are no string keys as they cannot be unpacked with the `...` operator
-$values = array_values($params);
-
-return new $class(...$values);
+return (new \ReflectionClass($class))->newInstanceArgs($params);

UPDATE: Apparently as of PHP 8.1 using the splat operator to pass named arguments is supported! See example 19 in https://www.php.net/manual/en/functions.arguments.php#example-505
So we don't need to use reflection at all.

Caveats

  • We name a lot of contructor args in injection yaml configuration - but those names don't always match up with real constructor argument names. We would need to review all of these and ensure that only real constructor argument names are used.
    • This is what makes it a breaking change.
  • InjectionCreator is effectively a factory (it's invoked in Injector::instantiate() as $factory->create($class, $constructorParams)) - this means the above solution would only work when a different specific factory has not been defined. There may therefore still be scenarios where named arguments are not respected.
    • tl;dr: In some of those scenarios it may result in silent failures where the argument is passed to the wrong parameter.
  • We swapped away from using injection for dependency injection for a performance boost - see ENH Faster method for creating injected instances #10265
@kinglozzer
Copy link
Member

It’s also worth noting that we switched from Reflection to using new $class(...$values) for a performance boost: #10265

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Nov 13, 2023

I'll add that to the caveats - that may be reason enough not to do this, but still worth discussing I reckon.

@GuySartorelli
Copy link
Member Author

UPDATE: Apparently as of PHP 8.1 using the splat operator to pass named arguments is supported! See example 19 in https://www.php.net/manual/en/functions.arguments.php#example-505

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

No branches or pull requests

2 participants