nette / di

πŸ’Ž Flexible, compiled and full-featured Dependency Injection Container with perfectly usable autowiring and support for all new PHP 8 features.
https://doc.nette.org/dependency-injection
Other
869 stars 70 forks source link

Autoload-based automatic registration of service dependencies #282

Open jtojnar opened 2 years ago

jtojnar commented 2 years ago

If I want to have some class usable for auto-wiring, it needs to be registered in the container, otherwise auto-wiring will throw a MissingServiceException – I assume this is to prevent accidentally trying to create a random object in the container, which could hide that a class actually needs the object instantiated in a different way.

Currently, my options are either:

The latter is based on RobotLoader, which, in addition to repeating the work already done by Composer in most modern projects, can cause issues when some of the classes are not loadable.

For instance, barista is a Nette-based tool and it currently uses the search extension to locate services in the src/ directory. But the src/ directory also contains some helper classes, including one that depends on PHPUnit – even though the tool itself does not depend on PHPUnit so it might not be installed. When that is indeed the case, the search extension causes it to be loaded by calling class_exists, crashing the autoloader due to a missing PHPUnit class in the process.

Since this app is not actually needed in the first place and is only attempted to be registered due to it being in the src/ directory, I propose adding another extension for automatic registration of dependencies in addition to search that, instead of a directory, would look at the dependencies of explicitly registered services and try to register them, if they are not registered already. The interface could look something like the following:

autoloaderRegistration:
    # Registers any dependency required by services that is in the Barista namespace:
    namespace: 'Barista\\'
    excludes:
      - 'Barista\\Testing\\'

Would that be possible to implement? My main worry would be that the container also supports runtime auto-wiring of classes that are not registered. Dependency trees of those would not be available to the compiler. But that does not seem to be the case if I am looking at DI\Autowiring correctly. And even if it were, we could just have ignored those.

mabar commented 2 years ago

Solving your problem needs either ignoring files which use not autoloadable classes by configuration on your side or using static reflection and ignoring incomplete classes on Nette side.

Registering class as a service because some service relies on it is unreliable. It could work, but only with non-abstract classes. Even requiring interface instead of its implementation would break that magic. Context-specific dependencies like ORM entities would probably pass automatic registration, but who wants entities registered as services?

Btw, RobotLoader in SearchExtension is used just to find classes in a folder, not for actual autoloading. Problem is on php level - loading php files requires php to resolve references (e.g. extending phpunit test case), which triggers all autoloaders (composer in your case) and fails on missing class. That's why static reflection would be needed if solved on Nette side.

jtojnar commented 2 years ago

Solving your problem needs either ignoring files which use not autoloadable classes by configuration on your side

The search extension does not really allow ignoring files:

Registering class as a service because some service relies on it is unreliable. It could work, but only with non-abstract classes. Even requiring interface instead of its implementation would break that magic.

Good point, but I would expect most of abstract classes/interfaces will have more than one implementation and then the SearchExtension would fail just as well.

Context-specific dependencies like ORM entities would probably pass automatic registration, but who wants entities registered as services?

Right, but again I am interested in this specifically as an opposition to SearchExtension which will have the exact same downside if you pass src/ to it without any excludes.

Btw, RobotLoader in SearchExtension is used just to find classes in a folder, not for actual autoloading.

Yes, I am aware, which is why I specifically tried to talk about registration, not loading.

The extension I propose would just replace the in-advance file system search with looking at dependencies of explicitly registered services (transitively). And instead of file system path prefix, it would be limited by PHP namespace prefix.

Problem is on php level - loading php files requires php to resolve references (e.g. extending phpunit test case), which triggers all autoloaders (composer in your case) and fails on missing class. That's why static reflection would be needed if solved on Nette side.

I am not really sure if this is something Nette should be expected to solve. After all, having a Nette app and a library with optional dependencies in the same src/ directory is pretty unorthodox.

But either way, it is orthogonal to this proposal. Here I am mostly interested in getting rid of RobotLoader since I do not find it very useful.

Another benefit to this would be that it would make the container smaller than the equivalent use of SearchExtension, unless one is meticulous about excluding every class they do not need – but then you might as well register just the classes you need manually. Automatic registration of dependency classes in a certain namespace would only register actual dependencies.

dg commented 1 year ago

I will add to SearchExtension the option to exclude files as well, I thought it was there a long time ago.

The latter is based on RobotLoader, which, in addition to repeating the work already done by Composer in most modern projects, can cause issues when some of the classes are not loadable.

That's not true. Composer's role and capability is not to provide a list of all classes in an application.

jtojnar commented 1 year ago

Composer's role and capability is not to provide a list of all classes in an application.

Yes, but I would argue most apps will know the list of classes statically so getting a list of classes only matters in two cases (AFAICT):

dg commented 1 year ago

But unless there is only a single implementation, the mapping would still need to be resolved manually, thus the class name would be also known statically.

And how are you going to find out that there is only one implementation and what is its class name without a list of all the classes?