Bug #97149
closed
GeneralUtility::makeinstance() container fetch should be allowed with or without arguments.
0%
Description
Currently if a service has arguments, fetching it from the container is disallowed even though in nearly all cases the container already holds an instance of the service or is ready to create one complete with autowiring.
The current approach is inefficient since one loses all autowiring benefits ...and kinda nuts because every service created using GeneralUtility::makeinstance() with arguments never benefits from DI.
So, instead of having this line in the method with the arguments prohibition in GeneralUtility::makeinstance():
if(self::$container!==null && $constructorArguments===[] && self::$container->has($className)){
return self::$container->get($className);
}
let's have this following instead so we can squeeze all the juice out of having Symphony's DI:
if(self::$container!==null && self::$container->has($className)){
$instance=self::$container->get($className);
if($constructorArguments){
return $instance(...$constructorArguments);
}
return $instance;
}
I've autowired a custom route aspect mapper. It is called by GeneralUtility::makeinstance() from AspectFactory through the PageResolver middleware, and because it has settings argument (as it should) in its constructor, fetching it from the DI is skipped. The truth is, it doesn't have to be. I believe you'll agree that this change is all benefit and no pain.
Updated by
Updated by Benjamin Franzke about 2 years ago
- Status changed from New to Rejected
- Complexity changed from no-brainer to medium
Hey John,
did you try to test your approach?
The change you are suggesting will not work as is and it would have been a breaking change which is why we added $constructorArguments===[].
The suggest will create an instance via the container and then call that instance as callable with the given arguments.
If the service isn't callable (doesn't have an __invoke method), it would even result in a fatal error.
So passing arguments like that would create an entirely new GeneralUtility semantic, as it would fetch-from-container and call __invoke on services.
We don't want to add "more" stuff to GeneralUtility::makeInstance but rather reduce it's usage.
Here is an example what kind of service your suggestion would require:
class MyAspect
{
public function __construct(protected MyDependency $myDependency)
{
}
public function __invoke($anArgument)
{
// $constructArguments would be passed to this function with the example code given in the description
}
}
Note that we do not allow arguments to the constructor, as symfony dependency injection doesn't support that.
All constructor arguments must be wired inside the container, not external arguments are supported by symfony DI.
There would have only been "one option":
Ignore parameters if the service is provided by the container, but I opted to not do that as ignoring invalid code is never a good option,
and there were cases where services where Singletons where initialized with explicit parameters and for that reasons we needed to skip direct DI usage.
I've autowired a custom route aspect mapper. It is called by GeneralUtility::makeinstance() from AspectFactory through the PageResolver middleware, and because it has settings argument (as it should) in its constructor, fetching it from the DI is skipped. The truth is, it doesn't have to be. I believe you'll agree that this change is all benefit and no pain.
I understand the pain, but what we need to change is AspectFactory to allow classes to be fetched from the container by adding a SettingsAwareAspectInterface where settings are added by a setter method.
Let's do that in https://forge.typo3.org/issues/97150
For now your aspect needs to makeInstance it's dependencies (consider using a separate public service that you use to inject other dependencies).
Thanks, Benjamin
Updated by Benjamin Franzke about 2 years ago
- Related to Bug #91750: Depencency Injection not working for xclass controllers added
Updated by Benjamin Franzke about 2 years ago
- Related to deleted (Bug #91750: Depencency Injection not working for xclass controllers)
Updated by Benjamin Franzke about 2 years ago
- Related to Feature #97150: Support dependency injection for routing aspects added