Skip to content

[Config] Handle ignoreExtraKeys in config builder #40987

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
Aug 26, 2021
Merged

[Config] Handle ignoreExtraKeys in config builder #40987

merged 1 commit into from
Aug 26, 2021

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Apr 29, 2021 •
edited
Loading

Q A
Branch? 5.x
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Currently the config builder doesn't handle the ignoreExtraKeys option. This is an attempt to fix that.

TODO

  • Tests, but I was hoping for some initial feedback first

@nicolas-grekas nicolas-grekas added this to the 5.3 milestone Apr 29, 2021
@Nyholm
Copy link
Member

Nyholm commented Apr 29, 2021 •
edited
Loading

Thank you.

Could you elaborate on the use case for this? Could you give an example of a public bundle that is using this?

Nyholm
Nyholm reviewed Apr 29, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

I did not know of this feature. Or at least it does not ring a bell right now.

I am happy with the implementation. Just some details. I would like to know a public bundle that is using this because it will be easy to test and review the generated output.

@Nyholm
Copy link
Member

Nyholm commented May 3, 2021 •
edited
Loading

Thank you for the update. Could you please answer me to this:

Could you give an example of a public bundle that is using this?

I would like to know a public bundle that is using this because it will be easy to test and review the generated output.

@HypeMC
Copy link
Contributor Author

HypeMC commented May 3, 2021 •
edited
Loading

Thank you.

Could you elaborate on the use case for this? Could you give an example of a public bundle that is using this?

I've went through some popular bundles & none of them use it, so it I'd say it not a feature often used. I've used it only once when I wanted to allow custom options to be passed to a service.

Nyholm
Nyholm reviewed May 21, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

I only have one thing that I would like to change.

@HypeMC HypeMC changed the base branch from 5.4 to 5.3 May 21, 2021 16:58
@ciaranmcnulty
Copy link
Contributor

@Nyholm It looks like SwiftMailerBundle does https://github.com/symfony/swiftmailer-bundle/blob/main/DependencyInjection/Configuration.php#L105

@Nyholm Nyholm changed the base branch from 5.3 to 5.4 June 6, 2021 06:49
@Nyholm Nyholm modified the milestones: 5.3, 5.4 Jun 6, 2021
Nyholm
Nyholm requested changes Jun 6, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

Im happy with the implementation.
I've changed the target branch. I've also added a few comments about the actual method on the Config class.

Nyholm
Nyholm approved these changes Jun 7, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@Nyholm
Copy link
Member

Nyholm commented Jun 7, 2021 •
edited
Loading

Im not sure why Travis didn't run...

@fabpot
Copy link
Member

fabpot commented Aug 26, 2021

Thank you @HypeMC.

@fabpot fabpot merged commit b975e4c into symfony:5.4 Aug 26, 2021
@HypeMC HypeMC deleted the config-ignoreextrakeys branch August 26, 2021 15:17
This was referenced Nov 5, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@Nyholm Nyholm Nyholm 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.
6 participants
@HypeMC @Nyholm @ciaranmcnulty @fabpot @nicolas-grekas @carsonbot