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] Add "PHP fluent format" for configuring the container #23834

Merged
merged 1 commit into from Sep 20, 2017

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Aug 8, 2017

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);

};

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

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 8, 2017

Member

(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?

Member

nicolas-grekas commented Aug 8, 2017

(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

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Aug 8, 2017

Contributor

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

Contributor

Ocramius commented Aug 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 8, 2017

Contributor

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.).

Contributor

mnapoli commented Aug 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 8, 2017

Member

@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.

Member

nicolas-grekas commented Aug 8, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 8, 2017

Contributor

Current status:

capture d ecran 2017-08-08 a 23 00 16

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…

Contributor

mnapoli commented Aug 8, 2017

Current status:

capture d ecran 2017-08-08 a 23 00 16

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…

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Aug 9, 2017

Contributor

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!

Contributor

iltar commented Aug 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Aug 9, 2017

Contributor

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.

Contributor

ro0NL commented Aug 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 9, 2017

Member

@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.

Member

stof commented Aug 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Aug 9, 2017

Member

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

Member

Tobion commented Aug 9, 2017

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

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Aug 10, 2017

Member

In my opinion, a fluent interface is only useful when it's fully used as a fluent interface. I type $this-> and my IDE autocompletes everything. I don't have to learn anything, I don't have to read any doc and things are always up-to-date because it's autocompleted from the code.

Defining a bunch of global functions param(), service(), etc. may look useful, but it's not that useful because I must learn all those global shortcuts. In fact, it increases the cognitive load of the framework and its learning curve.

If we want to do that, we can think of a nice fluent interface to configure everything. Quick refactor of the previous examples:

return $this
    ->makePublicAllServices(false)
    ->autowireAllServices(true)
    ->autoconfigureAllServices(true)

    ->import('another_file.yml')

    ->parameter('key', 'value')

    ->service(MyService::class)
        ->public()
    ->service(MyOtherService::class)
        ->arguments(['dir' => '%kernel.project_dir%'])

    ->load('App\\', '../src/*', '../../src/{Entity,Repository}')
;

According to our latest community survey, 80% of Symfony developers use PHPStorm, so $this->... would be autocompleted for them.

Member

javiereguiluz commented Aug 10, 2017

In my opinion, a fluent interface is only useful when it's fully used as a fluent interface. I type $this-> and my IDE autocompletes everything. I don't have to learn anything, I don't have to read any doc and things are always up-to-date because it's autocompleted from the code.

Defining a bunch of global functions param(), service(), etc. may look useful, but it's not that useful because I must learn all those global shortcuts. In fact, it increases the cognitive load of the framework and its learning curve.

If we want to do that, we can think of a nice fluent interface to configure everything. Quick refactor of the previous examples:

return $this
    ->makePublicAllServices(false)
    ->autowireAllServices(true)
    ->autoconfigureAllServices(true)

    ->import('another_file.yml')

    ->parameter('key', 'value')

    ->service(MyService::class)
        ->public()
    ->service(MyOtherService::class)
        ->arguments(['dir' => '%kernel.project_dir%'])

    ->load('App\\', '../src/*', '../../src/{Entity,Repository}')
;

According to our latest community survey, 80% of Symfony developers use PHPStorm, so $this->... would be autocompleted for them.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 10, 2017

Member

@javiereguiluz, as @stof said:

this would be a pain for the returned configurator object allow further configuration.

what it means is that long fluent string, with un-indentation to close a service, that you as a human sees as a termination of a service declaration, neither PHP nor PhpStorm sees it as such. I've been told by the same PhpStorm users that the fluent interface of the Config component is a pain because of this reason. The ; to "commit" a service/param/etc. is the salvatory syntax that removes any ambiguities.

Then, doing $this->service() instead of just service() is adding boilerplate for no reason.
When you type in Yaml, you don't "return" nor reference "$this". Same here, the DSL is declarative. BTW, "$this" here is meaningless for PhpStorm because we are not inside a class. To make it meaningful, you need to add another type annotation. More boilerplate.

Since functions are namespaced, PhpStorm is perfectly able to autocomplete them. About the mental overhead, that'll help a lot. The fact that the vocabulary closely matches what people are used to in yaml/xml also helps reduce the mental overhead.

The more I work on this, the more I like the current syntax. It's straightforward.

Member

nicolas-grekas commented Aug 10, 2017

@javiereguiluz, as @stof said:

this would be a pain for the returned configurator object allow further configuration.

what it means is that long fluent string, with un-indentation to close a service, that you as a human sees as a termination of a service declaration, neither PHP nor PhpStorm sees it as such. I've been told by the same PhpStorm users that the fluent interface of the Config component is a pain because of this reason. The ; to "commit" a service/param/etc. is the salvatory syntax that removes any ambiguities.

Then, doing $this->service() instead of just service() is adding boilerplate for no reason.
When you type in Yaml, you don't "return" nor reference "$this". Same here, the DSL is declarative. BTW, "$this" here is meaningless for PhpStorm because we are not inside a class. To make it meaningful, you need to add another type annotation. More boilerplate.

Since functions are namespaced, PhpStorm is perfectly able to autocomplete them. About the mental overhead, that'll help a lot. The fact that the vocabulary closely matches what people are used to in yaml/xml also helps reduce the mental overhead.

The more I work on this, the more I like the current syntax. It's straightforward.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Aug 10, 2017

Member

what it means is that long fluent string, with un-indentation to close a service, that you as a human sees as a termination of a service declaration, neither PHP nor PhpStorm sees it as such.

You can solve this with code and without having to add those ugly end() methods. I did it for the fluent config of this bundle.

I've been told by the same PhpStorm users that the fluent interface of the Config component is a pain because of this reason.

I agree, the Config component is a pain, and not only because of the end() methods.

I'm not saying that your proposal is wrong. I'm saying that your proposal requires me to learn and remember the names and usages of a bunch of global functions. The real fluent interface requires me to learn nothing. I type $this-> and everything flows smoothly.

Member

javiereguiluz commented Aug 10, 2017

what it means is that long fluent string, with un-indentation to close a service, that you as a human sees as a termination of a service declaration, neither PHP nor PhpStorm sees it as such.

You can solve this with code and without having to add those ugly end() methods. I did it for the fluent config of this bundle.

I've been told by the same PhpStorm users that the fluent interface of the Config component is a pain because of this reason.

I agree, the Config component is a pain, and not only because of the end() methods.

I'm not saying that your proposal is wrong. I'm saying that your proposal requires me to learn and remember the names and usages of a bunch of global functions. The real fluent interface requires me to learn nothing. I type $this-> and everything flows smoothly.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Aug 10, 2017

Contributor

When you type in Yaml

First mistake :P

Then, doing $this->service() instead of just service() is adding boilerplate for no reason.

It is giving you automatic documentation and auto-completion though, as @javiereguiluz stated.
In addition to that, all you are doing with the debug_backtrace() mumbo-jumbo (that somebody will have to debug at some point) is just providing an implicit $this: that's a lot of magic just to save typing the first $this. As much as I hate fluent APIs, this is a perfect use-case for one.

Since functions are namespaced, PhpStorm is perfectly able to autocomplete them.

You still need to know the namespace and all possible existing functions in there in order to be able to work with it: auto-completion would be manually typing `\The\Names

Contributor

Ocramius commented Aug 10, 2017

When you type in Yaml

First mistake :P

Then, doing $this->service() instead of just service() is adding boilerplate for no reason.

It is giving you automatic documentation and auto-completion though, as @javiereguiluz stated.
In addition to that, all you are doing with the debug_backtrace() mumbo-jumbo (that somebody will have to debug at some point) is just providing an implicit $this: that's a lot of magic just to save typing the first $this. As much as I hate fluent APIs, this is a perfect use-case for one.

Since functions are namespaced, PhpStorm is perfectly able to autocomplete them.

You still need to know the namespace and all possible existing functions in there in order to be able to work with it: auto-completion would be manually typing `\The\Names

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Aug 10, 2017

Contributor

I think we need to put api functions at least in a dedicated namespace, to improve autocomplete;

image

And while doing so it may open up to be used outside file loader context, ie why cant i do this?

use Symfony\Component\DependencyInjection\X as fn;

fn\set($container = new ContainerBuilder());
// configure as usual

$container->compile();
Contributor

ro0NL commented Aug 10, 2017

I think we need to put api functions at least in a dedicated namespace, to improve autocomplete;

image

And while doing so it may open up to be used outside file loader context, ie why cant i do this?

use Symfony\Component\DependencyInjection\X as fn;

fn\set($container = new ContainerBuilder());
// configure as usual

$container->compile();
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 10, 2017

Member

the debug_backtrace() mumbo-jumbo (that somebody will have to debug at some point)

I think these 4 lines of internal details are a much more affordable technical debts than the Yaml component itself. One I'm wiling to push to the community.

The fluent interface doesn't solve the helpers use case, which are also needed. Writing $this->ref('foo') instead of ref('foo') looks like another boilerplate that's better removed.

need to know the namespace

true, that's gonna be easy: just copy past from existing examples, as everyone has to do anyway already with the other formats.

auto-completion would be manually typing `\The\Names

that copy-pasteable "header" could also be
use Symfony\Component\DependencyInjection\Loader\PhpFileLoader as set; then set\...-autocomplete => set\service()->...-autocomplete, etc.

or use function Symfony\Component\DependencyInjection\Loader\PhpFileLoader\{ import, service, param, etc }; and then possible things would just be written explicitly at the top of the file.

All in all I agree with you guys that this targets the highest possible autocompletion experience, so be sure we're aligned on that.

@javiereguiluz what you achieve in your bundle is really nice, but it works around the issue we have here, which is nesting, so that we can't really take inspiration from it (but it's really nice!).

@ro0NL nice to see you tried, I wish everyone does. Yes, we need to do something here to un-pollute auto-completion.

BTW to all, I dunno what you mean with all the "👎", but to me at least, it adds emotional drama to a discussion that should be mostly technical. Not sure it really helps.

Member

nicolas-grekas commented Aug 10, 2017

the debug_backtrace() mumbo-jumbo (that somebody will have to debug at some point)

I think these 4 lines of internal details are a much more affordable technical debts than the Yaml component itself. One I'm wiling to push to the community.

The fluent interface doesn't solve the helpers use case, which are also needed. Writing $this->ref('foo') instead of ref('foo') looks like another boilerplate that's better removed.

need to know the namespace

true, that's gonna be easy: just copy past from existing examples, as everyone has to do anyway already with the other formats.

auto-completion would be manually typing `\The\Names

that copy-pasteable "header" could also be
use Symfony\Component\DependencyInjection\Loader\PhpFileLoader as set; then set\...-autocomplete => set\service()->...-autocomplete, etc.

or use function Symfony\Component\DependencyInjection\Loader\PhpFileLoader\{ import, service, param, etc }; and then possible things would just be written explicitly at the top of the file.

All in all I agree with you guys that this targets the highest possible autocompletion experience, so be sure we're aligned on that.

@javiereguiluz what you achieve in your bundle is really nice, but it works around the issue we have here, which is nesting, so that we can't really take inspiration from it (but it's really nice!).

@ro0NL nice to see you tried, I wish everyone does. Yes, we need to do something here to un-pollute auto-completion.

BTW to all, I dunno what you mean with all the "👎", but to me at least, it adds emotional drama to a discussion that should be mostly technical. Not sure it really helps.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Aug 10, 2017

Member

About the nesting problem, it's simple to solve with smart coding:

  1. All methods return self to keep the fluent interface.
  2. Some methods change the behavior of the previous methods.

For example, public() and arguments() configure a service, so they change the definition of the immediately previous service() call. This just requires to save the global state of the config. But it's very simple to do because the number of methods is incredibly limited. This is not a general-purpose configurator.

Member

javiereguiluz commented Aug 10, 2017

About the nesting problem, it's simple to solve with smart coding:

  1. All methods return self to keep the fluent interface.
  2. Some methods change the behavior of the previous methods.

For example, public() and arguments() configure a service, so they change the definition of the immediately previous service() call. This just requires to save the global state of the config. But it's very simple to do because the number of methods is incredibly limited. This is not a general-purpose configurator.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 10, 2017

Member

That'd mean all possible options would be provided all the time. Here, that's maybe not visible at glance, but autocompletion is already context full: you'll get only the suggestions that make sense for the current context.
Eg after parent(), you won't be suggested with autoconfigure(), because that's forbidden anyway. Etc.

That's a major UX feature. Full fluentness doesn't deserve a higher priority to me.

Member

nicolas-grekas commented Aug 10, 2017

That'd mean all possible options would be provided all the time. Here, that's maybe not visible at glance, but autocompletion is already context full: you'll get only the suggestions that make sense for the current context.
Eg after parent(), you won't be suggested with autoconfigure(), because that's forbidden anyway. Etc.

That's a major UX feature. Full fluentness doesn't deserve a higher priority to me.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Aug 10, 2017

Contributor

To be honest, I like all of the suggestions as they all have their own pros and cons:

  • $this->, more verbose but easier autocomplete
  • fn\set() slightly less verbose and only a single use statement with nice autocomplete
  • set() no "visual debt" (woops), but slightly more difficult UX wise for imports
  • array notation feels odd because of the extra indent, but really simple and less magic internally
  • The factory (from #23819) is very verbose but also very powerful

The second and third go hand in hand as they can rely on the same API, but I think I can live with any of those implementations. My criteria would be:

  • Internal workings can be ugly as long as it doesn't bother the developers (debugging shouldn't be too complex either in case of bugs)
  • The public API should be intuitive, easy to remember and be IDE friendly without any docblock hacks to get things to work (UX factor)
Contributor

iltar commented Aug 10, 2017

