laminas / laminas-servicemanager

Factory-Driven Dependency Injection Container
https://docs.laminas.dev/laminas-servicemanager/
BSD 3-Clause "New" or "Revised" License
152 stars 59 forks source link

Introduce `ConfigValidatorInterface` for minimum runtime validation of SM configurations #189

Open boesing opened 1 year ago

boesing commented 1 year ago
Q A
Documentation yes
Bugfix no
BC Break no
New Feature yes

Description

Starting with v4.0.0, the ServiceManager itself will not validate anymore as it provides psalm array-shape type for configuration values.

As per existing projects, the ServiceManager is usually configured somehow like these examples:

$smConfig = $configuration['service_manager'] ?? [];
$smConfig = new ServiceManagerConfig($smConfig);

$serviceManager = new ServiceManager();
$smConfig->configureServiceManager($serviceManager);
$config = require __DIR__ . '/config.php';

$dependencies                       = $config['dependencies'];
$dependencies['services']['config'] = $config;

$serviceManager = new ServiceManager($dependencies);

The first example does use deprecated Config instance which just verified that pre-defined array keys are provided. The second example does not verify anything and assumes the config to be correct.

Both cases do either need a psalm-baseline entry or a psalm-suppress once migrated to SM v4.0.0 (at least the second example already needs that as it uses the ServiceManager#__construct which is already properly typed.

Closes #131


So every pluginmanager providing component (such as laminas-cache, laminas-form, etc. - I've listed some more in the TSC meeting) would also need either a baseline entry or suppression due to the fact that there is no current way to verify that configuration matches expectations.

To avoid that, we could in fact use a runtime configuration validator which is provided by this PR.


I have not yet a clue on how to effectively use it, as I would actually prefer to only validate prior configuration caching so that not every request has to validate the same valid/invalid config over and over again. I've created this PR as a draft so that we can find a way of how to effectively use such a validator. In mezzio projects, having this as some kind of PostProcessor for config-aggregator would work but that won't work for i.e. laminas-form

Would love to get some help here.

gsteel commented 1 year ago

Could we wire up a service in the config provider here so that components that ship plugin managers can use that service to perform runtime validation in the plugin manager factory?

Say a service something like this:


namespace Laminas\ServiceManager;

/** @psalm-import-type ServiceManagerConfiguration from ServiceManager */
final readonly class ConfigValidationService
{
    public function __construct(private ValidatorInterface $validator, private bool $enabled)
    {
    }

    /** @psalm-assert ServiceManagerConfiguration $config */
    public function validate(array $config): void
    {
        if (! $this->enabled) {
            return;
        }

        $this->validator->assertValidConfig($config);
    }
}

… where $this->enabled is equivalent to $config['debug'] and in a plugin manager factory, something like:

final class PluginManagerFactory
{
    public function __invoke(ContainerInterface $container): ValidatorPluginManager
    {
        $config = $container->get('config');
        $services = $config['validators'] ?? [];

        $validator = $container->get(ValidatorInterface::class);
        $validator->validate($services);

        return new PluginManager($services);
    }
}
kynx commented 1 year ago

I have not yet a clue on how to effectively use it, as I would actually prefer to only validate prior configuration caching so that not every request has to validate the same valid/invalid config over and over again.

Yeah. I'd argue that validating configuration is a job for whatever build tool you use, not a runtime check.

As a developer, if I'm changing the config it's going to be obvious pretty quickly if an alias I've introduced does not resolve. If it's set to something different in production that's a problem. But does throwing an exception when the configuration is first loaded - as opposed to when the invalid dependency is first used - really make my life any easier? The borked config should never have got so far.

Maybe these could be made into reusable smoke tests instead?