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

Fix script tag helper dependencies inheriting the script's HTML attributes #15438

Merged
merged 18 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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 @@ -43,11 +43,23 @@ public RequireSettings(ResourceManagementOptions options)
AppendVersion = options.AppendVersion;
}

public bool HasAttributes
public RequireSettings(ResourceManagementOptions options, ResourceDefinition resource)
: this(options)
{
get { return _attributes != null && _attributes.Any(a => a.Value != null); }
Name = resource.Name;
Type = resource.Type;
BasePath = resource.BasePath;
Version = resource.Version;
Position = resource.Position;

if (resource.Attributes != null)
{
_attributes = new Dictionary<string, string>(resource.Attributes);
}
}

public bool HasAttributes => _attributes != null && _attributes.Any(a => a.Value != null);

/// <summary>
/// The resource will be displayed in the head of the page.
/// </summary>
Expand Down Expand Up @@ -194,7 +206,7 @@ public RequireSettings SetAttribute(string name, string value)
private Dictionary<string, string> MergeAttributes(RequireSettings other)
{
// efficiently merge the two dictionaries, taking into account that one or both may not exist
// and that attributes in 'other' should overridde attributes in this, even if the value is null.
// and that attributes in 'other' should override attributes in this, even if the value is null.
if (_attributes == null)
{
return other._attributes == null ? null : new Dictionary<string, string>(other._attributes);
Expand Down Expand Up @@ -246,18 +258,17 @@ public RequireSettings CombinePosition(RequireSettings dependent)
return this;
}

public RequireSettings NewAndCombine(RequireSettings other)
{
return new RequireSettings
public RequireSettings New() =>
new()
{
Name = Name,
Type = Type,
Location = Location,
Position = Position
}
.Combine(other)
;
}
};

public RequireSettings NewAndCombine(RequireSettings other) =>
New().Combine(other);

public RequireSettings Combine(RequireSettings other)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ namespace OrchardCore.ResourceManagement
{
public class ResourceDefinition
{
private string _basePath;

public ResourceDefinition(ResourceManifest manifest, string type, string name)
{
Manifest = manifest;
Expand All @@ -32,6 +30,7 @@ private static string Coalesce(params string[] strings)

public ResourceManifest Manifest { get; private set; }

public string BasePath { get; private set; }
public string Name { get; private set; }
public string Type { get; private set; }
public string Version { get; private set; }
Expand Down Expand Up @@ -59,7 +58,7 @@ public ResourceDefinition SetAttribute(string name, string value)

public ResourceDefinition SetBasePath(string basePath)
{
_basePath = basePath;
BasePath = basePath;
return this;
}

Expand Down Expand Up @@ -216,9 +215,9 @@ public TagBuilder GetTagBuilder(RequireSettings settings,

if (url != null && url.StartsWith("~/", StringComparison.Ordinal))
{
if (!string.IsNullOrEmpty(_basePath))
if (!string.IsNullOrEmpty(BasePath))
{
url = string.Concat(_basePath, url.AsSpan(1));
url = string.Concat(BasePath, url.AsSpan(1));
}
else
{
Expand Down
34 changes: 15 additions & 19 deletions src/OrchardCore/OrchardCore.ResourceManagement/ResourceManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,14 @@ private ResourceRequiredContext[] DoGetRequiredResources(string resourceType)
protected virtual void ExpandDependencies(
ResourceDefinition resource,
RequireSettings settings,
ResourceDictionary allResources)
ResourceDictionary allResources) =>
ExpandDependenciesImplementation(resource, settings, allResources, isTopLevel: true);

private void ExpandDependenciesImplementation(
ResourceDefinition resource,
RequireSettings settings,
ResourceDictionary allResources,
bool isTopLevel)
{
if (resource == null)
{
Expand All @@ -385,24 +392,13 @@ protected virtual void ExpandDependencies(
dependencies = new List<string>(settings.Dependencies);
}

// Settings is given so they can cascade down into dependencies. For example, if Foo depends on Bar, and Foo's required
// location is Head, so too should Bar's location, similarly, if Foo is First positioned, Bar dependency should be too.
//
// Forge the effective require settings for this resource.
// (1) If a require exists for the resource, combine with it. Last settings in gets preference for its specified values.
// (2) If no require already exists, form a new settings object based on the given one but with its own type/name.
// Settings is given so the location can cascade down into dependencies. For example, if Foo depends on Bar,
// and Foo's required location is Head, so too should Bar's location, similarly, if Foo is First positioned,
// Bar dependency should be too. This behavior only applies to the dependencies.

var dependencySettings = (((RequireSettings)allResources[resource])
?.NewAndCombine(settings)
?? new RequireSettings(_options)
{
Name = resource.Name,
Type = resource.Type,
Position = resource.Position
}
.Combine(settings))
.CombinePosition(settings)
;
var dependencySettings = ((RequireSettings)allResources[resource])?.New() ?? new RequireSettings(_options, resource);
dependencySettings = isTopLevel ? dependencySettings.Combine(settings) : dependencySettings.AtLocation(settings.Location);
dependencySettings = dependencySettings.CombinePosition(settings);

if (dependencies != null)
{
Expand Down Expand Up @@ -431,7 +427,7 @@ protected virtual void ExpandDependencies(
continue;
}

ExpandDependencies(dependency, dependencySettings, allResources);
ExpandDependenciesImplementation(dependency, dependencySettings, allResources, isTopLevel: false);
}
}

Expand Down
Loading
Loading