-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] [RFC] Plain array configs #11953
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
Conversation
AFAIU, this PR is about two unrelated things: support for closure and a new PHP format. I think it would help to separate the two. |
I agree that it should be split into independant PRs. It will be easier to discuss and review (and will give us the possibility to vote for the features separately) |
@@ -701,6 +717,16 @@ private function addNewInstance($id, Definition $definition, $return, $instantia | |||
} | |||
|
|||
if (null !== $definition->getFactoryMethod()) { | |||
if ($definition->getFactoryMethod() instanceof \Closure) { | |||
if ($this->closureDumper === null) { | |||
throw new RuntimeException('DIC PhpDumper requires ClosureParser in order to dump closures'); |
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.
requires a ClosureDumperInterface implementation set in order to dump closures
👍 for both features. |
given that you have split the closure support in a separate PR, the correspondign code should be removed from this branch |
It would be better to create new issue with discussion about array config format, this PR now looks messy as it is tighly coupled with closures conception. |
In this PR I want to discuss the following config format:
There are some reasons to use plain arrays instead of YAML/XML in some cases:
The main disadvantage of plain array against YAML/XML is lack of sandboxing. It can lead abusing of code in configs. But on other hand pure PHP config format has the same problem.
Unfortunately, I see some problems in order to create real PR:
PhpFileLoader
already holds *.php extension, so order of loaders matters.What do you think?