search

aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

Change param for top-level IIS and Apache extension methods #98

Closed natemcmaster closed 8 years ago

natemcmaster commented 8 years ago

Change IHostingEnvironment parameter to IFileProvider

Also fixup some xmldocs

cc @Tratcher

natemcmaster commented 8 years ago

As @Tratcher mentioned, we know we want to remove IHostingEnvironment from the .AddApacheModRewrite and .AddIISUrlRewrite extensions, but an adequate replacement is not obvious. Does either of these options look better? Should we leave as is?

cc @DamianEdwards @muratg @davidfowl

option 1 - what's in this PR

// use IFileProvider instead of IHostingEnvironment
var options = new RewriteOptions()
    .AddIISUrlRewrite(hostingEnv.ContentRootFileProvider, "web.config")
    .AddApacheModRewrite(hostingEnv.ContentRootFileProvider, ".htaccess");

option 2 - suggested by @Tratcher 

// set the IFileProvider on RewriteOptions
var options = new RewriteOptions()
    .SetConfigFilePathBase(env.ContentRootPath)
    .AddIISUrlRewrite("web.config")
    .AddApacheModRewrite(".htaccess");

Unresolved question: what is the default value of .SetConfigFileBasePath?

Tratcher commented 8 years ago

(from @davidfowl reading over my shoulder): Number 2 looks cleaner, that's what we do in configuration. Do it.

natemcmaster commented 8 years ago

:up: :date:

natemcmaster commented 8 years ago

Discussed in person with @Tratcher. I'm reverting the PR to represent option 1 (https://github.com/aspnet/BasicMiddleware/pull/98#issuecomment-245120564). The implementation of option2 is introduces state into options that really belongs on a builder API (which we don't have yet and would need to design.)

natemcmaster commented 8 years ago

Now that we've had API review, ping for code review.

Tratcher commented 8 years ago

:shipit: