Skip to content

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 28, 2021

Backport of #59716 to release/6.0

/cc @safern

Customer Impact

When we added this support on RC1 we accidentally introduced to bugs which are breaking costumers:
#58852
#58330

Since we are late on the release cycle I think it is better to revert a late change into the release where the costumer requesting the feature hasn't taken a dependency on it rather than trying to fix the bugs and introducing more risk. We can then do this feature on 7.0 considering the bugs that where reported.

Risk

It is a direct revert, so this goes back to how the product was prior to RC1.

@ghost
Copy link

ghost commented Sep 28, 2021

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #59716 to release/6.0

/cc @safern

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@danmoseley
Copy link
Member

Supportive assuming this is a literal revert.

How breaking will this be for code written since the original change? Are there any upstack dependencies eg asp.net? cc @HaoK @vidommet

@HaoK
Copy link
Member

HaoK commented Sep 29, 2021

I don't think that we (asp.net) would have taken a dependency on this as this was mostly intended for xml config arrays right?

@HaoK
Copy link
Member

HaoK commented Sep 29, 2021

For 7.0 when this is revisited, maybe its worth keeping this behavior off by default and allow opt in via a new https://github.com/dotnet/runtime/blob/cb042669bcc1b52be8444e5b865588117310edf1/src/libraries/Microsoft.Extensions.Configuration.Binder/src/BinderOptions.cs property, since the binder definitely tends to have a lot of side effects customers might be relying on

@safern
Copy link
Member

safern commented Sep 29, 2021

That was the original proposal, however on API Review it was rejected for the reasons described here: #57325 (comment)

And yes this was intended for xml arrays that have a single element, because on xml there is no way to express a single item belongs to an array as there is on json or other object notations.

@vidommet
Copy link
Contributor

Supportive assuming this is a literal revert.

How breaking will this be for code written since the original change? Are there any upstack dependencies eg asp.net? cc @HaoK @vidommet

I don't think there are any new major dependencies on this code yet. We have been validating these changes internally but not have taken a full dependency.

@safern
Copy link
Member

safern commented Sep 29, 2021

Supportive assuming this is a literal revert.

Yes this is a literal revert of both commits.

@safern safern added the Servicing-approved Approved for servicing release label Sep 29, 2021
@safern
Copy link
Member

safern commented Sep 29, 2021

@Anipik @danmoseley can I just merge this?

@Anipik Anipik merged commit e013f14 into release/6.0 Sep 29, 2021
@safern safern deleted the backport/pr-59716-to-release/6.0 branch September 29, 2021 19:33
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants