Skip to content

Fix Configuration with IList and ICollection - #77857

Merged
tarekgh merged 3 commits into
dotnet:mainfrom
tarekgh:FixConfigurationWithIList
Nov 9, 2022
Merged

Fix Configuration with IList and ICollection#77857
tarekgh merged 3 commits into
dotnet:mainfrom
tarekgh:FixConfigurationWithIList

Conversation

@tarekgh

@tarekghtarekgh commented Nov 3, 2022

Copy link
Copy Markdown
Member

#77725

In .NET 7.0 we had some code refactoring and fixes in configuration. This change had been handling the binding to ICollction<T> and IList<T> by creating underlying Array<T> object. It is obvious the returned object is not expandable which means users cannot add more items to that object. This is wrong and regression from previous versions of .NET/Core. The fix here is in case of binding to ICollction<T> and IList<T>, we'll create List<T> object instead which is expandable and allow adding more items to such object.

#77725 is a good example demonstrating this problem.

@ghost

ghost commented Nov 3, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

#77725

Author:tarekgh
Assignees:tarekgh
Labels:

area-Extensions-Configuration

Milestone:-

@tarekgh

Copy link
Copy Markdown
MemberAuthor

CC @SteveDunn@ericstj

@tarekgh

Copy link
Copy Markdown
MemberAuthor

#68133
#73267

@tarekghtarekgh added this to the 8.0.0 milestone Nov 3, 2022

@SteveDunnSteveDunn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tarekgh

Copy link
Copy Markdown
MemberAuthor

#77582

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this @tarekgh!

@tarekgh

Copy link
Copy Markdown
MemberAuthor

All failures in CI are not related. merging it.

@tarekgh
tarekgh merged commit 63511ba into dotnet:mainNov 9, 2022
@tarekgh
tarekgh deleted the FixConfigurationWithIList branch November 9, 2022 00:50
@tarekgh

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3430460958

@github-actions

Copy link
Copy Markdown
Contributor

@tarekgh backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix Configuration with IList and ICollection
Applying: Address the feedback
error: sha1 information is lacking or useless (src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Address the feedback
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions

Copy link
Copy Markdown
Contributor

@tarekgh an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

carlossanlop pushed a commit that referenced this pull request Nov 11, 2022
…> implementer types (#78118)
* Fix Binding with IDictionary<,> implementer types
* Address the feedback
* Feedback addressing
* Fix Configuration with IList and ICollection (#77857)
* Add servicing version to the source project
Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Dec 9, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tarekgh@SteveDunn@maryamariyan@eerhardt