[DI] Add "PHP fluent format" for configuring the container #23834

nicolas-grekas · 2017-08-08T18:20:33Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22407
License	MIT
Doc PR	-

This PR allows one to write DI configuration using just PHP, with full IDE auto-completion.
Example:

namespace Symfony\Component\DependencyInjection\Loader\Configurator;

use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo;

return function (ContainerConfigurator $c) {

    $c->import('basic.php');

    $s = $c->services()->defaults()
        ->public()
        ->private()
        ->autoconfigure()
        ->autowire()
        ->tag('t', array('a' => 'b'))
        ->bind(Foo::class, ref('bar'))
        ->private();

    $s->set(Foo::class)->args([ref('bar')])->public();
    $s->set('bar', Foo::class)->call('setFoo')->autoconfigure(false);

};

Ocramius · 2017-08-08T18:23:59Z

src/Symfony/Component/DependencyInjection/Loader/PhpFileLoader/functions.php

+ */
+function _defaults()
+{
+    return PhpFileLoader::call(function($file, Definition $defaults) {


Don't really like any of these global functions - they rely on a single container existing, and these are a big problem if there is more than one container in an app (not uncommon at all)

Thanks for joining :) If you look at the implementation of the PhpFileLoader::call function, it borrows the current container from the stack trace, and complains if not called in the correct context. So that there is no issues with having several containers.
Or you meant something else?
Of course, this is done for the purpose of making the DSL work, while still writting PHP. I wouldn't advocate this technique anywhere else.

Ah, I see what you mean here.

Still don't think it's a good idea. Isn't it simpler to use $this, and have the PhpFileLoader or equivalent being the current context?

This also leads to easy auto-completion in the file itself, as you just need to add /* @var $this The\Thing */ at the top to have static analysis and autocompletion.

nicolas-grekas · 2017-08-08T19:41:53Z

(continuing #23834 (comment))

Isn't it simpler to use $this, and have the PhpFileLoader or equivalent being the current context?

Fortunately, the current context is already the PhpFileLoader instance, so $this is the correct one.
That's how I started in fact, but wasn't convinced. We can try harder. With functions, it looked nicer to me (no matter the implementation, which is not the thing I tried to optimize.)

This also leads to easy auto-completion in the file itself

don't you have auto-completion here also with the functions in the current namespace?

Ocramius · 2017-08-08T19:44:01Z

don't you have auto-completion here also with the functions in the current namespace?

Yes, but with an ugly ugly hack ;-)

Just using $this is perfectly fine

mnapoli · 2017-08-08T20:23:48Z

Very happy to see this,

The referenced issue was using an array-like syntax (service(MyService::class) vs [MyService::class => service()]),
but having the $id of the services while creating or loading them allows better error messages, and is required for the load() function anyway. It also requires less keystrokes.

Not sure about error messages, but the main motivation I had to write an array is that it looks more like configuration (i.e. an array of container definitions, much like a service provider). The example of a file containing method calls looks more like a script/actions.

I believe also the issue with $this stems from the fact that global function calls affect a specific container instance. With an array that problem disappears because the array is simply a list of definitions, which can then be applied to a container but the list (the array) is not tied to a specific instance. Easier to reason about IMO.

Maybe another option would be an in-between (same example as above):

namespace Symfony\Component\DependencyInjection\Loader\PhpFileLoader;

use App\MyService;

return [

    // loads another resource
    import('another_file.yml'),

    // applies to any following services
    _defaults()->private()->autowire()->autoconfigure(),
    
    // creates a parameter named "foo"
    param('foo', 'bar'),
    
    // creates a public service whose id<>class is "App\MyService"
    service(MyService::class)->public(),
    
    // loads all classes in the `../src` directory, relative to the current file
    load('App\\', '../src/*', '../../src/{Entity,Repository}'),

]

I concede it's a bit less practical to type (indentation, and don't forget the , at the end of the line) but now it looks more like a list of definitions.

And I'm biased of course but a comparison with an array indexed by ids:

namespace Symfony\Component\DependencyInjection\Loader\PhpFileLoader;

use App\MyService;

return [

    // loads another resource
    import('another_file.yml'),

    // applies to any following services
    _defaults()->private()->autowire()->autoconfigure(),
    
    // creates a parameter named "foo"
   'foo' => 'bar',
    
    // creates a public service whose id<>class is "App\MyService"
    MyService::class => service()->public(),
    
    // loads all classes in the `../src` directory, relative to the current file
    load('App\\', '../src/*', '../../src/{Entity,Repository}'),

]

Not much difference in this example except for services and parameters. In a "regular" configuration file the difference will be more striking (I especially like shortcuts like for parameters, or setting up a service for autowiring, etc.).

nicolas-grekas · 2017-08-08T20:57:21Z

@mnapoli there is a major feature that I did not emphasis enough in the description:
accurate error reporting.

All the array-like syntax can't report errors accurately. With the implementation in this PR, exceptions will report the line where the error occurred, taking the context into account (eg using a ChildDefinition with incompatible things in "_defaults".)

That's THE feature that kills array-like variants to me. Imagine the number of people that WILL do mistake and spot where in a snap. That's of paramount importance.

mnapoli · 2017-08-08T21:02:38Z

Current status:

Yep that's a good point! The file/line of the definition could be stored by playing with debug_backtrace() but then it could be usable only in the exception message, not the exception stack trace I guess…

linaori · 2017-08-09T06:42:55Z

I like the DX part of writing this configuration, I'm concerned about debugging though. PhpFileLoader::call() does some nasty (yet interesting) stuff which might make finding bugs a lot harder for developers that think they might have found a bug, or developers that do encounter bugs with (for example) multiple containers. @Ocramius made a valid point, if it looks like an ugly hack, it probably is an ugly hack.

I would also like to see a lot more information in the docblocks in functions.php, as this will be the API we would be using.

While I'm not sure about the implementation, I think it's a very good step in the right direction when writing service configurations!

ro0NL · 2017-08-09T07:36:53Z

Nice :) I dont really get why you prefer __call though.

About the implem. why not inverse the logic? Register the container from load() (set($container)?), and use get()? from there...

Also consider some plural functions like imports(), aliases(), params().

Not sure about error messages, but the main motivation I had to write an array is that it looks more like configuration (i.e. an array of container definitions, much like a service provider). The example of a file containing method calls looks more like a script/actions.

Im btw in favor of the latter; no extra array boilerplate syntax.

stof · 2017-08-09T08:07:19Z

@ro0NL __call is used only in places where function names are using reserved keywords of PHP 5.x (which are allowed when calling functions, but not when defining them). They can be replaced by actual methods in 4.0 when dropping support for PHP 5.X.

Regarding the plural functions, this would be a pain for the returned configurator object allow further configuration.

Tobion · 2017-08-09T22:08:13Z

I thought #23819 is the better way to go and replaces the need for this.

Tobion · 2017-09-20T09:05:59Z

the whole code base is consistent with this convention

It's because there has been no variadics in php 5.3. That means we could never use new php features because it might surprise people and it's not consistent with our existing code.

Using an array, is not self-explaining at all. What does $service->args([1 => 1, 2, '$k' => 3, 'foo' => 4]) mean? And what is the error message if there is one.

nicolas-grekas · 2017-09-20T09:09:40Z

@Tobion if someone (you?) wants to push stronger for variadics, I'd suggest doing so in a dedicated PR, after this one (and the Routing one) have been merged, so that we can move forward.

mvrhov · 2017-09-20T10:46:21Z

👍 as Nicolas said. I'd like to start playing with this and especially with the routing one.

…icolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [Routing] Add PHP fluent DSL for configuring routes | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - If we add a PHP DSL for DI config (#23834), we must have a similar one for routing. Here it is. See fixtures. So, you always start with a `RoutingConfigurator $routes`, which allows you to: ```php $routes->add($name, $path); // adds a route $routes->import($resource, $type = null, $ignoreErrors = false); // imports routes $routes->collection($name = ''); // starts a collection, all *names* might be prefixed ``` then - for "import" and "collection", you can `->prefix($path)`; - for "add" and "collection", you can fluently "add" several times; - for "collection" you add sub"`->collection()`"; - and on all, you can configure the route(s) with: ```php ->defaults(array $defaults) ->requirements(array $requirements) ->options(array $options) ->condition($condition) ->host($pattern) ->schemes(array $schemes) ->methods(array $methods) ->controller($controller) ``` Commits ------- f433c9a [Routing] Add PHP fluent DSL for configuring routes

…iner (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Add "PHP fluent format" for configuring the container | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22407 | License | MIT | Doc PR | - This PR allows one to write DI configuration using just PHP, with full IDE auto-completion. Example: ```php namespace Symfony\Component\DependencyInjection\Loader\Configurator; use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo; return function (ContainerConfigurator $c) { $c->import('basic.php'); $s = $c->services()->defaults() ->public() ->private() ->autoconfigure() ->autowire() ->tag('t', array('a' => 'b')) ->bind(Foo::class, ref('bar')) ->private(); $s->set(Foo::class)->args([ref('bar')])->public(); $s->set('bar', Foo::class)->call('setFoo')->autoconfigure(false); }; ``` Commits ------- 814cc31 [DI] Add "PHP fluent format" for configuring the container

nicolas-grekas · 2017-09-20T13:09:30Z

Merged, thanks to everyone for the discussion and review, time for practice now :)

ro0NL · 2017-09-20T13:14:25Z

Thank you @nicolas-grekas. It's gonna be huge.

OskarStark · 2017-09-26T08:20:08Z

src/Symfony/Component/DependencyInjection/Loader/Configurator/AbstractConfigurator.php

+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\ExpressionLanguage\Expression;
+
+abstract class AbstractConfigurator


missing @author tag

OskarStark · 2017-09-26T08:20:16Z

src/Symfony/Component/DependencyInjection/Loader/Configurator/AbstractServiceConfigurator.php

+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException;
+
+abstract class AbstractServiceConfigurator extends AbstractConfigurator


missing @author tag

@author tags are not required.

…es_{env} with php extension (helyakin) This PR was merged into the 6.4 branch. Discussion ---------- [HttpKernel] when configuring the container add services_{env} with php extension | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | none | License | MIT Hello the community This is my first PR attempt. So after reading this [documentation](https://symfony.com/doc/current/service_container.html#remove-services) and unsuccessfully trying to load my `service_test.php`, I've noticed that the `configureContainer(..)` function in the `MicroKernelTrait` file was not configured to automatically load this file. So either we should fix the documentation, either we should fix the configuration. Since this the [framework-bundle](https://github.com/symfony/framework-bundle) is backed by [Alximy](https://alximy.io) I figured it would be cool 😎 to try and fix 🐛 the configuration. Anyway share me your thoughts about what should be done ! And I wanted to say that php service configuration is 🔥 so shoutout to the one who did this (I think it's you `@nicolas`-grekas with this [PR](#23834) right ? 💪🏻) Also big thanks to `@jeremyFreeAgent` for debugging this with me and `@HeahDude` for showing me the php service configuration PR. Commits ------- 4aac1d9 🐛 (kernel) when configuring the container add services with php ext

…es_{env} with php extension (helyakin) This PR was merged into the 6.4 branch. Discussion ---------- [HttpKernel] when configuring the container add services_{env} with php extension | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | none | License | MIT Hello the community This is my first PR attempt. So after reading this [documentation](https://symfony.com/doc/current/service_container.html#remove-services) and unsuccessfully trying to load my `service_test.php`, I've noticed that the `configureContainer(..)` function in the `MicroKernelTrait` file was not configured to automatically load this file. So either we should fix the documentation, either we should fix the configuration. Since this the [framework-bundle](https://github.com/symfony/framework-bundle) is backed by [Alximy](https://alximy.io) I figured it would be cool 😎 to try and fix 🐛 the configuration. Anyway share me your thoughts about what should be done ! And I wanted to say that php service configuration is 🔥 so shoutout to the one who did this (I think it's you `@nicolas`-grekas with this [PR](symfony/symfony#23834) right ? 💪🏻) Also big thanks to `@jeremyFreeAgent` for debugging this with me and `@HeahDude` for showing me the php service configuration PR. Commits ------- 4aac1d9fee 🐛 (kernel) when configuring the container add services with php ext

nicolas-grekas added this to the 3.4 milestone Aug 8, 2017

carsonbot added Status: Needs Review DependencyInjection Feature labels Aug 8, 2017

Ocramius reviewed Aug 8, 2017

View reviewed changes

nicolas-grekas force-pushed the di-php-dsl branch 3 times, most recently from ca4a496 to 772ad9d Compare August 8, 2017 19:11

nicolas-grekas mentioned this pull request Aug 8, 2017

[DI] Add a new "PHP fluent format" for configuring the container #22407

Closed

nicolas-grekas changed the title ~~[DI] Add a new "PHP fluent format" for configuring the container~~ [DI] Add a "PHP fluent format" for configuring the container Aug 8, 2017

nicolas-grekas changed the title ~~[DI] Add a "PHP fluent format" for configuring the container~~ [DI] Add "PHP fluent format" for configuring the container Aug 8, 2017

nicolas-grekas force-pushed the di-php-dsl branch from 772ad9d to 9849a3d Compare August 8, 2017 19:32

nicolas-grekas force-pushed the di-php-dsl branch 2 times, most recently from 6ba0ba1 to 1d9f16f Compare August 8, 2017 21:54

nicolas-grekas force-pushed the di-php-dsl branch 2 times, most recently from 8b6272e to a4327a0 Compare August 9, 2017 07:46

nicolas-grekas force-pushed the di-php-dsl branch 4 times, most recently from e72f59f to f3e1bb1 Compare August 9, 2017 21:08

fabpot mentioned this pull request Aug 10, 2017

[DI] Allow using a class factory #23819

Closed

[DI] Add "PHP fluent format" for configuring the container

814cc31

nicolas-grekas force-pushed the di-php-dsl branch from 6fc61f0 to 814cc31 Compare September 20, 2017 07:09

ogizanagi self-requested a review September 20, 2017 07:31

ogizanagi approved these changes Sep 20, 2017

View reviewed changes

nicolas-grekas merged commit 814cc31 into symfony:3.4 Sep 20, 2017

nicolas-grekas deleted the di-php-dsl branch September 20, 2017 13:09

Spomky mentioned this pull request Sep 20, 2017

Submission to Symfony mnapoli/fluent-symfony#32

Open

OskarStark reviewed Sep 26, 2017

View reviewed changes

javiereguiluz mentioned this pull request Oct 3, 2017

Added new fluent PHP config formats for services and routes symfony/symfony-docs#8462

Closed

This was referenced Oct 9, 2017

[3.4] Completion for PHP-based configuration for services Haehnchen/idea-php-symfony2-plugin#1041

Open

[3.4] Index for PHP-based configuration for services Haehnchen/idea-php-symfony2-plugin#1045

Open

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

nicolas-grekas mentioned this pull request Oct 25, 2017

Autoconfigure options overridden with PHP config #24687

Closed

GuilhemN mentioned this pull request Jan 4, 2018

Add areas support nelmio/NelmioApiDocBundle#1169

Merged

jderusse mentioned this pull request Mar 26, 2018

WIP: Switch to fluent services ekino/EkinoNewRelicBundle#173

Closed

ajgarlag mentioned this pull request Jul 30, 2018

Allow importing services from PHP and XML configuration files jakzal/BehatNoExtension#16

Merged

This was referenced Jun 23, 2023

🐛 (kernel) when configuring the container add services with php extension symfony/framework-bundle#37

Closed

[HttpKernel] when configuring the container add services_{env} with php extension #50754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add "PHP fluent format" for configuring the container #23834

[DI] Add "PHP fluent format" for configuring the container #23834

nicolas-grekas commented Aug 8, 2017 •

edited

Loading

Ocramius Aug 8, 2017

nicolas-grekas Aug 8, 2017

Ocramius Aug 8, 2017

Ocramius Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

Ocramius commented Aug 8, 2017

mnapoli commented Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

mnapoli commented Aug 8, 2017

linaori commented Aug 9, 2017

ro0NL commented Aug 9, 2017 •

edited

Loading

stof commented Aug 9, 2017

Tobion commented Aug 9, 2017 •

edited

Loading

Tobion commented Sep 20, 2017

nicolas-grekas commented Sep 20, 2017

mvrhov commented Sep 20, 2017

nicolas-grekas commented Sep 20, 2017

ro0NL commented Sep 20, 2017

OskarStark Sep 26, 2017

OskarStark Sep 26, 2017

fabpot Sep 26, 2017

[DI] Add "PHP fluent format" for configuring the container #23834

[DI] Add "PHP fluent format" for configuring the container #23834

Conversation

nicolas-grekas commented Aug 8, 2017 • edited Loading

Ocramius Aug 8, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 8, 2017

Choose a reason for hiding this comment

Ocramius Aug 8, 2017

Choose a reason for hiding this comment

Ocramius Aug 8, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Aug 8, 2017

Ocramius commented Aug 8, 2017

mnapoli commented Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

mnapoli commented Aug 8, 2017

linaori commented Aug 9, 2017

ro0NL commented Aug 9, 2017 • edited Loading

stof commented Aug 9, 2017

Tobion commented Aug 9, 2017 • edited Loading

Tobion commented Sep 20, 2017

nicolas-grekas commented Sep 20, 2017

mvrhov commented Sep 20, 2017

nicolas-grekas commented Sep 20, 2017

ro0NL commented Sep 20, 2017

OskarStark Sep 26, 2017

Choose a reason for hiding this comment

OskarStark Sep 26, 2017

Choose a reason for hiding this comment

fabpot Sep 26, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Aug 8, 2017 •

edited

Loading

ro0NL commented Aug 9, 2017 •

edited

Loading

Tobion commented Aug 9, 2017 •

edited

Loading