-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] More generic loaders #21062
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
[DI] More generic loaders #21062
Conversation
Practically ready. Tests for |
👎 in the same spirit as my other votes today: help everyone save time on discussion+coding when the use case so thin that we can bike-shade about it. |
Regarding the DI loaders; it just simplifies things (imo.). For SF, for me, for everybody. Anyway, if it's not the better approach or so.. or something is wrong the diff let me know. I dont know about other use cases, mine showed this works |
@ro0NL this still doesn't answer the "use case" question ("simplifies things" is not a use case, you+we need an incentive for your PRs to be merged). You talk about "other use cases", but you still did not describe yours (standardizing/simplifying/moving/refactoring is not a use case - a use case is something that triggers a "wow, with this, I could do that" to a least a few significant people). |
I'm aiming for an as low-level usage of the framework backbones as possible. With little deps and by default best DX/feature set as possible. If i have #19596 (maybe not a true core feature) + DI array loader i'd be super happy :) and im willing to work on it for that matter but ideally not in userland of course. |
@nicolas-grekas i feel the need to clarify my intentions, a a few PR's of mine basically relate to the same thing; but i overdid it / went too fast. Goal is: DI\ArrayLoader $container->setDefinition('id', new Definition());
new Reference()
new Alias() // etc. etc.
// vs.
return [
// ...
'some' => '@ref',
];
// SomeFileLoader
$arrayLoader->setCurrentResource($this->currentResource); // we're basically preserving any cosmetic errors here
return $arrayLoader->load(include (array) $phpFile);
Now, to move on, imo. it's worth it (DI\ArrayLoader) and i have few days this week to work on it. The bigger picture is to at least let YAML make use of it and perhaps tweak the PHP one in order to allow array returning. Im not sure about JSON and we can always do XML later on, or decide not to. I truly think it's doable and the better approach. And of course happy new year already 🥂 |
Speaking about wow effects... but, wow :) So given #11954, #11953 and now me... does that qualify (not saying im significant here :))
And for
Im not sure we should qualify the implementation as strictly being YAML conventions or so. There's nothing YAML specific to some nested key value pairs, neither is |
This terminology makes sense in your abstract model of DI configuration. But the question is does this abstraction make sense? I understand that if you want to have YAML-like PHP array configs, you'd duplicate most of the YAML loader. However, I don't see a problem, I think this kind of duplication is OK, you can assume that you just mimic YAML conventions ( As for me, I don't like idea of |
Good question. I guess super technically spoken yes, but that's truly my opinion. And actually your comment indeed gave good insight about other perspective. Thanks. I (personally) would favor the cost of abstraction here, and be just more flexible here for whatever format a user is trying to implement. Including core ones. Back to
It's the true question. We dont really need strict "model of DI config", it' just that having a generic |
$ret = $loader->load($resource, $type); | ||
} finally { | ||
unset(self::$loading[$resource]); | ||
$this->setCurrentResource($currentResource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong, as it does not reset the object in the previous state (as $currentResource
is not set directly in the property here, and may not come from the property either).
Adding 1 more state property in the loader is a bad idea IMO, as state in service objects makes them harder to use.
I'm -1 on this. I don't see the benefit of this, and it makes the code more complex by adding more state in the loader. |
👎 as well. On a side note, I don't think that adding other formats make sense (at least not in core). Second side note: we don't want to uniform the different formats, quite the contrary, we want to make sure we are using the full expressiveness of each format. Closing as 3 core team members voted -1. |
👍 i get the perspective. |
Config\Loader::import()
withImportingLoaderInterface
(new interface)Config\FileLoader::setCurrentResource
oversetCurrentDir
DI\FileLoader::setCurrentResource
fixing nested resource tracking out-of-the box.If we deprecate
setCurrentDir
the upgrade path would be;