search

laminas / laminas-config-aggregator

Lightweight library for collecting and merging configuration from different sources
https://docs.laminas.dev/laminas-config-aggregator/
BSD 3-Clause "New" or "Revised" License
23 stars 14 forks source link

[RFC]: Allow a ConfigProvider to require ConfigProviders from dependencies #25

Open internalsystemerror opened 2 years ago

internalsystemerror commented 2 years ago

RFC

Q A
Proposed Version(s) 1.10.0
BC Break? No

Goal

laminas/laminas-modulemanager has a great feature that allows modules to be in a dependency tree. I would like a similar feature in this component.

There should be no changes to the current mechanisms, but dependent providers should be obtained and required in the order that they first appear, ahead of the ConfigProvider that requires them. (Edit: A clarification on this last point, and not my original intention but after a good suggestion by @boesing, the current list of providers in config.php should be considered as the first appearance if a provider is listed there).

Background

It is always necessary at the application level to include the ConfigProvider from all components used, even if there is no application code that specifically relies on those components (as they would be required and used internally by the external component). This can often lead to not knowing about providers that also need adding and there is no check to ensure that they are, nor any sufficient error message to explain to the user that this is the case.

Considerations

To use the new proposed mechanism should be an opt-in, and not require changes for those applications or components that do not wish to use it. This should be checked only the once and not affect the performance of retrieving from cache. Dependent providers should only be included once, and in the order that they first appear in the tree.

Since the changes would need to be made to provider loading, doing this as an external package would not be possible without also providing a modified ConfigAggregator.

Proposal(s)

I propose to introduce a new interface which requires a method providing a list of direct dependent ConfigProvider classes. This component would then check for this interface and prepend those required classes to the list of providers.

Appendix

Possible interface (names are just my first suggestions):

interface HasConfigProviderDependencies
{
    /**
     * @return iterable Array or \Iterator of provider dependencies. These are string values representing
     *     classes that act as providers. They must be instantiable without constructor arguments.
     */
    public function configProviderDependencies(): iterable;
}

Similar to/overlap with:

Ocramius commented 2 years ago

Does this just decide the order of merged configs?

internalsystemerror commented 2 years ago

That could be a result of this yes. But it would also reduce the amount of ConfigProviders you need to include at the application level (at least the ones where the ConfigProvider would be the only result from composer-require-checker for instance, and they were already included by a component you do interact with)

boesing commented 2 years ago

I would only allow class-string of config providers. The ConfigAggregator should only verify if the components are in the right order but I would not suggest any kind of reordering being done automatically.

Let upstream handle the order - we can throw an exception and announce problems but automagically reordering the config providers might introduce more problems than its helpful.

Besides the fact that I like the idea as it is somehow equal to the logic of laminas-modulemanager (which has module dependencies as well), I am not 100% sure if we should do more than just notifying a user in case of an invalid "order".

Thanks to the component-installer, there wont be such "missing entries in config.php" anymore and thus, half of the task of this interface are already obsolete. Yes, there are projects which do actually not use that component-installer, but if they do not want to use automation, I don't see why we should have redundant logic around here. But thats probably only my personal PoV and thus I'm open to see whats going on.

I am not 100% sure if I understand this part:

These may be callables [...]

What does that mean? Do you expect these callables to provide a configuration? So I could do something like:

final class LaminasCacheStorageAdapterRedisConfigProvider implements HasConfigProviderDependencies
{

    public function __invoke(): array
    {
        return [];
    }

    public function configProviderDependencies(): iterable
    {
        return [
            static fn () => [
                'whatever-config-value' => [
                    'existing-value' => 'override',
                ],
            ],
        ];
    }
}

Or do you expect the storage adapter to instantiate the config provider from laminas-cache?

Maybe a PoC PR might be helpful to better understand what the main goal is. But I am strongly against any auto-reorder or any kind of magic as it might even break upstream configs if we mess it up.

Imho the only TODO for this marker interface should be to tell what other config providers are required to be already loaded when the iterator reaches the provider implementing the interface.

internalsystemerror commented 2 years ago

The task of this would not be to simply check the order or even to reorder anything, but to allow components to have their own config dependencies.

As a user of component X, my interaction is only with component X. However, using the component installer, I would also have component Y added to my list of configurations. But I never touch component Y in my application code ever, that is all handled by X.

This RFC proposes a way to hide the details of Y from the application, so that the user only needs to include component X. Users of the component installer would not be affected, current implementations and configurations would also not be affected. This would be a purely "opt-in" feature of this package for use by libraries that hide implementation details.

I'm averse to magic (having followed @Ocramius for some time, that's a natural expectation), so it is not my intention to have an automagic feature. If your application code is using component X AND Y, then you would/could still include Y's ConfigProvider as you currently do.

What does that mean? Do you expect these callables to provide a configuration?

That was a copy and paste from the current ConfigAggregator, I'm unsure specifically how callables would be handled, I think just class-string is what I originally was thinking anyway.

Maybe a PoC PR might be helpful to better understand what the main goal is.

I think we're at that point now, most of the comments here are confirming that we should avoid the things I wanted to avoid anyway.

Imho the only TODO for this marker interface should be to tell what other config providers are required to be already loaded when the iterator reaches the provider implementing the interface.

That is exactly what this RFC is proposing, in the least just a check, and at most to include those dependent providers immediately prior to the one that includes the interface. Where the confusion around ordering comes up is because if 2 components include the same ConfigProvider depedency, it should only be included once the first time it appears and not the second time as it has already been included. The end result being as if they were all declared in the top level config.php.

boesing commented 2 years ago

As a user of component X, my interaction is only with component X. However, using the component installer, I would also have component Y added to my list of configurations. But I never touch component Y in my application code ever, that is all handled by X.

If you don't need component Y, why is it a hard dependency then? I don't understand the use-case. Do you have a real example where this would be useful so I can try to better get your point here?

If a component is part of the vendor, then its ConfigProvider should also be loaded as it provides the whole container wiring and stuff. I can't think of any scenario where we do need component X which depends on component Y (whyever) but not the ConfigProvider from component Y 🤔

internalsystemerror commented 2 years ago

It's a hard dependency because component X needs the config from Y to be loaded prior to loading X. So you end up with a hard dependency on Y, simply because X uses it.

What I'm proposing here is that component X can tell the aggregator itself that it needs component Y, meaning the user doesn't need to do it at the application level. I'm essentially doing this all over the place hiding Doctrine/Mezzio/other framework specific stuff behind implementation components, and our applications then have a mix of the various components. This would not only reduce the maintenance burden for applications but would make testing components significantly easier too as you would only need to require that component's ConfigProvider.

boesing commented 2 years ago

Then you need to take care of ordering, which will be magic. imho, the only Job of such a marker Interface is to tell application what dependend component(s) should be loaded.

No magic "I provide you with the config of that component as well".

The component installed adds and removes config Providers so at least I dont get the "maintenance burden" part.

The only thing what wont be ensured by the installer is to verify the ordering. but again, that should not be handled by any automation.

What if multiple components base on the same component - lets say cache Adapter memory and filesystem. who is providing the config? both? but what if both override the same Interface in the servicemanager config to I.e. provide a default implementation of a specific Interface?

Imho, this sounds more complex than it needs to be.

My opinion here is:

  1. provide list of component configproviders which should be loaded before the component providing the marker Interface is being loaded
  2. use component installer to handle add/removal of config providers
  3. do not require concrete components if not used in upstream project

but again - Maybe I dont get the point. A PoC of a project with cache Adapter Filesystem and memory would give a better idea of what should be achieved.

internalsystemerror commented 2 years ago

No magic "I provide you with the config of that component as well".

I wouldn't consider a component requiring it's own config magic at all, but better handling of the responsibilities. If component X needs the config for Y, then it should be including the config for Y. Otherwise all documentation always needs to say, not only do you need to include the config for this component (X) but you also need to include the config for this other component you never use or touch or even know is there (Y).

The component installed adds and removes config Providers so at least I dont get the "maintenance burden" part.

Not everyone is using the installer. I don't think I ever have, as for me, that's the kind of magic I strictly try to avoid. Not everyone is using mezzio/lamins, but they do make PSR compatible libraries for instance.

If component X is already installed but adds a dependency on Y, is that covered by the installer (no idea btw, just thinking of ways the installer used to fail me)? That's the burden I talk of, the installer does it for you but you also need to know about it in cases where you need to manually intervene.

What if multiple components base on the same component

As stated, this is what I mean by "in the order they first appear". So if you include Adapter and then Filesystem, and they both rely on generic, then generic would be included by Adapter, and not AGAIN by Filesystem. If you reverse the order, then generic would be included by Filesystem and not AGAIN by Adapter, if that makes more sense.

Imho, this sounds more complex than it needs to be.

Agreed, this is actually really straight forward and not even that huge a feature. Allow a ConfigProvider to specify any other ConfigProviders that it depends upon and must be included prior. Whether this generates an error when invalid, or it moves the responsibility to the library, I can do either. I will still be using the latter going forward though as this has GREATLY simplified a large part of refactoring, and made setting up PHPUnit for new components really simple and straight forward, resusing existing code without requiring some custom implementation every time.

boesing commented 2 years ago

If component X needs the config for Y, then it should be including the config for Y.

And this is where we disagree.

[...] but you also need to include the config for this other component you never use or touch or even know is there (Y).

Why are you that sure about it? What if I do know and what if I want to modify the config, i.e. the dependencies config of a component? Since we do usually provide interfaces from a component, that can be implemented and overriden within the configuration.

There could be even a 3rd-party module requiring laminas-cache-storage-adapter-apcu which was never required by myself and now you start to override my configuration because your stuff might be added somewhere in the list of config-providers (below my own component) and then just replaces my config.

There are so many ways on how a project might interact with components. You will never be able to handle all cases and therefore, I would not even try that.

Otherwise all documentation always needs to say, not only do you need to include the config for this component (X) but you also need to include the config for this other component you never use or touch or even know is there (Y).

But thats how its actually working for years. mezzio requires the router and the router requires a router implementation which is provided by either fastroute, laminas-router, etc. It works perfectly fine and I've never heard complains. Whenever there were complaints, it was due to the fact that some1 had not installed the component-installer and thus missing the ConfigProvider of fastroute for example. With your suggestion here, this problem won't be fixed at all because some1 still has to add the fastroute ConfigProvider to the config.php (the component-installer always adds components to the top tho - which btw. might be problematic, especially when it comes to interface replacements from the base component, but thats another story here).

Not everyone is using the installer.

Exactly, and thats problematic. We should encourage people either to use it, or - if they do not want to use it because they are advanced devs - let them add the config providers which are required themselves in the order they want to add them.

The problems we are introducing with this feature you are suggesting may become bigger than the problem that a single entry in the config.php is missing.

[...] If component X is already installed but adds a dependency on Y, is that covered by the installer (no idea btw, just thinking of ways the installer used to fail me)? [...]

Yes, the installer supports the following features:

As stated, this is what I mean by "in the order they first appear". So if you include Adapter and then Filesystem, and they both rely on generic, then generic would be included by Adapter, and not AGAIN by Filesystem. [...]

But if both components do provide the config of laminas-cache (because laminas-cache would not have a ConfigProvider anymore), both might replace each other? Lets take this code as an example, this might be moved to every cache adapter as you say that no1 should need to know that there is a dependency to laminas-cache (which would be kinda weird, as one is usally either consuming StorageInterface or CacheInterface/CacheItemPoolInterface within the project). So now multiple ConfigProvider provide us with the same config:

https://github.com/laminas/laminas-cache/blob/90e75853f0eaf7272188e95bd80e6131f92063ec/src/ConfigProvider.php#L37-L61

Don't you think that this will lead to super weird problems?

[...] That's the burden I talk of, the installer does it for you but you also need to know about it in cases where you need to manually intervene.

No, because the installer only adds the ConfigProvider in case it is missing. If its available in your project, it does not mess-up your order anymore. Then I can simply add my ConfigProvider and override stuff from that component and ensure that it has the highest priority. I do not want to know all the details and nested factories a currently implemented service hidden behind an interface needs. This would totally prevent us from adding new stuff to laminas-cache like a new implementation of an interface. That would require all components which do rely on that component to provide the new implementation via their ConfigProvider to switch to my new class.

Again, I am +1 to add the same logic as how the laminas-modulemanager works but I am strictly against all additional fancy stuff, especially the fact that we remove ConfigProviders from components which are usually "implemented" by i.e. mezzio-fastroute or lamians-cache-storage-adapter*. This will definitely become a nightmare and I don't see why we should change the way of how configs work since mezzio/expressive v1.

internalsystemerror commented 2 years ago

And this is where we disagree.

Fair, though I would go back on "should" and change that to "could". Should in order to complete the scenario I was proposing, but could for general users of the aggregator.

Why are you that sure about it? What if I do know and what if I want to modify the config, i.e. the dependencies config of a component? Since we do usually provide interfaces from a component, that can be implemented and overriden within the configuration.

Because this an opt-in proposal, and as stated existing users should not be affected by it. If you are already including a ConfigProvider, then it won't be added again because it first appears in your list (I'll update the issue to reflect this is what it should be).

The problems we are introducing with this feature you are suggesting may become bigger than the problem that a single entry in the config.php is missing.

I think this is where you're misunderstanding me, I'm not suggesting that this should fix missing entries, I'm proposing this an opt-in feature that users or libraries could use.

Yes, the installer supports the following features

Some of those were either broken or unimplemented then when I last used it. That makes me happier to go with showing a warning/error instead of just including them for you. Though I would probably still use my own version to gain the benefits of quickly swapping implementations as well as making testing easier to maintain.

Lets take this code as an example, this might be moved to every cache adapter as you say that no1 should need to know that there is a dependency to laminas-cache (which would be kinda weird, as one is usally either consuming StorageInterface or CacheInterface/CacheItemPoolInterface within the project).

Not at all, if it doesn't make sense for laminas-cache to use it, then why would you. This would be an opt-in feature that changes nothing about the current usage. And the example you give is outside the use case of this, as this would be for instances where you are not interacting with the other component at all. As in, if you were to run composer-require-checker on your app, the only place the dependency would show up would be in config.php and no where else.

No, because the installer only adds the ConfigProvider in case it is missing. If its available in your project, it does not mess-up your order anymore.

Not the case as you already stated since this is always appended to the end of your list of config providers, so you always have to move it to the right spot anyway. The user should be aware of what is being done on their behalf as this isn't a config cache, its your source files that are being modified.

but I am strictly against all additional fancy stuff

Likewise, I'm not proposing any changes to the current workings, but simply to provide an extra feature that allows a ConfigProvider to require other ConfigProviders from that libs own dependencies. This may not make sense within laminas or mezzio, and my target audience for this would be users who separate the application logic from their own reusable components based on laminas/mezzio or otherwise (i.e. me :D).

internalsystemerror commented 2 years ago

I'll come up with a demonstrative PR to hopefully clear up what exactly my intentions are.

boesing commented 2 years ago

Not the case as you already stated since this is always appended to the end of your list of config providers, so you always have to move it to the right spot anyway. The user should be aware of what is being done on their behalf as this isn't a config cache, its your source files that are being modified.

To the top. And since composer SAT ensures the packages are installed in the correct order (i.e. laminas-cache before laminas-cache-adapter-* due to it is required by the adapter components), thats correct.

Some of those were either broken or unimplemented then when I last used it.

That was introduced in November 2021, but yes, it had to be introduced in a later version.

internalsystemerror commented 2 years ago

@boesing Although I believe my intentions with this do not impact current usage at all, you are correct that there is more to this that I should be considering, and I thank you for your comments.

Considering all that you've said, we may be able to benefit from this within laminas, and still ensure that this is opt-in only by implementing this as a ConfigPreProcessor which only receives the list of providers and returns it modified. The default list of preprocessors would be empty. That way, laminas components could use the interface, and it would still be up to the end user to opt-in by specifying that they wish to use the preprocessor.

internalsystemerror commented 1 year ago

Coming back to this. I'm not sure what I had in mind is really possible. For example, if laminas/laminas-httphandlerrunner was able to say to this package that it requires the ConfigProvider from laminas/laminas-diactoros, an interface would then create a dependency from laminas/laminas-httphandlerrunner to this package, which I'm sure everyone agrees we would not want.

The ability to do this within my own packages has made bootstrapping unit tests much easier as I only need to import that components ConfigProvider. It also means that if I add a dependency, A, as a dependency of B, I can have A's config added without having to create a dependency on A and then adding its ConfigProvider, in every app where B is included.

Unfortunately, I don't see a good way that I can share this in a way that laminas/mezzio can take advantage of. Only users that already have a dependency on this package laminas/laminas-config-aggregator.

So without any further suggestions, my current opinion is that the config dependency preprocessor I had in mind should become its own standalone package similar to laminas/laminas-config-aggregator-parameters, and we would not be able to take advantage of that in laminas/mezzio packages without creating a dependency on it (and by extension, this).

Xerkus commented 1 year ago

I do not think this should be in config aggregator tbh - don't make simple thing unnecessarily complicated.

We have feature in module manager where modules can declare dependencies on other modules and make them load even when not declared in modules list. It was never used in our modules or Zfc modules or any other modules I have seen used. I do not remember exactly why after so many years, but it proved problematic and generally not worth the hassle. All of that even before component installer composer plugin was even a thing.

May be consider ability to declare ordering requirements with component installer instead?

internalsystemerror commented 1 year ago

This would be an attempt to gain the benefit from that feature of the module manager, without all the other headache that comes with the rest of it. The preprocessor itself isn't even that large or complex.

We're in agreement that it shouldn't be a part of this package, but what about my suggestion to make it stand alone as (e.g.) laminas/laminas-config-aggregator-dependencies?

Now that this package at least allows preprocessors, I could/should (probably will) release it under my own name instead. That way at least its out there if people do want to use it.

Xerkus commented 1 year ago

That module manager feature for modules declaring dependencies is specifically not used because it transparently loads those modules without declaration. It was worse than modules not getting registered as no one can clearly tell what is actually being loaded and what overrides what because it is obscured by some internal workings. Another pain point I remember was already covered by @boesing above, the one pertaining to intentionally not loading module and providing own alternate configuration.

Even with module manager you can't say for certain if required module was loaded because there is no definite unique identifier attached to module, with config aggregator it is worse because config provider is something, anything, callable that returns array.

It does not matter if this functionality will be hidden by a separate package or in config provider itself - the effect is the same.

I see too many parallels here and I really expect the repetition of giving "Don't use module dependencies. We recommend to always register modules explicitly." to those looking for support after a year or two.