To be honest, I like all of the suggestions as they all have their own pros and cons:

  • $this->, more verbose but easier autocomplete
  • fn\set() slightly less verbose and only a single use statement with nice autocomplete
  • set() no "visual debt" (woops), but slightly more difficult UX wise for imports
  • array notation feels odd because of the extra indent, but really simple and less magic internally
  • The factory (from #23819) is very verbose but also very powerful

The second and third go hand in hand as they can rely on the same API, but I think I can live with any of those implementations. My criteria would be:

  • Internal workings can be ugly as long as it doesn't bother the developers (debugging shouldn't be too complex either in case of bugs)
  • The public API should be intuitive, easy to remember and be IDE friendly without any docblock hacks to get things to work (UX factor)
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 10, 2017

Contributor

Just some feedback on my experience with using a similar syntax (i.e. no $this->): it works fine, yes you don't get autocompletion for the first letters but if what you have to memorize is param and service then it's not really an issue. All other scenarios will probably be copy-pasted from the documentation or examples, or even it's easy to autocomplete once you've typed the namespace or the first letters.

So in my experience using that kind of API (on PHP-DI), it's not a problem. The shortness is very enjoyable.

A suggestion: currently the "scope/context" is defined by the file. Why not use a closure to encapsulate the config? That would avoid playing with the backtrace, and that would be a bit clearer? (but that would indent the config by 1 tab)
I'm thinking this could be also useful to "embedded" config in another file, for example to override a bit of container config in tests, or to write container config in a micro-application (i.e. not having to store it in a separate file). That's a pro (IMO) of array config since it's context-agnostic, so maybe the same could be achieve with a closure here (or any kind of callable?).

