Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] GenerateConfig 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
alexandre-daubois wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromalexandre-daubois:generate-array-shapes

Conversation

alexandre-daubois
Copy link
Member

@alexandre-dauboisalexandre-daubois commentedNov 5, 2024•
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

This PR adds a newConfig class to configure a Symfony application:

<?php/** @var \Symfony\Config\Config $config */$config->framework(['secret' =>'abc123',]);$config->parameters(['some_parameter' =>'some_value',]);$config    ->imports(['test.php'])    ->services([        ['id' =>'some.service','class' => \App\Service\ConcreteService::class],    ]);

One method exists for each extension, as well asservices(),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:

<?phpnamespaceSymfony\Config;/** * This trait is automatically generated to help in creating a config. */trait WebProfilerTrait{/**     * @param array{     *     toolbar?: array{ // Profiler toolbar configuration     *         enabled?: bool, // Default: false     *         ajax_replace?: bool, // Replace toolbar on AJAX requests Default: false     *     },     *     intercept_redirects?: bool, // Default: false     *     excluded_ajax_paths?: string|int|float|bool, // Default: "^/((index|app(_[\\w]+)?)\\.php/)?_wdt"     * } $config     */publicfunctionwebProfiler(array$config):static    {$this->webProfilerConfig->configure($config);return$this;    }}

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 🙂

javiereguiluz, shochdoerfer, vinceAmstoutz, COil, welcoMattic, and yceruto reacted with thumbs up emojijaviereguiluz, dunglas, and welcoMattic reacted with hooray emojijaviereguiluz, welcoMattic, OskarStark, and zajca reacted with heart emojiChris53897, javiereguiluz, dunglas, vinceAmstoutz, michaljusiega, welcoMattic, OskarStark, smnandre, and Nialsihg reacted with rocket emoji
@carsonbot

This comment was marked as outdated.

@alexandre-daubois
Copy link
MemberAuthor

Thank you for your review! I'm having a look at it and will soon address your comments

@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@alexandre-daubois
Copy link
MemberAuthor

alexandre-daubois commentedNov 24, 2024•
edited
Loading

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.

dunglas and OskarStark reacted with heart emoji

@nicolas-grekas
Copy link
Member

To anyone willing to see improved support for this updated PHP config format, please upvote this phpstorm issue ;)
https://youtrack.jetbrains.com/issue/WI-79484/ArrayShape-and-PHPDoc-shapes-on-param-doesnt-hint-well

alexandre-daubois and dunglas reacted with rocket emoji

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

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?

alexandre-daubois reacted with eyes emoji
@nicolas-grekas
Copy link
Member

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[', the IDE has a very precise context now.

@alexandre-daubois
Copy link
MemberAuthor

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

@alexandre-daubois
Copy link
MemberAuthor

Comments in array shapes are live in the latest PhpStorm 2025.1 Beta:

image

dunglas reacted with heart emoji

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 24, 2025•
edited
Loading

I'm shaking again this config format to find the best syntax. I'm wondering about how one could define several arrays in the same file. We do so currently when some settings depend on the env - with#[WhenEnv]. Porting these to the new format is going to be not fun at the moment.

What about using a config object with as many methods as needed, and no return value?

<?php/** @var \Symfony\Config\Config $config */;$config->framework([...]);if ('dev' ===$config->env) {$config->framework([...]);}

If we want to keep splitting shapes in separate files (to ease scrolling the shape?) we might generate traits and use them in the generated Config class.

WDYT?

@alexandre-daubois
Copy link
MemberAuthor

Suits me fine, I particularly like the trait part. I definitely think it would beway easier to scroll through shapes if we haveFrameworkConfigTrait,WebProfilerConfigTrait, etc.

What about using a config object with as many methods as needed, and no return value

I agree, just a suggestion on the last bit. What about actually returning$this to chain this way?

$config    ->framework([...])    ->webProfiler([...])    ->security([...]);

fabpot added a commit that referenced this pull requestApr 4, 2025
…bois)This PR was merged into the 7.3 branch.Discussion----------[Config] Add `NodeDefinition::docUrl()`| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITAdding such information would allow extensions and bundles to provide even more info with a documentation "one click away".The primary goal is to use this feature in conjunction with#58771, allowing to dump a ``@see`https://symfony.com/doc/...` right next to the configuration array shape.Commits-------3c7fce2 [Config] Add `NodeDefinition::docUrl()`
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull requestApr 4, 2025
…bois)This PR was merged into the 7.3 branch.Discussion----------[Config] Add `NodeDefinition::docUrl()`| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITAdding such information would allow extensions and bundles to provide even more info with a documentation "one click away".The primary goal is to use this feature in conjunction withsymfony/symfony#58771, allowing to dump a ``@see`https://symfony.com/doc/...` right next to the configuration array shape.Commits-------3c7fce2e326 [Config] Add `NodeDefinition::docUrl()`
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestApr 4, 2025
…bois)This PR was merged into the 7.3 branch.Discussion----------[Config] Add `NodeDefinition::docUrl()`| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITAdding such information would allow extensions and bundles to provide even more info with a documentation "one click away".The primary goal is to use this feature in conjunction withsymfony/symfony#58771, allowing to dump a ``@see`https://symfony.com/doc/...` right next to the configuration array shape.Commits-------3c7fce2e326 [Config] Add `NodeDefinition::docUrl()`
symfony-splitter pushed a commit to symfony/web-profiler-bundle that referenced this pull requestApr 4, 2025
…bois)This PR was merged into the 7.3 branch.Discussion----------[Config] Add `NodeDefinition::docUrl()`| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITAdding such information would allow extensions and bundles to provide even more info with a documentation "one click away".The primary goal is to use this feature in conjunction withsymfony/symfony#58771, allowing to dump a ``@see`https://symfony.com/doc/...` right next to the configuration array shape.Commits-------3c7fce2e326 [Config] Add `NodeDefinition::docUrl()`
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 15, 2025•
edited
Loading

JetBrains did their part, thanks to them! Coming to PhpStorm 2025.1

#[ArrayShape] and PHPDoc shapes on param doesn't hint well : WI-79484

Allow comments in array shape : WI-73436

What about actually returning $this to chain this way?

Why not indeed.

Up to resume work on this topic?

pronskiy and GromNaN reacted with heart emojialexandre-daubois and dunglas reacted with rocket emoji

@alexandre-daubois
Copy link
MemberAuthor

It's a bit of a rush at the moment, but as soon as I have a bit of time to get on with the job, this PR is my priority. 🙂

@alexandre-dauboisalexandre-dauboisforce-pushed thegenerate-array-shapes branch 4 times, most recently from43b5e13 toe394d68CompareApril 18, 2025 13:35
@alexandre-dauboisalexandre-daubois changed the title[FrameworkBundle] Generate configuration functions[FrameworkBundle] GenerateConfig classApr 18, 2025
@alexandre-daubois
Copy link
MemberAuthor

I just pushed the revamped version. PR description updated with the new syntax. After playing with, navigating through the shapes is much simpler now that traits are generated.


private static function generateInlinePhpDocForNode(BaseNode $node): string
{
$hasContent = false;
Copy link
Member

@nicolas-grekasnicolas-grekasApr 18, 2025•
edited
Loading

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

Copy link
MemberAuthor

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 forframework:

     *         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;?

Copy link
MemberAuthor

@alexandre-dauboisalexandre-dauboisApr 18, 2025•
edited
Loading

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.

// the closure forbids access to the private scope in the included file
$load = \Closure::bind(function ($path, $env) use ($container, $loader, $resource, $type) {
$config = class_exists(Config::class) ? new Config() : null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why does the generated Config class have constructor arguments when those are never used?
Also, it'd be nice to add public readonly vars to the $config object to get access to the other tools one could use, especially$config->env (possibly $config->path, container, loader, resource, type, although I'm not sure what they could be used for there - for thoughts.)

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Updated, it now looks like this:

<?phpnamespaceSymfony\Config;useSymfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;useSymfony\Config\FrameworkConfig;useSymfony\Config\SecurityConfig;useSymfony\Config\TwigConfig;useSymfony\Config\WebProfilerConfig;useSymfony\Config\MonologConfig;useSymfony\Config\DebugConfig;/** * This class is automatically generated to help in creating a config. */class Config {use \Symfony\Component\DependencyInjection\Resource\ImportsTrait;use \Symfony\Component\DependencyInjection\Resource\ParametersTrait;use \Symfony\Component\DependencyInjection\Resource\ServicesTrait;use \Symfony\Config\FrameworkTrait;use \Symfony\Config\SecurityTrait;use \Symfony\Config\TwigTrait;use \Symfony\Config\WebProfilerTrait;use \Symfony\Config\MonologTrait;use \Symfony\Config\DebugTrait;private$builders;private$frameworkConfig;private$securityConfig;private$twigConfig;private$webProfilerConfig;private$monologConfig;private$debugConfig;publicfunction__construct(publicreadonly ?string$env)    {$this->frameworkConfig =newFrameworkConfig();$this->securityConfig =newSecurityConfig();$this->twigConfig =newTwigConfig();$this->webProfilerConfig =newWebProfilerConfig();$this->monologConfig =newMonologConfig();$this->debugConfig =newDebugConfig();$this->builders = [$this->frameworkConfig,$this->securityConfig,$this->twigConfig,$this->webProfilerConfig,$this->monologConfig,$this->debugConfig];    }publicfunctiongetBuilders():array    {return$this->builders;    }}

$env is injected. It's always possible to add other ones if needed,$env being the most important to me as well

private array $parameters = [];

/**
* @var array<string, mixed> $parameters

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

isn't mixed too broad?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Narrowed the type a bit toarray<string, array|bool|string|int|float|\UnitEnum|null>

* @var array<array{
* id: string,
* class?: string|int|float|bool,
* }>|array<string, class-string|null> $services

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?

Copy link
MemberAuthor

@alexandre-dauboisalexandre-dauboisApr 19, 2025•
edited
Loading

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?

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

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

@fabpotfabpotfabpot requested changes

@dunglasdunglasdunglas requested changes

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

8 participants
@alexandre-daubois@carsonbot@nicolas-grekas@stof@yceruto@ondrejmirtes@dunglas@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp