Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: ServiceManager configuration validation #131

Open
boesing opened this issue May 11, 2022 · 12 comments · May be fixed by #189
Open

Feature: ServiceManager configuration validation #131

boesing opened this issue May 11, 2022 · 12 comments · May be fixed by #189

Comments

@boesing
Copy link
Member

boesing commented May 11, 2022

Feature Request

Q A
New Feature yes

Summary

Whenever we are working in projects with own plugin managers (like laminas-validator, etc.), we do probably ignore the configuration structure which is fetched from the container.

https://github.com/laminas/laminas-validator/blob/bdd503adc83d814a5c94e598ea0eb9fc7ca56339/src/ValidatorPluginManagerFactory.php#L49


So there will be some point in time where we have to validate the plugin managers configuration. To avoid having to do this manually, I'd prefer fetching a service from the container which is able to verify that the configuration is valid.

Such method/interface like I've created in the laminas-cache component would suffice.

https://github.com/laminas/laminas-cache/blob/130cc8db636d97b8287ff817515bb1c9ae0ef099/src/Service/StorageAdapterFactoryInterface.php#L45


WDYT? Feedback very welcome.

@Ocramius
Copy link
Member

To avoid having to do this manually, I'd prefer fetching a service from the container which is able to verify that the configuration is valid.

Perhaps something like cuyz/valinor suffices? We already have most psalm types declared, AFAIK

@boesing
Copy link
Member Author

boesing commented May 11, 2022

Not sure how good valinor works with array shapes.
But yes, we do have psalm types around.

Do you have an example on how you would use valinor to instantiate a ServiceManager or a ValidatorPluginManager?

@Ocramius
Copy link
Member

Would probably be something like:

$mapper->map(ServiceManager::class, ['config' => $config]);

The array shape should be validated this way, although I don't know how deep it can validate closures.

@boesing
Copy link
Member Author

boesing commented May 16, 2022

Maybe passing over validation to the Config instance would work better.

So one has to use:

$pluginManagerConfigFromProjectConfiguration = $config['validators'];
Assert::isMap($pluginManagerConfigFromProjectConfiguration);
$pluginManagerConfig = new \Laminas\ServiceManager\Config($pluginManagerConfigFromProjectConfiguration);
$pluginManager = new ValidatorPluginManager();
$pluginManagerConfig->configure($pluginManager);

In case of the laminas-validator package and all the other packages (which are plenty) which do provide a plugin manager, having to use valinor to verify that the config is valid would be a no-go for me tho.

@Ocramius
Copy link
Member

We don't really have type-checking packages inside laminas: our validators are designed for checking HTTP input data (and similar), not in-memory object structures.

That's really the realm of more type-friendly tools (like in this case, valinor just being better at it).

I don't mind using other packages in general, but:

  1. no, please no more plugin managers: they hurt ;_;
  2. let's avoid adding dependency cycles wherever possible

A big if/else block of conditionals is better value than using laminas/laminas-validator in this scope.

@boesing
Copy link
Member Author

boesing commented May 16, 2022

Uhm, not sure if we are talking about the same.

  • I do not want to add more plugin managers
  • I do not want to use laminas-validator to validate a config

I just want to provide a proper flow for reading a config which can be extended by upstream projects (by Design) without the need of external tools. We do have 10+ existing plugin managers which will - at some time - use the latest servicemanager version and thus the typed array shape for the constructor.
At this point, psalm will bitch in the factory of that plugin manager, that the constructor expects an array with optional keys rather than just a map.

What I do want to achieve is that we do not have to use psalm-suppress at that LoC but instead some "safe" way to pass the plugin manager config from the application config to the plugin managers constructor.

@Ocramius
Copy link
Member

Do you have a more practical example of a required @psalm-suppress? I thought this was about runtime validation, rather than compile-time validation :|

@boesing
Copy link
Member Author

boesing commented May 17, 2022

The example would be in the initial post. 1st link to an existing factory within laminas components.

Reading a config for the plugin manager from the application config and passing it to the constructor.

This actually only works because either there is no psalm in the validator component or the locked servicemanager version is below that version providing the array shape (or the error is already baselined).

@Ocramius
Copy link
Member

Hmm, adding that at runtime to general use seems wasteful 🤔

@boesing
Copy link
Member Author

boesing commented May 17, 2022

Sure but how are we supposed to tell psalm that everything is correct then?

Because - the annotation is set. And thus something has to assert stuff. 🤷🏼‍♂️

@Ocramius
Copy link
Member

Ocramius commented May 17, 2022

Sure but how are we supposed to tell psalm that everything is correct then?

This is one of those cases where a leap of faith is required: adding runtime validation of a complex nested data structure has a huge performance impact (which I've already experienced with cuyz/valinor and azjezz/psl), so something like this should be opt-in.

I did have to do similar kind of "cuts" when dealing with ElasticSearch documents: the performance impact of checking the whole structure was having real impact on CPU usage.

Because - the annotation is set. And thus something has to assert stuff. 🤷🏼‍♂️

Usually /** @var */, as ugly as it is, is a pragmatic solution for these cases.

EDIT: here's an example from a project where I needed to do this sort of corner-cutting:

    /**
     * @param bool $validatePayload payload validation, at the time of this writing, is a really
     *                              CPU-intensive operation, since a lot of recursion happens
     *                              within type assertions. At the risk of production crashes,
     *                              we therefore conditionally enable it with this flag. It is
     *                              endorsed to use {@see true} when running integration tests,
     *                              as well as for local development.
     */
    public static function fromRawArray(array $raw, bool $validatePayload): self
    {
        if ($validatePayload) {
            $data = Type\shape([
                'hits' => Type\shape([
                    'total' => Type\shape(['value' => Type\union(Type\positive_int(), Type\literal_scalar(0))], true),
                    'hits' => Type\vec(
                        Type\shape([
                            '_score' => Type\float(),
                            '_source' => Product::rawArrayType(),
                        ], true)
                    ),
                ], true),
            ], true)->coerce($raw);

            return new self(
                array_map([Product::class, 'fromArray'], $data['hits']['hits']),
                $data['hits']['total']['value'],
            );
        }

@boesing
Copy link
Member Author

boesing commented May 17, 2022

Mhm. Maybe having some kind of runtime validation for non-production environments might work?
So providing some kind of validation which can be disabled in prod which then just early returns but still provides psalm-assert?

Maybe disabled by default (to reflect current behavior) and enable in development to have runtime validation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants