Add support for autowiring of services; apply to OAuth2 code

[EreMaijala]

Adds support for autowiring services with dependencies that may be other services or VuFind configurations.

• Autowiring is opt-in; there must be an #[Autowire] attribute on the constructor.
• Configurations must be in array format and the attribute must indicate the configuration to inject.
• Service name is by default resolved from parameter type, but can be indicated with an additional `#[Autowire(service: 'name')] attribute.

Example constructor:

    /**
     * Constructor
     *
     * @param array       $mainConfig   Main VuFind configuration
     * @param array       $exportConfig Export-specific configuration
     * @param PhpRenderer $viewRenderer View renderer
     */
    #[Autowire]
    public function __construct(
        #[Autowire(config: 'config')]
        protected array $mainConfig,
        #[Autowire(config: 'export')]
        protected array $exportConfig,
        protected PhpRenderer $viewRenderer
    ) {
    }

TODO:

• Decide if this is a useful thing to have.
• Add support for autowiring different plugin classes (ILS drivers, database services etc.).
• Document autowiring in wiki.

[EreMaijala]

On my system with PHP 8.3.28 it takes about 2-4 microseconds to get the reflection data for the constructor. Trying to cache it would be slower. It seems that the only way this could be made faster would be an additional compilation step that would build service-specific factories ahead of time. The time to actually get the two configurations and the renderer in the example is typically around 350-600 microseconds (with extremes between 253 and 1665), so it seems that the reflection part adds very little.

It'd also be an option to use the reflection data for a utility that builds or updates an actual factory upon request. It would still require service manager configuration, though.

[demiankatz]

Thanks, @EreMaijala, this is a great start! A few initial thoughts:

1.) I wonder if it would make sense to split this into two parts: an AbstractAutowiringFactory that checks for classes that are eligible for autowiring, and a non-abstract AutowiringFactory that builds an object using the annotations. This way, we could explicitly configure autowiring without having to rely on the magic of the abstract factory if we want to start using the functionality in the short term, and in the longer term, the generic abstract factory I revised in #4801 could just add autowiring support to its broader bag of tricks without having to run multiple abstract factories on every service creation.

2.) It might also make sense to soften the requirement that the class have the Autowire attribute -- we could use the attribute to detect classes that are eligible for autowiring, but we could still allow explicit autowiring of classes that don't contain the attribute.

3.) Regarding support for plugins, I think the easiest solution is to add a 'container' option to the autowire attribute -- then we can specify which plugin manager to pull the plugin from.

4.) It might be interesting to look at the Laminas ConstructorParameterResolver for inspiration (this class isn't available until ServiceManager 4, but maybe we can use it directly in the future). At very least, I think its handling of default values for parameters might be worth incorporating into our version.

[EreMaijala]

@demiankatz Points 1 to 3 make sense to me. Regarding default values, I intentionally excluded default value handling. We have often used default values for new constructor params when we've wanted to maintain back-compatibility (with the expense of missing some functionality if they're not provided), and though they're probably now mostly properly typed, I didn't want us to automatically skip something that might still be untyped. At this point I believe it's better to require an explicit factory that could e.g. use the provided options to override default values. If we have cases where supporting the defaults would make sense, we could maybe require an attribute for it. This said, I'm ready to hear compelling arguments against these thoughts, of course. :)

[demiankatz]

@demiankatz Points 1 to 3 make sense to me. Regarding default values, I intentionally excluded default value handling. We have often used default values for new constructor params when we've wanted to maintain back-compatibility (with the expense of missing some functionality if they're not provided), and though they're probably now mostly properly typed, I didn't want us to automatically skip something that might still be untyped. At this point I believe it's better to require an explicit factory that could e.g. use the provided options to override default values. If we have cases where supporting the defaults would make sense, we could maybe require an attribute for it. This said, I'm ready to hear compelling arguments against these thoughts, of course. :)

Yes, that makes sense -- we can leave default values alone until there's a use case! (Though maybe there's an argument for using default values if they are scalar types).

[demiankatz]

Thanks, @EreMaijala, see below for thoughts on the latest changes.

Maybe it's time to start building some tests to cover the edge cases.

[EreMaijala]

@demiankatz Yes, tests are next on the list.

[demiankatz]

Thanks, @EreMaijala! Looking much better; see below for a couple more minor things.

I've also been thinking about how to actually introduce this to the codebase. I wonder if one approach might be to start replacing the odd exceptional top-level service factories that couldn't be automatically replaced by #4801. If we can switch some of those to the autowiring factory instead, we can reduce the number of unconventional factories and also increase the benefit of the proposed abstract factory. (I haven't looked to see whether this is actually feasible in any/all cases, but maybe we could at least pick one or two of those as proof-of-concept cases).

[demiankatz]

Tests look good! Just one question below related to our other open discussion thread.

[demiankatz]

Thanks, @EreMaijala, this all looks good to me now. Do you want to merge it now so it's available, or should we figure out a way to actually apply it before we merge? (I lean toward finding an application before merging, but I can be persuaded to go the other route if you think it would be more helpful to take that approach).

[EreMaijala]

@demiankatz Well, that's a good question. I know I'd probably be happy to use when adding anything new, but I'm fine waiting until that moment is here.

[demiankatz]

@EreMaijala, I took a look at my idea of using autowiring to reduce remaining special cases in #4801. There are a number of factories that can't be easily replaced with autowiring, but the OAuth2 code looks like a promising place to start. See commit e913eaa for a quick proof-of-concept (I've confirmed that tests pass after the refactoring, and break if I use an inappropriate factory). If you like this, we could carry the work forward to deprecate/replace VuFind\OAuth2\Repository\TokenRepositoryFactory as well, and maybe that's enough of a start to justify merging this. What do you think?

[EreMaijala]

@demiankatz That looks good to me!

[EreMaijala]

@demiankatz Also if you encounter something that could be made easier with a smarter autowiring factory, please add a note.

[demiankatz]

Thanks, @EreMaijala! I have set up autowiring to replace the other "unconventional" OAuth2 factory in commit ff25074. I think this is probably a good enough excuse to merge the PR, but I'll leave it to you to push the button in case you have any last-minute concerns.

So far, this seems to cover all of the obvious use cases I can think of... but we'll see if we run into anything weirder!

[EreMaijala]

@demiankatz Looks good otherwise, but I started thinking that using YamlReader as container and config as service is really unintuitive. How about making it possible to use the config setting also for yaml? It could first try to get it from configManager, but if the result is empty, it could try to find a yaml instead. Then it would be ready for the day when ConfigManager also handles yaml.

[demiankatz]

@demiankatz Looks good otherwise, but I started thinking that using YamlReader as container and config as service is really unintuitive. How about making it possible to use the config setting also for yaml? It could first try to get it from configManager, but if the result is empty, it could try to find a yaml instead. Then it would be ready for the day when ConfigManager also handles yaml.

I agree that might be more clear, but I hesitate to add a lot of fallback logic, both for performance reasons and because that can be unintuitive in a different way. How about this for a compromise:

Add a configType setting that can be default or yaml. When set to yaml, it appends .yaml on the config name and uses the YamlReader instead of the ConfigManager. This way, we can explicitly configure things, and in the future, if the YamlReader merges into the ConfigManager, no change will need to be made except ignoring the configType setting, which can be removed at that time.

[EreMaijala]

@demiankatz That's a good idea. I'll add it.

[EreMaijala]

@demiankatz Done, including a test case.

[demiankatz]

Thanks again, @EreMaijala -- I finally circled back around to this and had one last thought.

Also, do you plan to add the wiki page proposed in the TODO list, or do you want my help with that? I'm happy to help (with drafting, reviewing, proposing a location in the structure, or anything else you might need).

[demiankatz]

Thanks, @EreMaijala, I think this covers all the major use cases I can think of now!