Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Add "PHP fluent format" for configuring the container#23834
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| */ | ||
| function _defaults() | ||
| { | ||
| return PhpFileLoader::call(function($file, Definition $defaults) { |
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.
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)
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.
Thanks for joining :) If you look at the implementation of thePhpFileLoader::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.
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.
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 thePhpFileLoader or equivalent being the current context?
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 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.
ca4a496 to772ad9dComparenicolas-grekas commentedAug 8, 2017
(continuing#23834 (comment))
Fortunately, the current context is already the PhpFileLoader instance, so
don't you have auto-completion here also with the functions in the current namespace? |
Ocramius commentedAug 8, 2017
Yes, but with an ugly ugly hack ;-) Just using |
mnapoli commentedAug 8, 2017
Very happy to see this,
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 Maybe another option would be an in-between (same example as above): namespaceSymfony\Component\DependencyInjection\Loader\PhpFileLoader;useApp\MyService;return [// loads another resourceimport('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 fileload('App\\','../src/*','../../src/{Entity,Repository}'),] I concede it's a bit less practical to type (indentation, and don't forget the And I'm biased of course but a comparison with an array indexed by ids: namespaceSymfony\Component\DependencyInjection\Loader\PhpFileLoader;useApp\MyService;return [// loads another resourceimport('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 fileload('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 commentedAug 8, 2017
@mnapoli there is a major feature that I did not emphasis enough in the description: 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 commentedAug 8, 2017
6ba0ba1 to1d9f16fComparelinaori commentedAug 9, 2017
I like the DX part ofwriting this configuration, I'm concerned about debugging though. I would also like to see a lot more information in the docblocks in 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 commentedAug 9, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Nice :) I dont really get why you prefer __call though. About the implem. why not inverse the logic? Register the container from load() ( Also consider some plural functions like
Im btw in favor of the latter; no extra array boilerplate syntax. |
8b6272e toa4327a0Comparestof commentedAug 9, 2017
@ro0NL Regarding the plural functions, this would be a pain for the returned configurator object allow further configuration. |
e72f59f tof3e1bb1CompareTobion commentedAug 9, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I thought#23819 is the better way to go and replaces the need for this. |
Tobion commentedSep 20, 2017
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 |
nicolas-grekas commentedSep 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 commentedSep 20, 2017
👍 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:```phpnamespace 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 commentedSep 20, 2017
Merged, thanks to everyone for the discussion and review, time for practice now :) |
ro0NL commentedSep 20, 2017
Thank you@nicolas-grekas. It's gonna be huge. |
| use Symfony\Component\DependencyInjection\Reference; | ||
| use Symfony\Component\ExpressionLanguage\Expression; | ||
| abstract class AbstractConfigurator |
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.
missing@author tag
| use Symfony\Component\DependencyInjection\Definition; | ||
| use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException; | ||
| abstract class AbstractServiceConfigurator extends AbstractConfigurator |
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.
missing@author tag
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.
@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 | MITHello the communityThis 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 | MITHello the communityThis 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…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 | MITHello the communityThis 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… format for semantic configuration (nicolas-grekas)This PR was merged into the 7.4 branch.Discussion----------[Config][DependencyInjection] Deprecate the fluent PHP format for semantic configuration| Q | A| ------------- | ---| Branch? | 7.4| Bug fix? | no| New feature? | yes| Deprecations? | no| Issues | -| License | MITThis PR deprecates the fluent PHP format for semantic configuration introduced in Symfony 5.4 by `@Nyholm` (see#40600).It aims to replace it with the new array-based PHP config format (see#61490).The fluent PHP config format was a great experiment:- It helped us improve the Config component and the code generation of fluent config builders.- It confirmed the community’s interest in PHP-based configuration.- And it showed us its limits.Those limits are structural. Writing fluent config is difficult and full of edge cases. Its rigidity comes from having to match one canonical interpretation of the semantic config tree. Automatic code generation can’t capture the custom logic that before-normalizers introduce, yet those normalizers are essential for flexibility and backward compatibility. This rigidity makes fluent config fragile. How do we deal with this fragility as config tree authors? At the moment, we don't care. Maybe this format is too niche for you to have experienced this issue, but we cannot guarantee that simple upgrades won't break your fluent PHP config.The new array-based PHP format builds directly on the same code used for loading YAML configs.That means:- trivial conversion between YAML and PHP arrays,- identical flexibility and behavior,- and support for auto-completion and static analysis through generated array shapes.The generated array shapes are rigid too, but that rigidity is non-breaking: even if your config no longer matches the canonical shape, your app keeps working. Static analyzers might warn you: that’s an invitation to update, not a failure.I'm submitting this PR a bit l late for 7.4 but I think it's important to merge now. Deprecating the fluent PHP config format will:- prevent new code from relying on a fragile approach,- make room in the documentation for the array-based format,- and consolidate Symfony’s configuration story around a robust PHP-based format.Fluent PHP for semantic config served us well but it's time to retire it. ```diff -return function (AcmeConfig $config) { - $config->color('red'); -} +return new AcmeConfig([ + 'color' => 'red', +]); ```PS: there's another fluent config format for services and routes (see#23834 and#24180). This other format is handwritten. It doesn't have the issues listed above and it is *not* deprecated. It's actually the recommended way *for bundles* to declare their config (instead of XML, see#60568).Commits-------332b4ac [Config][DependencyInjection] Deprecate the fluent PHP format for semantic configuration

Uh oh!
There was an error while loading.Please reload this page.
This PR allows one to write DI configuration using just PHP, with full IDE auto-completion.
Example: