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

#3169 extends servicecollection to update env.WebRootFileProvider #3172

Open
wants to merge 5 commits into
base: dev
from

Conversation

Projects
None yet
4 participants
@ghost
Copy link
Contributor

ghost commented Feb 10, 2019

This updates the env.WebRootFileProvider to include module/theme static resources and resolves issue #3169

It enables use of asp-append-version taghelper when used with asp tag helpers, e.g.
<link rel="stylesheet" type="text/css" href="~/Theme/styles/theme.css" asp-append-version="true">
If this is an appropriate change then I'd like to extend the Orchard Tag Helpers to support asp-append-version

extends servicecollection to update env.WebRootFileProvider to includ…
…e static embedded resource #3169, enabling asp-version taghelper
@sebastienros

This comment has been minimized.

Copy link
Member

sebastienros commented Feb 10, 2019

Looks like this change would also allow us to get rid of the UseStaticFiles call for the main app.

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Feb 11, 2019

Yes, have tested and the UseStaticFiles call is now unnecesary.

What do you think about me updating the Orchard Tag Helpers to use asp-append-version?

@sebastienros

This comment has been minimized.

Copy link
Member

sebastienros commented Feb 11, 2019

Functional tests are failing at setup on a SaaS recipe creating a new Blog tenant, does it work for you?

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 12, 2019

Just after a quick look so not sure ;)

Need also to be checked in a published context (e.g published in a local folder) where static assets are embedded in their related modules / themes assemblies (unless for the app's module).

Because, in a published context, if these embedded assets belong to a module / theme, how they will be found by the env.WebRootFileProvider which is e.g a physical provider.

Maybe we would need a ModuleEmbeddedWebRootFileProvider.

I will take a look more in depth asap.

Update: You removed UseStaticFiles() only in the Mvc.Web project. It is also used in Cms.Web, Nancy.Web, templates projects and so on.

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 12, 2019

Okay, i didn't see that you also init env.WebRootFileProvider = fileProvider;, but here AddStaticFiles() is called for each tenant so maybe not the right place, maybe we have to do something in UseOrchardCore() where we also init env.ContentRootFileProvider, to be continued.

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 12, 2019

  • So, in UseOrchardCore(), just after what is already done on the ContentRootFileProvider.

          env.ContentRootFileProvider = new CompositeFileProvider(
              new ModuleEmbeddedFileProvider(appContext),
              env.ContentRootFileProvider);
    
  • I suggest to do the following on WebRootFileProvider which will be done once for all tenants.

          var fileProviders = new List<IFileProvider>();
    
          fileProviders.Add(new ModuleEmbeddedStaticFileProvider(appContext));
          fileProviders.Add(env.WebRootFileProvider);
    
          if (env.IsDevelopment())
          {
              fileProviders.Insert(0, new ModuleProjectStaticFileProvider(appContext));
          }
    
          env.WebRootFileProvider = new CompositeFileProvider(fileProviders);
    
  • Then in AddStaticFiles() which is called for each tenant we could just do

              options.RequestPath = "";
              options.FileProvider = env.WebRootFileProvider;
    

  • Then yes it seems that we could get rid of UseStaticFiles() at the app level and then for all apps.

  • Moreover, i think that what we do with options.OnPrepareResponse will be now also applied to the physical static files under the app wwwroot folder.

  • Finally, we also UseStaticFiles() under App_Data/Sites/{tenant} when the OC.Tenants.FileProvider feature is enabled. Here we already use options.OnPrepareResponse but here also we would need to adapt the WebRootFileProvider. But that's ok for me as it is for this specific one.

Dean Marcussen
move env.WebRootFileProvider registration to ApplicationBuilderExtens…
…ions, implement use of env.WebRootFileProvider for tenant static assets and media physical file provider. Remove UseStatic files from Nancy & Cms Startup Projects
@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Feb 12, 2019

@jketch Thanks for the suggestions :)

I believe I've implemented as you recommended.

Unfortunately I can't seem to get the Functional Tests running locally. Continually get errors about port 5000 being in use from Kestrel. Any suggestions as the functional tests look really useful? However they have just passed remotely, so great.

I've also implemented both the Tenants.UseStaticFiles() and Media.UseStaticFiles() to update and use the env.WebRootFileProvider and have tested running locally. Tenant creation is fine, media works well.

The only issue I discovered which I think maybe unrelated is calling

Model.ContentItem.Content.Image.Media.Paths[0]

in a Liquid Template returns the correct tenant and media path for the media asset but calling the same in a razor template does not return the correct path - was trying to test whether I could call asp-append-version on a media asset.

However I think this issue is unrelated, as removing my changes continued to cause the missing tenant and media path. I haven't tinkered with the multitenancy options previously, so maybe missing something myself in the razor.

Update: If this works I still need to update the template projects to remove the UseStaticFiles()

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Feb 12, 2019

Further Update: What I've done with the Tenant and Media static files is a terrible idea as it will expose all the assets already in the env.WebRootFileProvider to the StaticFileMiddleware.

I'll review tomorrow, and amend. Probably adding the path to the env.WebRootFileProvider, but only exposing that tenant/media path to the StaticFileMiddleware

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 13, 2019

Okay cool, thanks for your updates.

  • Note: For the Tenants module the related startup configure is called if the OC.Tenants.FileProvider feature is enabled. And just saw that this feature is DefaultTenantOnly, see in Manifest.cs, meaning that we can't enable this feature for any other tenant, i think we need to remove this option.

  • For the Media / Tenants static files, we need that for a given tenant prefix we serve the right file and if the related feature is enabled for this tenant. Here i think that you can still init FileProvider as before and we would just have to add the specific provider to the WebRootFileProvider.

  • But the problem i see is that WebRootFileProvider is defined at the host level, so if many tenants it will become a big composite, and not updated if we disable the feature on a tenant. And there are some race conditions when building multiple tenant pipelines (Configure() is called) concurrently.

  • Maybe 2 custom webroot file providers, added once to the WebRootFileProvider (as we did for other providers). Each provider would need to be aware of the requested tenant and if the related feature is enabled. And then no need to call UseStaticFiles() in Media and Tenants startups.

Maybe in another PR, i will think about this tomorrow.

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Feb 13, 2019

Yes, generally makes sense, I think.

Something like a TenantAwareStaticFileProvider, that gets added in the ApplicationBuilderExtensions, which can manage which files are available based on it's awareness of tenant infrastructure / featuers?

I wonder if Media may still need to call UseStaticFiles() to deal with the RequestPath = "/media" however there maybe a way around that.

This is starting to sound like another PR.

Suggest I revert the changes to Media and Tenants, update template projects, and review options for the multitenanted scenario in another issue?

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 14, 2019

Yes, i think that's what we'll do

But i just started some tests that i would like to finish tomorrow.

  • I confirm that we need to remove the line DefaultTenantOnly = true for the OC.Tenants.FileProvider feature, to make this feature usable by any tenant. Can you remove it in this PR? Thanks.

  • I confirm that for Tenants, just adding the specific file provider to WebRootFileProvider is sufficient without having to change the one passed to UseStaticFiles().

  • For Media we don't need UseStaticFile(), the files are served by the UseImageSharp() middleware which runs before. Then because we prefix the url with /media (not wwwroot), to make it working i needed to use this testing code which removes the last /Media segment.

          var fileProvider = new PhysicalFileProvider(mediaPath.Replace("/Media", ""));
          fileProviders.Add(env.WebRootFileProvider);
    
  • Just as a reminder: Also saw, not related, that our custom img tag helper (not the mvc one) doesn't manage url starting with ~/ to be request.PathBase relative, necessary when e.g under a virtual folder.

It's just for infos because as we saw, not good to update WebRootFileProvider in each tenant pipeline. But understanding what's going on, we will be able to open another issue for Media and Tenants specific static files, before finalizing this PR that fixes the append version issue for Modules / Themes static files.

I will let you know my other findings tomorrow and we will decide what to do.

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 14, 2019

But as i remember through the Media module we also want to manage assets that are not images, so finally Media still need to call UseStaticFiles() ;)

I noticed other issues, the main one being that our custom img helper (when using asset-src) doesn't work under a virtual folder, idem with the MediaUrlFilter liquid filter. But these issues are not related to the append version.

Suggest I revert the changes to Media and Tenants, update template projects, and review options for the multitenanted scenario in another issue?

Okay, then i will approve this PR.

Dean Marcussen
Reverted env.WebRootFileProvider changes to Tenants and Media, Remove…
…d unnecesary calls to UseStaticFiles in Templates & Test project, updated Tenants.Manifest.FileProvider removing DefaultTenantOnly
@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Feb 16, 2019

Have made suggested changes.

Thoughts regarding Media Module and FileProviding.
As we look at managing Media through an alternative FileProvider it may be worth considering moving the Media to a feature, similar to the Tenant Static File Provider feature. This could potentially make it easier to resolve an older issue that's been on my mind when thinking about this #2413 regarding blob storage. There's some work starting in ImageSharp.Web to use blob storage -> https://github.com/SixLabors/ImageSharp.Web/commit/2d784e65f445ff493b28870de8c1097ee648e8d0 and when this works then it may prove easier to integrate with the OrchardBlobStorage if Media Static Assets are a feature that can be disabled when using the OrchardBlobMedia feature, so that the blob storage can be served better than through a static file middleware

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Feb 17, 2019

Okay thanks, i will review it tomorrow, today i worked on #3200 to fix the mapping between media paths / urls if under a virtual folder.

When ImageSharp will be ready, may be you would like to open another issue / PR and work on it ;)

@@ -187,6 +189,7 @@ public override void ConfigureServices(IServiceCollection services)

public override void Configure(IApplicationBuilder app, IRouteBuilder routes, IServiceProvider serviceProvider)
{
var env = serviceProvider.GetRequiredService<IHostingEnvironment>();

This comment has been minimized.

@jtkech

jtkech Feb 17, 2019

Member

Okay, so right now we can also remove this line.

@@ -37,6 +39,7 @@ public class FileProviderStartup : StartupBase

public override void Configure(IApplicationBuilder app, IRouteBuilder routes, IServiceProvider serviceProvider)
{
var env = serviceProvider.GetRequiredService<IHostingEnvironment>();

This comment has been minimized.

@jtkech
@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Feb 18, 2019

Yes, I'm keeping a close eye on ImageSharp developments!

@jtkech

jtkech approved these changes Feb 18, 2019

@deanmarcussen

This comment has been minimized.

Copy link

deanmarcussen commented Mar 15, 2019

@sebastienros is this ok to go in or does it need anything further? would like to start looking at extending the tag helpers to support file-versioning. Apologies for the ghost comments, had to change my github account around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.