silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

Support named arguments when instantiating records via injector #11048

Open GuySartorelli opened 8 months ago

GuySartorelli commented 8 months ago

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

kinglozzer commented 8 months ago

It’s also worth noting that we switched from Reflection to using new $class(...$values) for a performance boost: https://github.com/silverstripe/silverstripe-framework/pull/10265

GuySartorelli commented 7 months ago

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

GuySartorelli commented 5 months ago

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