Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Allow "page" route parameter to be used in Mvc controllers #6704

Closed
wants to merge 33 commits into from

Conversation

pranavkm
Copy link
Contributor

Fixes #6660

pranavkm and others added 30 commits July 10, 2017 11:44
Update FSharp.Core and FSharp.NET.Sdk to latest (#6538)
Ensure IPageApplicationModelProviders are invoked in the sequence of …
* Increase wait timeouts for flaky tests

Fixes #6540
Add empty baselines to suppress api check
Add global filters with the right scope
Remove the meta refresh tag from the layout page.
Add a new line at the end of Home/Index.cshtml
Refactor CORS support out of MVC Core
@@ -10,7 +10,7 @@ public class PageViewLocationExpander : IViewLocationExpander
{
public IEnumerable<string> ExpandViewLocations(ViewLocationExpanderContext context, IEnumerable<string> viewLocations)
{
if (string.IsNullOrEmpty(context.PageName))
if (!(context.ActionContext.ActionDescriptor is PageActionDescriptor) || string.IsNullOrEmpty(context.PageName))
Copy link
Member

Choose a reason for hiding this comment

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

Consider inverting this if to be more readable

@@ -376,5 +376,31 @@ public static IEnumerable<object[]> HtmlHelperLinkGenerationData
// Assert
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
}

[Fact]
public async Task UsingPageRouteParameterInConventionRouteWorks()
Copy link
Member

Choose a reason for hiding this comment

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

conventional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix it

@rynowak
Copy link
Member

rynowak commented Aug 24, 2017

Do these places need to change as well?
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Razor/RazorViewEngine.cs#L244
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Razor/RazorViewEngine.cs#L321

If these already do the right thing, they probably need some comments anyway explaining the subtleties.

@pranavkm
Copy link
Contributor Author

They don't as yet. If we do fix it in the View Engine, would we do the same thing for the inverse case, i.e. not read controller or action when working with a page?

@rynowak
Copy link
Member

rynowak commented Aug 24, 2017

Maybe....

@pranavkm
Copy link
Contributor Author

The layering makes it tricky to determine if you're working with a PageActionDescriptor. I guess we could solve it using a contract IRequiredRouteValueProvider but for the 2.0.1 release, the smallest amount of code we could write would be to test if we're not in a ControllerActionDescriptor

@rynowak
Copy link
Member

rynowak commented Aug 24, 2017

I would look at ActionDescriptor.RouteValues for the presence of page vs controller.

@pranavkm
Copy link
Contributor Author

🆙 📅

@@ -240,7 +240,13 @@ private ViewLocationCacheResult LocatePageFromPath(string executingFilePath, str
{
var controllerName = GetNormalizedRouteValue(actionContext, ControllerKey);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

leave this alone for 2.0.1?

if (actionContext.ActionDescriptor.RouteValues.ContainsKey(PageKey))
{
// Only calculate the Razor Page name if "page" is registered in RouteValues.
razorPageName = GetNormalizedRouteValue(actionContext, PageKey);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally I'd like to change how GetNormalizedRouteValue works to verify it returns a value only if it's present in RouteValues but that would be too wide a change for 2.0.1. We could consider it for 2.1.0 though.

Copy link
Member

Choose a reason for hiding this comment

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

Sure

@pranavkm pranavkm changed the base branch from dev to rel/2.0.1 August 24, 2017 21:34
@pranavkm pranavkm closed this Aug 24, 2017
@dougbu dougbu deleted the prkrishn/page-expander branch July 15, 2018 03:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants