5

Closed

Script.Include in Orchard.Core resolves to current theme scripts folder

description

We have a custom theme (Company.MyTheme) in which we have a Scripts folder.

In Orchard.Core\Shapes\Views\Document.cshtml on row 5, this line..:
Script.Include("html5.js").UseCondition("lt IE 9").AtHead();
...will generate in:
  • Orchard 1.6:
    <!--[if lt IE 9]>
    <script src="/Core/Shapes/Scripts/html5.js" type="text/javascript"></script>
    <![endif]-->
    
    The path resolves correctly to the Scripts folder in the same project, Orchard.Core.
  • Orchard 1.7.1:
    <!--[if lt IE 9]>
    <script src="/Themes/Company.MyTheme/scripts/html5.js" type="text/javascript">
    </script>
    <![endif]-->
    
    The path resolves to the Scripts folder in the current (active) theme, Company.MyTheme
This results in loss of functionality (in eg. IE lower than v9) when html5.js doesn't exist in the custom theme Scripts folder.

I don't know if it's a bug in Orchard, but it is an issue.
Closed Oct 16 at 8:42 PM by sebastienros

comments

CSADNT wrote Nov 14, 2013 at 6:54 PM

For me it is not a bug but the correct way to behave.

AimOrchard wrote Nov 15, 2013 at 6:54 AM

For me this sounds like a breaking change for some?

marioleed wrote Nov 15, 2013 at 8:06 AM

I'm going to agree with @AimOrchard that this would be a breaking change for some.

I can't really see how this would be the correct way to behave, expecting all custom themes to include all necessary scripts that already exists in the Orchard.Core project.

For me, the preferred execution would be:
  1. Try to resolve path to the current theme Scripts folder. (Maybe check if file exists.)
  2. If file not found, fail back to the same project Scripts folder - where the method was used.
One solution could be to either change the current Script.Include() method or create an alternative method for above execution to be used in Core, eg. Script.IncludeCore(), or something.

CSADNT wrote Nov 15, 2013 at 10:11 AM

Why don't use a ResourceManifest in modules and Script.Require ?

BertrandLeRoy wrote Nov 16, 2013 at 6:18 AM

Yes, or fully-qualify the path to the script: ~/Core/Shapes/Scripts/html5.js

marioleed wrote Nov 17, 2013 at 12:47 PM

Yeah, I used the fully qualified path instead.

I guess this is not that big of an issue, so somebody could probably close this issue.

mjy78 wrote Dec 6, 2013 at 3:16 AM

I really think this is a regression as it effects themes that have a base theme. For example, in a multi-tenant scenario, someone might have a TwitterBoostrap base theme and then provide custom themes for each tenant based on the bootstrap theme.

Stylesheet searching behaviour is great in both v1.7 and earlier versions, as it supports a number of scenarios when using a base theme. Consider the scenario where a base theme has a Layout.cshtml view that calls Style.Include("stylesheet.css") with a script located in the BaseTheme\Styles folder...
  1. When another theme is based on this theme, you can simply create a "stylesheet.css" in the child theme's Styles directory and it will be found and used in preference to the base theme's stylesheet.
  2. You can override the view (eg. Layout.cshtml) that has the Script.Include("stylesheet.css") and the stylesheet from the base theme will still be found and used (the stylesheet doesn't need to be replicated to the child theme)
  3. You can override the view as in step 3, and also override the stylesheet.css and it will find and use the child stylesheet.
This behaviour is quite nice because it means that in child themes you only need to override the minimum views and stylesheets and make the minimum changes to these files in order to change the base theme behaviour/style that you need. For example, if you want to override the Layout.cshtml to add a custom zone for a child theme, you don't need to modify the calls to Style.Include("stylesheet.css") in this view to explicitly reference the location of the base theme's stylesheet. It will just be found. You only need to make the bare minimum modifications to the overridden Layout.cshtml file to add your custom zone.

However it seems Script searching behaviour is somewhat lacking even in earlier versions. It's now worse in v1.7.

In earlier versions only points 2 and 3 above worked. If you wanted to override a script in your child theme (as in point 3 above) you would need to also override the view that included it.

In v1.7.2 point 2 above doesn't work either. If you override the view in the child theme, you must either replicate all the scripts that the view refers to in your child theme, or you must change all calls to Script.Include in the view in your child theme to refer to the fully qualified path of the base theme script location.

Is there any reason that the script searching behaviour can't work the same as for stylesheets?

mjy78 wrote Dec 6, 2013 at 3:20 AM

An EDIT to my previous post in the 3rd last paragraph...

In earlier versions only points 2 and 3 above worked. If you wanted to override a script in your child theme (as in point 1 above) you would need to also override the view that included it.

mjy78 wrote Dec 6, 2013 at 4:06 AM

Sorry. Jumped the gun with my comment regarding earlier versions. It appears v1.6 scripts work nicely just the way Stylesheets do.

Scripts not behaving nicely in v1.7 though :(

mjy78 wrote Dec 6, 2013 at 5:32 AM

Ok. I found that the difference between stylesheet behaviour and javascript behaviour is that for stylesheets there is a StyleSheetBindingStrategy class that discovers css files resulting in the appropriate stylesheets being found when overriding views or stylesheets in child themes.

I created a JavascriptBindingStrategy class that enables javascript discovery the same way. Seems to work nicely and I tested it with the 3 scenarios I mentioned in my previous comment and it works nicely. Is there any reason this couldn't be added into the core as the solution?

Class is below....
    // discovers .js files and turns them into Script__<filename> shapes.
    public class JavascriptBindingStrategy : StaticFileBindingStrategy, IShapeTableProvider {
        public JavascriptBindingStrategy(IExtensionManager extensionManager, ShellDescriptor shellDescriptor, IVirtualPathProvider virtualPathProvider) : base(extensionManager, shellDescriptor, virtualPathProvider) {
        }

        public override string GetFileExtension() {
            return ".js";
        }

        public override string GetFolder() {
            return "Scripts";
        }

        public override string GetShapePrefix() {
            return "Script__";
        }
    }

sebastienros wrote Dec 6, 2013 at 6:08 PM

This is well known, if you look into the commit history you will find it to be added, then removed in 1.7 because it caused more pain than good. Look at the issue items attached to the commits. If you find a solution we'll include it.

mjy78 wrote Dec 7, 2013 at 2:51 AM

It seems that the pain caused is around resources defined in manifests and required in views via Script.Require or Style.Require. It also seems the fallback behaviour we are after is relevant only when using Script.Include or Style.Include.

So I wonder if there is a way that we can differentiate the two? Could we change the discovery mechanism within the StaticFileBindingStrategy to only build shapes for files that are not added to a resource manifest (ie. only those that are defined inline)?

mjy78 wrote Dec 7, 2013 at 4:33 AM

How about something like below? This seems to resolve the issue of CDN usage with scripts and css defined in resource manifests, while still allowing scripts and css to be found in parent module/themes when "included" in a view.

It's a bit rough, just a proof of concept really. There's probably a neater way to get the manifests from within the StaticFileBindingStrategy class. I had problems passing through IResourceManager directly due to it being a per request instance whereas the binding strategy class is singleton, so I just duplicated the logic from the resource manager.

Diff the file below against v1.7.2 of Orchard\DisplayManagement\Descriptors\ResourceBindingStrategy\StylesheetBindingStrategy.cs to see the changes I made...
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using Orchard.Environment.Descriptor.Models;
using Orchard.Environment.Extensions;
using Orchard.Environment.Extensions.Models;
using Orchard.FileSystems.VirtualPath;
using Orchard.UI.Resources;
using Orchard.Utility.Extensions;
using Autofac.Features.Metadata;

namespace Orchard.DisplayManagement.Descriptors.ResourceBindingStrategy {
    // discovers static files and turns them into shapes.
    public abstract class StaticFileBindingStrategy {
        private readonly IEnumerable<Meta<IResourceManifestProvider>> _resourceProviders;
        private readonly IExtensionManager _extensionManager;
        private readonly ShellDescriptor _shellDescriptor;
        private readonly IVirtualPathProvider _virtualPathProvider;
        private static readonly char[] unsafeCharList = "/:?#[]@!&'()*+,;=\r\n\t\f\" <>.-_".ToCharArray();

        protected StaticFileBindingStrategy(IEnumerable<Meta<IResourceManifestProvider>> resourceProviders, IExtensionManager extensionManager, ShellDescriptor shellDescriptor, IVirtualPathProvider virtualPathProvider) {
            _resourceProviders = resourceProviders;
            _extensionManager = extensionManager;
            _shellDescriptor = shellDescriptor;
            _virtualPathProvider = virtualPathProvider;
        }

        public abstract string GetResourceType();
        public abstract string GetFileExtension();
        public abstract string GetFolder();
        public abstract string GetShapePrefix();

        private static string SafeName(string name) {
            if (string.IsNullOrWhiteSpace(name))
                return String.Empty;

            return name.Strip(unsafeCharList).ToLowerInvariant();
        }

        public static string GetAlternateShapeNameFromFileName(string fileName) {
            if (fileName == null) {
                throw new ArgumentNullException("fileName");
            }
            string shapeName;
            if (Uri.IsWellFormedUriString(fileName, UriKind.Absolute)) {
                var uri = new Uri(fileName);
                shapeName = uri.Authority + "$" + uri.AbsolutePath + "$" + uri.Query;
            }
            else {
                shapeName = Path.GetFileNameWithoutExtension(fileName);
            }
            return SafeName(shapeName);
        }

        private static IEnumerable<ExtensionDescriptor> Once(IEnumerable<FeatureDescriptor> featureDescriptors) {
            var once = new ConcurrentDictionary<string, object>();
            return featureDescriptors.Select(fd => fd.Extension).Where(ed => once.TryAdd(ed.Id, null)).ToList();
        }

        public void Discover(ShapeTableBuilder builder) {
            var availableFeatures = _extensionManager.AvailableFeatures();
            var activeFeatures = availableFeatures.Where(FeatureIsEnabled);
            var activeExtensions = Once(activeFeatures);

            var hits = activeExtensions.SelectMany(extensionDescriptor => {
                var basePath = Path.Combine(extensionDescriptor.Location, extensionDescriptor.Id).Replace(Path.DirectorySeparatorChar, '/');
                var virtualPath = Path.Combine(basePath, GetFolder()).Replace(Path.DirectorySeparatorChar, '/');
                var shapes = _virtualPathProvider.ListFiles(virtualPath)
                    .Select(Path.GetFileName)
                    .Where(fileName => string.Equals(Path.GetExtension(fileName), GetFileExtension(), StringComparison.OrdinalIgnoreCase))
                    .Select(cssFileName => new {
                        fileName = Path.GetFileNameWithoutExtension(cssFileName),
                        fileVirtualPath = Path.Combine(virtualPath, cssFileName).Replace(Path.DirectorySeparatorChar, '/'),
                        shapeType = GetShapePrefix() + GetAlternateShapeNameFromFileName(cssFileName),
                        extensionDescriptor
                    });
                return shapes;
            });

            foreach (var iter in hits) {
                var hit = iter;
                var featureDescriptors = hit.extensionDescriptor.Features.Where(fd => fd.Id == hit.extensionDescriptor.Id);
                var fileName = hit.fileName + GetFileExtension();


                var manifestBuilder = new ResourceManifestBuilder();
                foreach (var provider in _resourceProviders) {
                    manifestBuilder.Feature = provider.Metadata.ContainsKey("Feature") ?
                        (Feature)provider.Metadata["Feature"] :
                        null;
                    provider.Value.BuildManifests(manifestBuilder);
                }
                var resourceProviders = manifestBuilder.ResourceManifests;

                var isManifestResource = (from p in resourceProviders
                                          from r in p.GetResources(GetResourceType())
                                          where
                                              (string.IsNullOrWhiteSpace(r.Value.Url) == false && r.Value.Url.IndexOf(fileName, StringComparison.OrdinalIgnoreCase) >= 0) ||
                                              (string.IsNullOrWhiteSpace(r.Value.UrlDebug) == false && r.Value.UrlDebug.IndexOf(fileName, StringComparison.OrdinalIgnoreCase) >= 0) ||
                                              (string.IsNullOrWhiteSpace(r.Value.UrlCdn) == false && r.Value.UrlCdn.IndexOf(fileName, StringComparison.OrdinalIgnoreCase) >= 0) ||
                                              (string.IsNullOrWhiteSpace(r.Value.UrlCdnDebug) == false && r.Value.UrlCdnDebug.IndexOf(fileName, StringComparison.OrdinalIgnoreCase) >= 0)
                                          select r.Value).Any();
                if (!isManifestResource) {
                    foreach (var featureDescriptor in featureDescriptors) {
                        builder.Describe(iter.shapeType)
                            .From(new Feature { Descriptor = featureDescriptor })
                            .BoundAs(
                                hit.fileVirtualPath,
                                shapeDescriptor => displayContext => {
                                    var shape = ((dynamic)displayContext.Value);
                                    var output = displayContext.ViewContext.Writer;
                                    ResourceDefinition resource = shape.Resource;
                                    string condition = shape.Condition;
                                    Dictionary<string, string> attributes = shape.TagAttributes;
                                    ResourceManager.WriteResource(output, resource, hit.fileVirtualPath, condition, attributes);
                                    return null;
                                });
                    }
                }
            }
        }

        private bool FeatureIsEnabled(FeatureDescriptor fd) {
            return (DefaultExtensionTypes.IsTheme(fd.Extension.ExtensionType) && (fd.Id == "TheAdmin" || fd.Id == "SafeMode")) ||
                _shellDescriptor.Features.Any(sf => sf.Name == fd.Id);
        }
    }

    // discovers .css files and turns them into Style__<filename> shapes.
    public class StylesheetBindingStrategy : StaticFileBindingStrategy, IShapeTableProvider {
        public StylesheetBindingStrategy(IEnumerable<Meta<IResourceManifestProvider>> resourceProviders, IExtensionManager extensionManager, ShellDescriptor shellDescriptor, IVirtualPathProvider virtualPathProvider) : base(resourceProviders, extensionManager, shellDescriptor, virtualPathProvider) {
        }

        public override string GetResourceType() {
            return "stylesheet";
        }

        public override string GetFileExtension() {
            return ".css";
        }

        public override string GetFolder() {
            return "Styles";
        }

        public override string GetShapePrefix() {
            return "Style__";
        }
    }

    // discovers .js files and turns them into Script__<filename> shapes.
    public class JavascriptBindingStrategy : StaticFileBindingStrategy, IShapeTableProvider {
        public JavascriptBindingStrategy(IEnumerable<Meta<IResourceManifestProvider>> resourceProviders, IExtensionManager extensionManager, ShellDescriptor shellDescriptor, IVirtualPathProvider virtualPathProvider) : base(resourceProviders, extensionManager, shellDescriptor, virtualPathProvider) {
        }

        public override string GetResourceType() {
            return "script";
        }

        public override string GetFileExtension() {
            return ".js";
        }

        public override string GetFolder() {
            return "Scripts";
        }

        public override string GetShapePrefix() {
            return "Script__";
        }
    }
}

sebastienros wrote Oct 16 at 8:43 PM

The recommendation is to use a fully-qualified path