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

Update default Razor search paths to include ~/[PagesRoot]/Shared #6783

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ public class RazorViewEngineOptions
/// /Pages/Account/{0}.cshtml
/// /Pages/{0}.cshtml
/// /Views/Shared/{0}.cshtml
/// /Pages/Shared/{0}.cshtml
/// </para>
/// </remarks>
public IList<string> PageViewLocationFormats { get; } = new List<string>();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;
using Microsoft.AspNetCore.Mvc.Razor;
using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure;
Expand All @@ -10,29 +11,45 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
{
public class RazorPagesRazorViewEngineOptionsSetup : IConfigureOptions<RazorViewEngineOptions>
{
private readonly IOptions<RazorPagesOptions> _pagesOptions;
private readonly RazorPagesOptions _pagesOptions;

public RazorPagesRazorViewEngineOptionsSetup(IOptions<RazorPagesOptions> pagesOptions)
{
_pagesOptions = pagesOptions;
_pagesOptions = pagesOptions?.Value ?? throw new ArgumentNullException(nameof(pagesOptions));
}

public void Configure(RazorViewEngineOptions options)
{
Debug.Assert(_pagesOptions.Value.RootDirectory.Length > 0);

if (_pagesOptions.Value.RootDirectory == "/")
{
options.PageViewLocationFormats.Add("/{1}/{0}" + RazorViewEngine.ViewExtension);
}
else
if (options == null)
{
options.PageViewLocationFormats.Add(_pagesOptions.Value.RootDirectory + "/{1}/{0}" + RazorViewEngine.ViewExtension);
throw new ArgumentNullException(nameof(options));
}

var rootDirectory = _pagesOptions.RootDirectory;
Debug.Assert(!string.IsNullOrEmpty(rootDirectory));
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't be asserts, they are clearly possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We disallow these in the property setter (https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptions.cs#L32), but sure, we can throw for extra goodness.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok, unimportant then

var defaultPageSearchPath = CombinePath(rootDirectory, "{1}/{0}");
options.PageViewLocationFormats.Add(defaultPageSearchPath);

// /Pages/Shared/{0}.cshtml
var pagesSharedDirectory = CombinePath(rootDirectory, "Shared/{0}");
options.PageViewLocationFormats.Add(pagesSharedDirectory);

options.PageViewLocationFormats.Add("/Views/Shared/{0}" + RazorViewEngine.ViewExtension);

options.ViewLocationFormats.Add(pagesSharedDirectory);
options.AreaViewLocationFormats.Add(pagesSharedDirectory);
Copy link
Member

Choose a reason for hiding this comment

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

Wait so we're making pages/shared searchable from views too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's how @DamianEdwards wants this. The intent is to allow folks to migrate from Views -> Pages while keeping things working.


options.ViewLocationExpanders.Add(new PageViewLocationExpander());
}

private static string CombinePath(string path1, string path2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Path.Combine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to preserve path separators (they are all meant to be forward slashes). Path.Combine canonicalizes paths to use OS dependent path separators.

{
if (path1.EndsWith("/", StringComparison.Ordinal))
{
return path1 + path2 + RazorViewEngine.ViewExtension;
}

return path1 + "/" + path2 + RazorViewEngine.ViewExtension;
Copy link
Member

Choose a reason for hiding this comment

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

since path1 is always _pagesOptions.RootDirectory do we guarantee the absence of a trailing slash? or do we allow that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what the check in line #47 does, no?

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -274,5 +274,18 @@ public async Task PageRoute_UsingDefaultPageNameToRoute()
// Assert
Assert.Equal(expected, response.Trim());
}

[Fact]
public async Task Pages_ReturnsFromPagesSharedDirectory()
{
// Arrange
var expected = "Hello from Pages/Shared";

// Act
var response = await Client.GetStringAsync("/SearchInPages");

// Assert
Assert.Equal(expected, response.Trim());
}
}
}
13 changes: 13 additions & 0 deletions test/Microsoft.AspNetCore.Mvc.FunctionalTests/ViewEngineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -513,5 +513,18 @@ public async Task ViewEngine_ResolvesPathsWithSlashesThatDoNotHaveExtensions()
// Assert
Assert.Equal(expected, responseContent.Trim());
}

[Fact]
public async Task ViewEngine_DiscoversViewsFromPagesSharedDirectory()
{
// Arrange
var expected = "Hello from Pages/Shared";

// Act
var responseContent = await Client.GetStringAsync("/ViewEngine/SearchInPages");

// Assert
Assert.Equal(expected, responseContent.Trim());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1990,6 +1990,7 @@ private static IOptions<RazorViewEngineOptions> GetOptionsAccessor(
var options = new RazorViewEngineOptions();
optionsSetup.Configure(options);
options.PageViewLocationFormats.Add("/Views/Shared/{0}.cshtml");
options.PageViewLocationFormats.Add("/Pages/Shared/{0}.cshtml");

if (expanders != null)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc.Razor;
using Microsoft.AspNetCore.Mvc.Razor.Internal;
using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure;
using Moq;
using Xunit;

namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
{
public class RazorPagesRazorViewEngineOptionsSetupTest
Copy link
Member

Choose a reason for hiding this comment

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

Needs two functional tests. (View -> Pages/Shared), (Page -> Pages/Shared)

I don't think there's anywhere else in the system we unit test options setups 😆

{
[Fact]
public void Configure_AddsPageViewLocationFormats_WhenPagesRootIsAppRoot()
{
// Arrange
var expected = new[]
{
"/{1}/{0}.cshtml",
"/Shared/{0}.cshtml",
"/Views/Shared/{0}.cshtml",
};

var razorPagesOptions = new RazorPagesOptions
{
RootDirectory = "/"
};
var viewEngineOptions = GetViewEngineOptions();
var setup = new RazorPagesRazorViewEngineOptionsSetup(
new TestOptionsManager<RazorPagesOptions>(razorPagesOptions));

// Act
setup.Configure(viewEngineOptions);

// Assert
Assert.Equal(expected, viewEngineOptions.PageViewLocationFormats);
}

[Fact]
public void Configure_AddsPageViewLocationFormats_WithDefaultPagesRoot()
{
// Arrange
var expected = new[]
{
"/Pages/{1}/{0}.cshtml",
"/Pages/Shared/{0}.cshtml",
"/Views/Shared/{0}.cshtml",
};

var razorPagesOptions = new RazorPagesOptions();
var viewEngineOptions = GetViewEngineOptions();
var setup = new RazorPagesRazorViewEngineOptionsSetup(
new TestOptionsManager<RazorPagesOptions>(razorPagesOptions));

// Act
setup.Configure(viewEngineOptions);

// Assert
Assert.Equal(expected, viewEngineOptions.PageViewLocationFormats);
}

[Fact]
public void Configure_AddsSharedPagesDirectoryToViewLocationFormats()
{
// Arrange
var expected = new[]
{
"/Views/{1}/{0}.cshtml",
"/Views/Shared/{0}.cshtml",
"/PagesRoot/Shared/{0}.cshtml",
};

var razorPagesOptions = new RazorPagesOptions
{
RootDirectory = "/PagesRoot",
};
var viewEngineOptions = GetViewEngineOptions();
var setup = new RazorPagesRazorViewEngineOptionsSetup(
new TestOptionsManager<RazorPagesOptions>(razorPagesOptions));

// Act
setup.Configure(viewEngineOptions);

// Assert
Assert.Equal(expected, viewEngineOptions.ViewLocationFormats);
}

[Fact]
public void Configure_AddsSharedPagesDirectoryToAreaViewLocationFormats()
{
// Arrange
var expected = new[]
{
"/Areas/{2}/Views/{1}/{0}.cshtml",
"/Areas/{2}/Views/Shared/{0}.cshtml",
"/Views/Shared/{0}.cshtml",
"/PagesRoot/Shared/{0}.cshtml",
};

var razorPagesOptions = new RazorPagesOptions
{
RootDirectory = "/PagesRoot",
};
var viewEngineOptions = GetViewEngineOptions();
var setup = new RazorPagesRazorViewEngineOptionsSetup(
new TestOptionsManager<RazorPagesOptions>(razorPagesOptions));

// Act
setup.Configure(viewEngineOptions);

// Assert
Assert.Equal(expected, viewEngineOptions.AreaViewLocationFormats);
}

[Fact]
public void Configure_RegistersPageViewLocationExpander()
{
// Arrange
var viewEngineOptions = GetViewEngineOptions();
var setup = new RazorPagesRazorViewEngineOptionsSetup(new TestOptionsManager<RazorPagesOptions>());

// Act
setup.Configure(viewEngineOptions);

// Assert
Assert.Collection(
viewEngineOptions.ViewLocationExpanders,
expander => Assert.IsType<PageViewLocationExpander>(expander));
}

private static RazorViewEngineOptions GetViewEngineOptions()
{
var defaultSetup = new RazorViewEngineOptionsSetup(Mock.Of<IHostingEnvironment>());
var options = new RazorViewEngineOptions();
defaultSetup.Configure(options);

return options;
}
}
}
1 change: 1 addition & 0 deletions test/WebSites/RazorPagesWebSite/Pages/FileFromShared
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@Html.Partial("_FileInShared)
2 changes: 2 additions & 0 deletions test/WebSites/RazorPagesWebSite/Pages/SearchInPages.cshtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@page
@Html.Partial("_FileInShared")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello from Pages/Shared
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@{ throw new Exception("This file should not be processed"); }
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,7 @@ public IActionResult ViewWithComponentThatHasViewStart()
{
return View();
}

public IActionResult SearchInPages() => View();
}
}
1 change: 1 addition & 0 deletions test/WebSites/RazorWebSite/Pages/Shared/_Layout.cshtml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@{ throw new Exception("This file should not be processed"); }
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello from Pages/Shared