Another suggestion: write more specific helpers, e.g. an autowire() function to autowire a service? (that way the config is shorter, more explicit, simpler, etc.)

@iltar

The factory (from #23819) is very verbose but also very powerful

Yes, but with PHP-based config (as opposed to XML/YAML) another possibility opens: using closures to create services (à la Pimple). That would allow to use the "very powerful" aspect of service providers, but still make it optional to not be forced to write verbose config.

And that could be made compatible with Symfony's compilation pass. I've implemented that for PHP-DI 6 (PHP-DI/PHP-DI#507), that might be interesting to leave the door open for that in Symfony?

Contributor

mnapoli commented Aug 10, 2017

Just some feedback on my experience with using a similar syntax (i.e. no $this->): it works fine, yes you don't get autocompletion for the first letters but if what you have to memorize is param and service then it's not really an issue. All other scenarios will probably be copy-pasted from the documentation or examples, or even it's easy to autocomplete once you've typed the namespace or the first letters.

So in my experience using that kind of API (on PHP-DI), it's not a problem. The shortness is very enjoyable.

A suggestion: currently the "scope/context" is defined by the file. Why not use a closure to encapsulate the config? That would avoid playing with the backtrace, and that would be a bit clearer? (but that would indent the config by 1 tab)
I'm thinking this could be also useful to "embedded" config in another file, for example to override a bit of container config in tests, or to write container config in a micro-application (i.e. not having to store it in a separate file). That's a pro (IMO) of array config since it's context-agnostic, so maybe the same could be achieve with a closure here (or any kind of callable?).

Another suggestion: write more specific helpers, e.g. an autowire() function to autowire a service? (that way the config is shorter, more explicit, simpler, etc.)

@iltar

The factory (from #23819) is very verbose but also very powerful

Yes, but with PHP-based config (as opposed to XML/YAML) another possibility opens: using closures to create services (à la Pimple). That would allow to use the "very powerful" aspect of service providers, but still make it optional to not be forced to write verbose config.

And that could be made compatible with Symfony's compilation pass. I've implemented that for PHP-DI 6 (PHP-DI/PHP-DI#507), that might be interesting to leave the door open for that in Symfony?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Aug 11, 2017

Contributor

Will param() ever have a configuration chain? it implies void currently.. so from the looks of it we can support params() (plural) as well.

Also what about a env param wrapper? env('X')

Contributor

ro0NL commented Aug 11, 2017

Will param() ever have a configuration chain? it implies void currently.. so from the looks of it we can support params() (plural) as well.

Also what about a env param wrapper? env('X')

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 19, 2017

Member

Using variadics has clear advantage as it allows people to use variadics or an array with(...[1]). But doing the opposite prevents that. Why should I write with([1]) when I could write with(1)?

Member

Tobion commented Sep 19, 2017

Using variadics has clear advantage as it allows people to use variadics or an array with(...[1]). But doing the opposite prevents that. Why should I write with([1]) when I could write with(1)?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 20, 2017

Member

@Tobion during the last days, we considered 3 variants (being able to set a single arg and also being able to set all args at once has been a requirement):

  1. the Laravel-like (ie a polymorphic signature, incompatible with variadics):
    $service->with([1,2,'$k' => 3])->with('$name', 'value);
  2. the variadic one (ie yours almost):
    $service->args(1,2,3)->arg('$name', 'value);
  3. the classic one:
    $service->args([1,2,'$k' => 3])->arg('$name', 'value);

v3 is the most deceptive one, because it's the classic boring one. I personally also find v1 or v2 more smart. But following the least-surprise principle is always a wise advice. Considering also that in the routing DSL #24180, all methods with "s" take an array - and that the whole code base is consistent with this convention, choosing the variadic way (same for the polymorphic one) would be a surprise to many for sure. This reasoning supports choosing v3.

To a wider audience: I think we should now end this discussion and consider celebrating a better way to configure our services soon. This PR confirmed that a PHP DSL is desirable. We also confirmed that the implementation is technically OK. The only thing left is the flavor of the DSL. As @weaverryan wisely said:

We won't get 100% agreement from everyone on everything.

And that's fine. If we don't and make this discussion longer, we take the risk of having no PHP DSL at all... There is still non trivial work to do... (see https://github.com/symfony/symfony/projects/6).

Member

nicolas-grekas commented Sep 20, 2017

@Tobion during the last days, we considered 3 variants (being able to set a single arg and also being able to set all args at once has been a requirement):

  1. the Laravel-like (ie a polymorphic signature, incompatible with variadics):
    $service->with([1,2,'$k' => 3])->with('$name', 'value);
  2. the variadic one (ie yours almost):
    $service->args(1,2,3)->arg('$name', 'value);
  3. the classic one:
    $service->args([1,2,'$k' => 3])->arg('$name', 'value);

v3 is the most deceptive one, because it's the classic boring one. I personally also find v1 or v2 more smart. But following the least-surprise principle is always a wise advice. Considering also that in the routing DSL #24180, all methods with "s" take an array - and that the whole code base is consistent with this convention, choosing the variadic way (same for the polymorphic one) would be a surprise to many for sure. This reasoning supports choosing v3.

To a wider audience: I think we should now end this discussion and consider celebrating a better way to configure our services soon. This PR confirmed that a PHP DSL is desirable. We also confirmed that the implementation is technically OK. The only thing left is the flavor of the DSL. As @weaverryan wisely said:

We won't get 100% agreement from everyone on everything.

And that's fine. If we don't and make this discussion longer, we take the risk of having no PHP DSL at all... There is still non trivial work to do... (see https://github.com/symfony/symfony/projects/6).

@ogizanagi ogizanagi self-requested a review Sep 20, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 20, 2017

Member

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.

Member

Tobion commented Sep 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 20, 2017

Member

@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.

Member

nicolas-grekas commented Sep 20, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov Sep 20, 2017

Contributor

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

Contributor

mvrhov commented Sep 20, 2017

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

nicolas-grekas added a commit that referenced this pull request Sep 20, 2017

feature #24180 [Routing] Add PHP fluent DSL for configuring routes (n…
…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

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Sep 20, 2017

feature #23834 [DI] Add "PHP fluent format" for configuring the conta…
…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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 20, 2017

Member

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

Member

nicolas-grekas commented Sep 20, 2017

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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-php-dsl branch Sep 20, 2017

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 20, 2017

Contributor

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

Contributor

ro0NL commented Sep 20, 2017

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

@Spomky Spomky referenced this pull request Sep 20, 2017

Open

Submission to Symfony #32

@nicolas-grekas nicolas-grekas moved this from RFR to DONE in PHP-only config Sep 24, 2017

@sstok sstok referenced this pull request Sep 25, 2017

Merged

Symfony 3.4 services #18

sstok added a commit to park-manager/park-manager that referenced this pull request Sep 25, 2017

refactor #18 Symfony 3.4 services (sstok)
This PR was merged into the 1.0-dev branch.

Discussion
----------

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | 
| License       | MPL-v2.0
| Doc PR        | 

Related to symfony/symfony#23834

Commits
-------

13581ca Update Service definitions to use new DSL format
dd954a6 Weaken deprecation warnings to fix CI
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\ExpressionLanguage\Expression;
abstract class AbstractConfigurator

This comment has been minimized.

@OskarStark

OskarStark Sep 26, 2017

Contributor

missing @author tag

@OskarStark

OskarStark Sep 26, 2017

Contributor

missing @author tag

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

This comment has been minimized.

@OskarStark

OskarStark Sep 26, 2017

Contributor

missing @author tag

@OskarStark

OskarStark Sep 26, 2017

Contributor

missing @author tag

This comment has been minimized.

@fabpot

fabpot Sep 26, 2017

Member

@author tags are not required.

@fabpot

fabpot Sep 26, 2017

Member

@author tags are not required.

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