Ahead of time factory creation command

[boesing]

Q	A
Documentation	yes
BC Break	yes
New Feature	yes
RFC	yes

Description

Fixes #171

So just to quickly sum this up:

We do have project configuration, so this is not some fictional use-case.
This configuration contains service_manager configuration where some services (usually only depending on interfaces) are registered via ReflectionBasedAbstractFactory:

return [
    'service_manager' => [
        'factories' => [
            OurService::class => ReflectionBasedAbstractFactory::class,
        ],
    ],
];

Even tho, the ReflectionBasedAbstractFactory is an abstract factory, it also extends FactoryInterface and thus can be used as shown above.

• One of the "good" things is, that there is no need for a dedicated factory.
• The "bad" thing is, that the factory consumes runtime to use reflection and to determine the appropriate __construct argument values

I've played a little bit in our project and I ended up writing some code (which I now used to contribute here) which allows us to use ReflectionBasedAbstractFactory in development while providing a CLI command to generate a opcachable factory while putting it on the filesystem during release artifact generation.

Usage would be as follows:

Description:
  Creates factories which replace the runtime overhead for `ReflectionBasedAbstractFactory`.

Usage:
  servicemanager:generate-aot-factories [<localConfigFilename>]

Arguments:
  localConfigFilename        Should be a path targeting a filename which will be created so that the config autoloading will pick it up. Using a `.local.php` suffix should verify that the file is overriding existing configuration. [default: "config/autoload/generated-factories.local.php"]

One needs also to register a directory in the application config:

config/autoload/global.php

return [
    \Laminas\ServiceManager\ConfigProvider::CONFIGURATION_KEY_FACTORY_TARGET_PATH => 'path/to/config/autoloading/directory',
];

Since the path registered in that configuration key MUST also be added to composer.json autoload section using classmap, having this as a required configuration key to use the CLI command seemed to be a good solution.

{
     "autoload": {"classmap": ["path/to/factory/output"]}
}

So after configuring the application as mentioned above, the CLI command could be executed like:

$ php vendor/bin/laminas servicemanager:generate-aot-factories config/autoload/aot-factories.local.php

This will then scan the projects configuration for all container configurations (so everything which looks like a container configuration actually: has to be an array, containing key factories being an array, containing values with either class-string<ReflectionBasedAbstractFactory> or instance of ReflectionBasedAbstractFactory.

So in the end, we should have a bunch of generated factories located in the path/to/factory/output directory (there is a sub-directory per "container") PLUS the aot-factories.local.php file located in config/autoload.

In the end, running composer dump-autoload should "register" all the factories located in path/to/factory/output and the next vendor/bin/laminas servicemanager:generate-aot-factories run should tell us, that there are no (more) services registered via ReflectionBasedAbstractFactory (as the aot-factories.local.php should've overriden the initial application config).

🎉

[bcremer]

I like that proposed feature pretty much 👍

But I prefer to keep generated files in a dedicated directory (./data/generated).
I can archive that with your proposed feature by adding a new PhpFileProvider.

$generatedFactoriesConfig = [
    ServiceManagerConfigProvider::CONFIGURATION_KEY_FACTORY_TARGET_PATH => __DIR__ . '/../../data/generated/',
    'factory-override-path' => __DIR__ . '/../data/generated/generated-factories.php'
];

$aggregator = new ConfigAggregator([
    ServiceManagerConfigProvider::class,

    new PhpFileProvider(realpath(__DIR__) . '/autoload/{,*.}{global,local}.php'),

    // Add config for generated factories
    new ArrayProvider($generatedFactoriesConfig),

    // Add generated factories
    new PhpFileProvider($generatedFactoriesConfig['factory-override-path']),
]);

return $aggregator->getMergedConfig();

One feature I'd like to propose is adding localConfigFilename to the configuration so I don't have to provide it in the generate-aot-factories call.

php bin/console.php servicemanager:generate-aot-factories  data/generated/generated-factories.php

How about ServiceManagerConfigProvider::CONFIGURATION_KEY_FACTORY_LOCAL_CONFIG_FILENAME?

WDYT?

[boesing]

@bcremer thats exactly how it is proposed here. You have to configure ConfigProvider:: CONFIGURATION_KEY_FACTORY_TARGET_PATH to target to a path where the files have to be stored.
If that is missing, executing the command will output that the config key is missing.

You have to add it as your composer.json needs that directory in its autoload section.

If we are talking about the path which has to be added by arg1 via CLI - that won't make sense to make it configurable as it could differ from CLI. Not sure if I'd like to have that configurable as well, especially because if it is configurable, who do you expect that file to "add" to your config then? The whole config-merge has to be done via either ConfigAggregator or the moduleloader and frankly, having runtime logic which requires configs and merge them on-top of the already merged configs is nothing I'd like to support.

So imho, its up to the projects to specify a path and - in case the path is not automatically picked-up by configuration merge - make the changes to their ConfigAggregator/modulemanager implementations to ensure the file is properly loaded.
So IMHO, you can do what you provided above while synchronizing that path with your CLI execution as you still will have to provide that path via CLI argument to the command.

[bcremer]

I withdraw my idea of having a configuration key to configure the path for the override factory mapping (config/autoload/generated-factories.local.php).

For most users the the proposed default is the better choice as there is already the default PhpFileProvider in place to pick up that file.

I still support the proposed Pull Request from @boesing and would like to see it going forward.

[froschdesign]

The documentation is good, I will take over all further changes, because it would go beyond the scope here.