Skip to content

[Config] Add StringNode #58428

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 1 commit into from
Oct 23, 2024
Merged

[Config] Add StringNode #58428

merged 1 commit into from
Oct 23, 2024

Conversation

raffaelecarelle
Copy link
Contributor

@raffaelecarelle raffaelecarelle commented Oct 1, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #57660
License MIT

Introduce the StringNode class to handle string values within the configuration tree. Added validation for incorrect types and complemented it with unit tests to verify both valid and invalid scenarios.

Introduce the StringNode class to handle string values within the configuration tree.

@carsonbot carsonbot added this to the 7.2 milestone Oct 1, 2024
@OskarStark OskarStark changed the title [Config] add StringNode [Config] Add StringNode Oct 1, 2024
nicolas-grekas
nicolas-grekas approved these changes Oct 3, 2024
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.

Works for me

src/Symfony/Component/Config/Tests/Definition/StringNodeTest.php Outdated
class StringNodeTest extends TestCase
{
/**
* @dataProvider getValidValues
Copy link
Member

Choose a reason for hiding this comment

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

data providers could be replaced by @testWith annotation in this class

Copy link
Member

Choose a reason for hiding this comment

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

this would create more work when migrating to PHPUnit 11, so not sure it is worth it.

Copy link
Member

Choose a reason for hiding this comment

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

This will be automated, #[TestWith] FTW.

stof
stof requested changes Oct 3, 2024
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

This misses the corresponding node builders to make it usable easily.

@raffaelecarelle
Copy link
Contributor Author

Hi @stof

added StringNodeDefinition. Thank you.

@raffaelecarelle raffaelecarelle requested a review from stof October 3, 2024 10:18
@stof
Copy link
Member

stof commented Oct 3, 2024

@raffaelecarelle you should also add support for it in NodeBuilder (and would be good to add the stringNode shortcut in NodeBuilder and stringPrototype in ArrayNodeDefinition to have the same level of support than other core node definitions)

@raffaelecarelle raffaelecarelle force-pushed the 7.2-Config-add-string-node branch 2 times, most recently from 2f0bf87 to eec0d46 Compare October 3, 2024 11:42
@raffaelecarelle
Copy link
Contributor Author

@raffaelecarelle you should also add support for it in NodeBuilder (and would be good to add the stringNode shortcut in NodeBuilder and stringPrototype in ArrayNodeDefinition to have the same level of support than other core node definitions)

@stof sorry, here we go

@raffaelecarelle raffaelecarelle requested a review from stof October 3, 2024 12:10
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 9, 2024
Merged
nicolas-grekas
nicolas-grekas approved these changes Oct 9, 2024
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.

LGTM with minor comments

@raffaelecarelle raffaelecarelle force-pushed the 7.2-Config-add-string-node branch from 520552b to f69bf34 Compare October 11, 2024 06:43
@nicolas-grekas nicolas-grekas force-pushed the 7.2-Config-add-string-node branch from 2c9b18f to 88b53a0 Compare October 23, 2024 15:09
@nicolas-grekas
Copy link
Member

Thank you @raffaelecarelle.

@nicolas-grekas nicolas-grekas merged commit 05dedbf into symfony:7.2 Oct 23, 2024
7 of 8 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 23, 2024
Closed
@fabpot fabpot mentioned this pull request Oct 27, 2024
Merged
@raffaelecarelle raffaelecarelle deleted the 7.2-Config-add-string-node branch November 20, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof Awaiting requested review from stof

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Config Feature Status: Needs Review
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

[Config] Add support for stringNode in config component
4 participants
@raffaelecarelle @stof @nicolas-grekas @carsonbot