Skip to content

[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

Closed

Conversation

unkind
Copy link
Contributor

@unkind unkind commented Sep 18, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT
Doc PR n/a

In this PR I want to discuss the following config format:

<?php

use Symfony\Component\DependencyInjection\ContainerInterface;

return [
    'parameters' => [
        'foo' => 42,
        'bar' => 'bar'
    ],

    'services' => [
        // à la Pimple, by default
        'foo' => [
            'class' => FooBarBaz::class,
            'factory_method' => function (ContainerInterface $container) {
                return new FooBarBaz($container->getParameter('foo'));
            }
        ],

        // with arguments
        'bar' => [
            'class' => FooBarBaz::class,
            'factory_method' => function ($bar) {
                return new FooBarBaz($bar);
            },
            'arguments' => ['%bar%']
        ]
    ]
];

There are some reasons to use plain arrays instead of YAML/XML in some cases:

  1. Short array syntax was introduced years ago and it minimises difference between YAML and plain array in readability.
  2. Class name resolution allows IDE to find class usages, apply renaming, go to class by click, etc.
  3. Expression language is nice, but it looks awkward in YAML/XML configs (well, at least for me). My point is to use closures for inline logic instead.

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:

  1. My PR depends on Super Closure library for dumping closure's code, but I'm not sure if you accept it. By the way, only context free closures can be dumped, it's some sort of restriction.
  2. PhpFileLoader already holds *.php extension, so order of loaders matters.

What do you think?

@fabpot
Copy link
Member

fabpot commented Sep 19, 2014

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.

@stof
Copy link
Member

stof commented Sep 19, 2014

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');
Copy link
Contributor

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

@GromNaN
Copy link
Member

GromNaN commented Sep 19, 2014

👍 for both features.

@unkind unkind changed the title [DIC] [RFC] Plain array configs [DependencyInjection] [RFC] Plain array configs Sep 24, 2014
@stof
Copy link
Member

stof commented Sep 24, 2014

given that you have split the closure support in a separate PR, the correspondign code should be removed from this branch

@unkind
Copy link
Contributor Author

unkind commented Sep 24, 2014

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.

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

Successfully merging this pull request may close these issues.

5 participants