[RFC]: Allow a ConfigProvider to require ConfigProviders from dependencies #25
Comments
internalsystemerror
changed the title
internalsystemerror
changed the title
|
Does this just decide the order of merged configs? |
|
That could be a result of this yes. But it would also reduce the amount of |
|
I would only allow 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 Thanks to the I am not 100% sure if I understand this part:
What does that mean? Do you expect these callables to provide a configuration? 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 Maybe a PoC PR might be helpful to better understand what the main goal is. 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. |
|
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.
That was a copy and paste from the current
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.
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 |
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 |
|
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. |
|
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:
but again - Maybe I dont get the point. |
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).
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.
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.
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. |
And this is where we disagree.
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.
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
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
Yes, the installer supports the following features:
But if both components do provide the config of Don't you think that this will lead to super weird problems?
No, because the installer only adds the Again, I am +1 to add the same logic as how the |
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.
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).
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.
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.
Not at all, if it doesn't make sense for
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.
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). |
|
I'll come up with a demonstrative PR to hopefully clear up what exactly my intentions are. |
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.
That was introduced in November 2021, but yes, it had to be introduced in a later version. |
|
@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 |
|
Coming back to this. I'm not sure what I had in mind is really possible. For example, if The ability to do this within my own packages has made bootstrapping unit tests much easier as I only need to import that components 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 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 |
|
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? |
|
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.) 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. |
|
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. 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. |
RFC
Goal
laminas/laminas-modulemanagerhas 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
ConfigProviderthat 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 inconfig.phpshould be considered as the first appearance if a provider is listed there).Background
It is always necessary at the application level to include the
ConfigProviderfrom 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
ConfigProviderclasses. 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):
Similar to/overlap with:
The text was updated successfully, but these errors were encountered: