Skip to content
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.

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

[release/7.0] Prefer TypeConverter over TryParse #46687

Merged
merged 3 commits into from
Mar 13, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 15, 2023

Backport of #46577 to release/7.0

/cc @brunolins16

Prefer TypeConverter over TryParse

This PR reverts a break change introduced in .NET 7 where the MVC Model Binding prefers a TryParse method over a TypeConverter for a simple type.

Fixes #46384
Fixes #45338

Customer Impact

In .NET 7 was added a new Model Binder to enable binding from a string using a known TryParse method, in addition to a TypeConverter, however, was decided to register the new Model Binder to be executed first (preferring TryParse over TypeConverter) while all the functionality still in place and users still able to use a TypeConverter there are situations where a type already have a TryParse method and a TypeConverter and they have different default options (#45338), or when a user is trying to create a custom TypeConverter to these types (#46384).

This PR keeps the new Model Binder, however, will maintain compatibly with .NET 6 by preferring TypeConverter over a TryParse method.

Regression?

  • Yes
  • No

The regression was introduced in .NET when the new TryParseModelBinder was added.

Risk

  • High
  • Medium
  • Low

All the new functionality added in .NET 7 (Bind withTryParse) will keep working, however, we are making it more compatible with what we have before.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@github-actions github-actions bot requested a review from a team as a code owner February 15, 2023 19:07
@ghost ghost added this to the 7.0.x milestone Feb 15, 2023
@ghost
Copy link

ghost commented Feb 15, 2023

Hi @github-actions[bot]. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

Copy link
Member

@Nick-Stanton Nick-Stanton left a comment

Choose a reason for hiding this comment

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

I was curious about this when we were looking at individual DateOnly and TimeOnly model binders. I'll leave the official approving review to someone more knowledgeable, but this fix looks great to me

@brunolins16
Copy link
Member

someone more knowledgeable, but this fix looks great to me

Yeah, when we added the TryParseModelBinder was decided to prefer it over the TypeConverter but we got few feedbacks already that it was a bad decision since our recommendation is create a TypeConverter when bind from string.

https://learn.microsoft.com/en-us/aspnet/core/mvc/advanced/custom-model-binding?view=aspnetcore-6.0#recommendations-and-best-practices

@brunolins16 brunolins16 added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Feb 15, 2023
@brunolins16 brunolins16 requested a review from a team February 15, 2023 22:13
@brunolins16 brunolins16 linked an issue Feb 15, 2023 that may be closed by this pull request
1 task
@brunolins16 brunolins16 added the Servicing-consider Shiproom approval is required for the issue label Feb 24, 2023
@ghost
Copy link

ghost commented Feb 24, 2023

Hi @github-actions[bot]. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@ghost
Copy link

ghost commented Mar 4, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 4, 2023
@captainsafia
Copy link
Member

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 8, 2023
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@captainsafia captainsafia added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Mar 13, 2023
@ghost
Copy link

ghost commented Mar 13, 2023

Hi @github-actions[bot]. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@captainsafia
Copy link
Member

Approved via email.

@dotnet/aspnet-build Can I get help merging?

@wtgodbe wtgodbe merged commit 254fce0 into release/7.0 Mar 13, 2023
@wtgodbe wtgodbe deleted the backport/pr-46577-to-release/7.0 branch March 13, 2023 20:25
@ghost ghost modified the milestones: 7.0.x, 7.0.5 Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateTimeOffset modelbinding from form is always set to utc in net7
5 participants