Skip to content

[Config] Allow scalar configuration in PHP Configuration #46328

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 17, 2022
Merged

[Config] Allow scalar configuration in PHP Configuration #46328

merged 2 commits into from
May 17, 2022

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented May 11, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix symfony/monolog-bundle#417 (comment)
License MIT
Doc PR -

Fixes passing scalar values to array nodes that have a beforeNormalization hook, eg:

return static function (MonologConfig $config): void {
    $config->handler('console')
        // ...
        ->processPsr3Messages()
            ->enabled(true)
    ;
};

can be shortened thanks to a beforeNormalization hook:

return static function (MonologConfig $config): void {
    $config->handler('console')
        // ...
        ->processPsr3Messages(true)
    ;
};

I've used some of the code from #44166 by @jderusse. Since his PR is a feature it can't go on 5.4, but it still helped a lot.

@carsonbot carsonbot added this to the 5.4 milestone May 11, 2022
@HypeMC HypeMC mentioned this pull request May 11, 2022
Merged
nicolas-grekas
nicolas-grekas reviewed May 14, 2022
View reviewed changes
Copy link
Member

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

Is there a way to do this only when the node actually has a beforeNormalization? Unless I missed something, right now this will always generate the adaptative code while it might not be needed when beforeNormalization isn't defined.

@HypeMC
Copy link
Contributor Author

HypeMC commented May 14, 2022

Is there a way to do this only when the node actually has a beforeNormalization? Unless I missed something, right now this will always generate the adaptative code while it might not be needed when beforeNormalization isn't defined.

@nicolas-grekas As far as I can tell there's no way to currently know if there are any $normalizationClosures set or not, a hasNormalizationClosures method could be added to the BaseNode, but that would be a new feature if I'm not mistaking.
Another way would be by using the reflection api.

@nicolas-grekas
Copy link
Member

Let's do it by reflection then: that's within the same package, that's fine.

@HypeMC HypeMC force-pushed the scalars-in-config-builder branch from 66a8969 to be1071b Compare May 15, 2022 17:16
@HypeMC
Copy link
Contributor Author

HypeMC commented May 15, 2022

Let's do it by reflection then: that's within the same package, that's fine.

@nicolas-grekas The adaptive code is now conditionally generated depending on whether there are normalization closures set or not. I added a new method to the Builder/Property class, I think this shouldn't be a problem since it's an internal class, but even if it is a problem, the code can easily be moved to a private method in ConfigBuilderGenerator, it's just a little cleaner this way.

nicolas-grekas
nicolas-grekas approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Good one, thank you!

@nicolas-grekas nicolas-grekas force-pushed the scalars-in-config-builder branch from 9d16892 to 2d81a3a Compare May 17, 2022 10:40
@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit 56c6dc1 into symfony:5.4 May 17, 2022
@nicolas-grekas
Copy link
Member

And thank you @jderusse also!

@HypeMC HypeMC deleted the scalars-in-config-builder branch May 17, 2022 10:41
@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 17, 2022 •
edited
Loading

I've merged this up into 6.0 but there is a failing test:

  1. Symfony\Component\Config\Tests\Builder\GeneratedConfigTest::testConfig with data set "ScalarNormalizedTypes" ('ScalarNormalizedTypes', 'scalar_normalized_types')
    TypeError: Symfony\Config\ScalarNormalizedTypesConfig::simpleArray(): Argument #1 ($value) must be of type Symfony\Component\Config\Loader\ParamConfigurator|array, string given, called in src/Symfony/Component/Config/Tests/Builder/Fixtures/ScalarNormalizedTypes.config.php on line 16

Could you please have a look and send a fix? 🙏

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 17, 2022 •
edited
Loading

I'd be also happy to let you merge 6.0 into 6.1 if you're up to 😬 (you can do it on your fork and let me know the commit hash here.)

@HypeMC
Copy link
Contributor Author

HypeMC commented May 17, 2022

@nicolas-grekas Sure, I'll take a look.

@HypeMC HypeMC mentioned this pull request May 17, 2022
Merged
nicolas-grekas added a commit that referenced this pull request May 17, 2022
@nicolas-grekas
minor #46374 [Config] Fix PHP Configuration type hints & tests (HypeMC)
154637b 
This PR was merged into the 6.0 branch.

Discussion
----------

[Config] Fix PHP Configuration type hints & tests

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46328 (comment)
| License       | MIT
| Doc PR        | -

Followup to #46328

Commits
-------

9d38089 [Config] Fix PHP Configuration type hints & tests
@HypeMC
Copy link
Contributor Author

HypeMC commented May 17, 2022

I'd be also happy to let you merge 6.0 into 6.1 if you're up to grimacing (you can do it on your fork and let me know the commit hash here.)

@nicolas-grekas Done HypeMC@123b165

@nicolas-grekas
Copy link
Member

Great thank you so much! Now into 6.1.

@nicolas-grekas nicolas-grekas mentioned this pull request May 17, 2022
Merged
@fabpot fabpot mentioned this pull request May 27, 2022
Merged
This was referenced May 27, 2022
Merged
Merged
@wouterj wouterj mentioned this pull request Jun 1, 2022
Closed
nicolas-grekas added a commit that referenced this pull request Oct 24, 2022
@nicolas-grekas
feature #44166 [Config] Use better typehint in PHP Configuration (jde…
8321162 
…russe)

This PR was merged into the 6.2 branch.

Discussion
----------

[Config] Use better typehint in PHP Configuration

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This PR now followup #46328 to replace the generic `mixed` type-hint by type inferred from normalizations (ie, if a config use `ifString`, when now that the normalization works only for string, these `mixed` can safely be replaced by `string`)

Commits
-------

0e590dc Allow scalar configuration in PHP Configuration
@alexndlm alexndlm mentioned this pull request Sep 4, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Config Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@HypeMC @nicolas-grekas @carsonbot @jderusse