-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[FrameworkBundle] Generate Config class
#58771
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
[FrameworkBundle] Generate Config class
#58771
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
280475c to
1f9c0b0
Compare
|
Thank you for your review! I'm having a look at it and will soon address your comments |
1f9c0b0 to
1d3af0c
Compare
|
Prototyped arrays are now supported, here's a screenshot from the newly generated config (snippet): /* @param array{
* access_denied_url?: string|int|float|bool,
* session_fixation_strategy?: "none"|"migrate"|"invalidate",
* hide_user_not_found?: bool,
* erase_credentials?: bool,
* access_decision_manager?: array{
* strategy?: "affirmative"|"consensus"|"unanimous"|"priority",
* service?: string|int|float|bool,
* strategy_service?: string|int|float|bool,
* allow_if_all_abstain?: bool,
* allow_if_equal_granted_denied?: bool,
* },
* password_hashers?: array<array{
* algorithm?: string|int|float|bool,
* migrate_from?: array<string|int|float|bool>,
* hash_algorithm?: string|int|float|bool,
* key_length?: string|int|float|bool,
* ignore_case?: bool,
* encode_as_base64?: bool,
* iterations?: string|int|float|bool,
* cost?: int<4, 31>,
* memory_cost?: string|int|float|bool,
* time_cost?: string|int|float|bool,
* id?: string|int|float|bool,
* }>,
* providers?: array<array{
* id?: string|int|float|bool,
* chain?: array{
* providers?: array<string|int|float|bool>,
* },
* memory?: array{
* users?: array<array{
* password?: string|int|float|bool,
* roles?: array<string|int|float|bool>,
* }>,
* },
* ldap?: array{
* service: string|int|float|bool,
* base_dn: string|int|float|bool,
* search_dn?: string|int|float|bool,
* search_password?: string|int|float|bool,
* extra_fields?: array<string|int|float|bool>,
* default_roles?: array<string|int|float|bool>,
* uid_key?: string|int|float|bool,
* filter?: string|int|float|bool,
* password_attribute?: string|int|float|bool,
* },
* }>,
* ...
*/Providers, password hashers, etc. are all prototypes. |
1d3af0c to
81124f2
Compare
src/Symfony/Component/Config/Builder/ConfigBuilderGenerator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Config/Builder/ConfigFunctionGenerator.php
Outdated
Show resolved
Hide resolved
81124f2 to
0cd82fd
Compare
0cd82fd to
e84099f
Compare
|
To anyone willing to see improved support for this updated PHP config format, please upvote this phpstorm issue ;) |
e84099f to
35abd04
Compare
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ConfigBuilderCacheWarmer.php
Outdated
Show resolved
Hide resolved
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 is missing pre-filled shapes for "services", "imports" and "parameters" tree, which are not defined by a bundle, so don't have a configuration definition.
Also, can you investigate how an IDE like vscode could add support for auto-completing shapes? who's in charge of the most used vscode extensions so that we can ping them?
|
I'm also wondering if using named arguments for the 1st level provides the best DX: the IDE cannot know we want to type the name of an argument quickly enough I fear. Whereas if we start typing |
|
I like the named argument way of doing because it is a bit more convenient to Cmd/Ctrl+Click on the named argument to have the full doc at a glance, whereas the Cmd/Ctrl+Click isn't available for array indexes. If there are a lot of bundles enabled, being able to go to the "definition" of the named argument could really help |
35abd04 to
4a674ab
Compare
|
|
||
| private static function generateInlinePhpDocForNode(BaseNode $node): string | ||
| { | ||
| $hasContent = false; |
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.
needless var: $comment should start empty and be prefixed only at the end, if not empty anymore
also, CR/LF in $comment should be replaced by a space so that we're sure it remains a oneliner
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.
It currently outputs something like this for framework:
* strict_requirements?: string|int|float|bool, // set to true to throw an exception when a parameter does not match the requirements set to false to disable exceptions when a parameter does not match the requirements (and return null instead) set to null to disable parameter checks against requirements 'true' is the preferred configuration in development mode, while 'false' or 'null' might be preferred in production Default: true
Maybe we could instead replace \r and \n with ; ?
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.
Actually maybe not, it seems it's a particular case. Other places end the info with a ., so maybe we should adjust the info of this particular node.
src/Symfony/Component/DependencyInjection/Loader/PhpFileLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Resource/ParametersTrait.php
Outdated
Show resolved
Hide resolved
| * @var array<array{ | ||
| * id: string, | ||
| * class?: string|int|float|bool, | ||
| * }>|array<string, class-string|null> $services |
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 is minimalistic. In reality, we could be way more descriptive here, see what YamlFileLoader describes for services. Maybe you preferred postponing this to a follow up?
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.
I started implementing what's left (lazy, abstract, args, synthetic, etc.) but there's a bit more work than I expected. I propose to validate the PR as-is, then comeback to this bit when the rest is OK?
a371101 to
2990a68
Compare
1ab55b6 to
01fbab6
Compare
bf3efae to
dd104bf
Compare
dd104bf to
f185732
Compare
|
I completed the services array shape in f185732. |
|
I feel like we might have too much indirection with the current approach. namespace Symfony\Config;
return new FrameworkConfig([...]);IIUC, the constructor is already accepting the arrays we want, so we should just have to remove the current And in order to be able to configure many trees at once, we could also support returning an iterable: namespace Symfony\Config;
return [
new FrameworkConfig([...]),
new ServicesConfig([...]),
];WDYT? Wouldn't this make things a bit simpler? |
|
What do you mean by "too much indirection"?
Makes sense!
I find the current way already really nice. However, I don't have a strong opinion on this and the syntax you propose is also fine to me. Maybe it will finish to convince me once I understand the "indirection" part 🙂 |
|
Indirection from the Config class to the traits to the configure() methods. |
| $node instanceof StringNode => 'string', | ||
| $node instanceof NumericNode => self::handleNumericNode($node), | ||
| $node instanceof EnumNode => $node->getPermissibleValues('|'), | ||
| $node instanceof ScalarNode => 'string|int|float|bool', |
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.
| $node instanceof ScalarNode => 'string|int|float|bool', | |
| $node instanceof ScalarNode => 'string|int|float|bool|null', |
…alexandre-daubois) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Config] Add array-shapes to generated config builders | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT A subset of #58771 Note that only constructors get the array-shape as that's what we're going to need first to achieve what we target in #58771. We could also add array shapes to the generated fluent methods. I left that as an exercise to a future contributor ;) Commits ------- a79297a [Config] Add array-shapes to generated config builders
…ig-builders from config files (nicolas-grekas) This PR was merged into the 7.4 branch. Discussion ---------- [DependencyInjection] Handle returning arrays and config-builders from config files | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT Another step on the path explored in #58771 This allows returning PHP arrays or config-builders from config files. Several styles are supported, some examples below. What this does *not* support is config trees under the `import`, `parameters` or `services` namespaces (that's for another PR). Arrays: ```php return [ 'framework' => [...], ]; ``` ```php return static function () { yield 'framework' => [...]; ]; ``` Config builders (best DX together with #61879): ```php return new FrameworkConfig([...]); ``` ```php return static function () { return new FrameworkConfig([...]); ]; ``` ```php // $env is always available in config files if ('dev' === $env) { return new FrameworkConfig([...]); } ``` ```php return static function (string $env) { if ('dev' === $env) { yield new FrameworkConfig([...]); } }; ``` Commits ------- ff3fbd7 [DependencyInjection] Handle returning arrays and config-builders from config files
…es and routes using PHP arrays that follow the same shape as corresponding yaml files (nicolas-grekas) This PR was merged into the 7.4 branch. Discussion ---------- [DependencyInjection][Routing] Handle declaring services and routes using PHP arrays that follow the same shape as corresponding yaml files | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT Another PR on the path explored in #58771 This PR allows returning plain arrays to configure services and routes, using the exact same shape as yaml files. eg for `routes.php`: ```php return [ 'a' => ['path' => '/a'], 'when@dev' => [ 'x' => ['path' => '/x'], ], ]; ``` and for `services.php`: ```php return [ 'parameters' => [ 'foo' => 'bar', ], 'services' => [ '_defaults' => [ 'public' => true, ], Bar::class => null, 'my_service' => [ 'class' => Bar::class, 'arguments' => ['%foo%'], ], ], ]; ``` The final step will be to add support for explicit array shapes that static analyzers can understand. PR on its way. Commits ------- 0aaeef6 [DependencyInjection][Routing] Handle declaring services and routes using PHP arrays that follow the same shape as corresponding yaml files
|
I think that the task is now complete. Many PRs were merged lately about this topic, so this one isn't necessary anymore. The feature will be shipped in 7.4. Many thanks to everyone involved! 🚀 |
…help writing PHP configs using yaml-like arrays (nicolas-grekas) This PR was merged into the 7.4 branch. Discussion ---------- [DependencyInjection][Routing] Define array-shapes to help writing PHP configs using yaml-like arrays | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT Related to #58771. This PR adds array-shape helpers: An example for `routes.php`: ```php use Symfony\Config\RoutesConfig; return new RoutesConfig([ 'controllers' => [ 'resource' => 'attributes', 'type' => 'tagged_services', ], ]); ``` And one for `services.php`: ```php use App\Bar; use Symfony\Config\ServicesConfig; return new ServicesConfig( defaults: [ 'autowire' => true, 'autoconfigure' => true, ], services: [ 'App\\' => ['resource' => '../src/'] Bar::class => null, 'my_service' => [ 'class' => Bar::class, 'arguments' => ['%foo%'], ], ]); ``` The `ServicesConfig` and `RoutesConfig` wrappers around the arrays are totally optional: the style in #61894 remains possible. Yet, those wrappers provide array-shapes that can be used by static analyzers and IDEs. This also works when nesting behind `when@%env%`: ```php return [ 'when@dev' => new ServicesConfig([...]); ]; ``` Note that as far as IDEs are concerned, autocompletion doesn't work yet for the complex shapes used in `ServicesConfig` - neither in vscode nor in phpstorm /cc `@pronskiy` 🙏 Commits ------- 38306ed [DependencyInjection][Routing] Define array-shapes to help writing PHP configs using yaml-like arrays
This PR adds a new
Configclass to configure a Symfony application:One method exists for each extension, as well as
services(),imports()andparameters().The class is generated at cache warmup, allowing to generate the corresponding phpdoc. Here a sneak peek of what the functions looks like:
This will bring nice IDE autocomplete as well as strong static analysis. Also, you're always one click away from the complete documentation.
Credits to Nicolas, Kevin and Ryan for the idea 🙂