Skip to content

[MVC] Remove obsolete APIs #7374

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

Merged
merged 1 commit into from
Feb 11, 2019
Merged

[MVC] Remove obsolete APIs #7374

merged 1 commit into from
Feb 11, 2019

Conversation

NTaylorMullen
Copy link

  • Removed ViewAttribute Property
  • Removed RazorViewAttribute
  • Removed ViewsFeatureProvider
  • Removed PageArgumentBinder and its internal implementation DefaultPageArgumentBinder.
  • Removed RazorPageAttribute
  • Removed corresponding test classes/methods for all the above.
  • Reacted to class/member changes in dependencies.

Addresses #7326

/// An <see cref="IApplicationFeatureProvider{TFeature}"/> for <see cref="ViewsFeature"/>.
/// </summary>
[Obsolete("This type is obsolete and will be removed in a future version. See " + nameof(IRazorCompiledItemProvider) + " for alternatives.")]
public class ViewsFeatureProvider : IApplicationFeatureProvider<ViewsFeature>
Copy link
Member

Choose a reason for hiding this comment

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

For contrast, this is random old extensibility that is a good thing to remove since it will rarely appear in user-code.

Copy link
Author

Choose a reason for hiding this comment

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

One concern I do have, is if I remove this but don't remove the RazorViewAttribute and a user doesn't heed the obsolete warnings they'll get a hard to resolve, significant behavior change.

Copy link
Contributor

Choose a reason for hiding this comment

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

and a user doesn't heed the obsolete warnings they'll get a hard to resolve

We can add it to the migration notes as something to be aware of. If we get lots of user feedback (I'd think it's fairly unlikely given this attribute comes from Precompilation), we can always change the runtime to throw.

Copy link
Author

Choose a reason for hiding this comment

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

I'm still not convinced since i'd rather break hard or not at all then break soft when it comes to these things (can have migration guidance with either hard or soft breaks). I'll leave it as removed though since you both feel differently 😄

@danroth27 is there a place where we're collecting migration guidance?

Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Once @rynowak's happy, I'm happy

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 8, 2019
@NTaylorMullen
Copy link
Author

🆙 📅

- Removed ViewsFeatureProvider
- Removed PageArgumentBinder and its internal implementation DefaultPageArgumentBinder.
- Removed corresponding test classes/methods for all the above.
- Reacted to class/member changes in dependencies.

#7326
@NTaylorMullen
Copy link
Author

ping

@NTaylorMullen NTaylorMullen merged commit dfddc4e into master Feb 11, 2019
@NTaylorMullen NTaylorMullen deleted the nimullen/7326 branch February 11, 2019 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants