Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 0 additions & 5 deletions src/Libraries/Nop.Core/Http/NopHttpDefaults.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,4 @@ public static partial class NopHttpDefaults
/// Gets the name of a request item that stores the value that indicates whether the request is being redirected by the generic route transformer
/// </summary>
public static string GenericRouteInternalRedirect => "nop.RedirectFromGenericPathRoute";

/// <summary>
/// Gets the name of a request item that stores the value with default language for sitemap.xml
/// </summary>
public static string ForcedSitemapXmlLanguage => "nop.ForcedSitemapXmlLanguage";
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Nop.Core;
using Nop.Core.Http;
using Nop.Core.Infrastructure;
using Nop.Services.Localization;

Expand Down Expand Up @@ -39,11 +38,7 @@ public LanguageParameterTransformer(IHttpContextAccessor httpContextAccessor,
/// <returns>The transformed value</returns>
public string TransformOutbound(object value)
{
// first check if we have forced value for sitemap.xml
if (_httpContextAccessor.HttpContext?.Items[NopHttpDefaults.ForcedSitemapXmlLanguage] is string forcedLang && !string.IsNullOrEmpty(forcedLang))
return forcedLang;

// then try to get a language code from the route values
//first try to get a language code from the route values
var routeValues = _httpContextAccessor.HttpContext.Request.RouteValues;
if (routeValues.TryGetValue(NopRoutingDefaults.RouteValue.Language, out var routeValue))
{
Expand Down
86 changes: 31 additions & 55 deletions src/Presentation/Nop.Web/Factories/SitemapModelFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@
using Nop.Core.Domain.Localization;
using Nop.Core.Domain.News;
using Nop.Core.Domain.Seo;
using Nop.Core.Domain.Stores;
using Nop.Core.Domain.Topics;
using Nop.Core.Events;
using Nop.Core.Http;
using Nop.Core.Infrastructure;
using Nop.Services.Blogs;
using Nop.Services.Catalog;
Expand Down Expand Up @@ -524,7 +522,7 @@ protected virtual async Task WriteSitemapUrlAsync(XmlWriter writer, SitemapUrlMo
/// <param name="fullPath">The path and name of the sitemap file</param>
/// <param name="id">Sitemap identifier</param>
/// <returns>A task that represents the asynchronous operation</returns>
protected virtual async Task GenerateAsync(string fullPath, int id = 0)
protected virtual async Task GenerateAsync(string fullPath, int id = 0)
{
//generate all URLs for the sitemap
var sitemapUrls = await GenerateUrlsAsync();
Expand Down Expand Up @@ -605,22 +603,6 @@ protected string GetLocalizedUrl(string currentUrl, Language lang)
return new Uri(new Uri(scheme), localizedPath).ToString();
}

/// <summary>
/// Retrieves the list of languages for the given store that are not excluded in the sitemap XML settings,
/// if SEO-friendly URLs for languages are enabled.
/// </summary>
/// <param name="store">The store to retrieve allowed languages for.</param>
/// <returns>
/// A list of <see cref="Language"/> if SEO-friendly URLs are enabled; otherwise, <c>null</c>.
/// </returns>
protected async Task<List<Language>> GetAllowedLanguagesAsync(Store store)
{
return _localizationSettings.SeoFriendlyUrlsForLanguagesEnabled
? (await _languageService.GetAllLanguagesAsync(storeId: store.Id))
.Where(lang => !_sitemapXmlSettings.DisallowLanguages.Contains(lang.Id))
.ToList()
: null;
}
#endregion

#region Methods
Expand Down Expand Up @@ -841,43 +823,27 @@ public virtual async Task<SitemapModel> PrepareSitemapModelAsync(SitemapPageMode
/// </returns>
public virtual async Task<SitemapXmlModel> PrepareSitemapXmlModelAsync(int id = 0)
{
var workingLanguage = await _workContext.GetWorkingLanguageAsync();
var language = await _workContext.GetWorkingLanguageAsync();
var store = await _storeContext.GetCurrentStoreAsync();

// get list of allowed languages (null if multilingual URLs are disabled)
var languages = await GetAllowedLanguagesAsync(store);

// select current language if allowed, fallback to first allowed if needed
var language = languages?.FirstOrDefault(lang => lang.Id == workingLanguage?.Id) ?? languages?.FirstOrDefault() ?? workingLanguage;
var fileName = string.Format(NopSeoDefaults.SitemapXmlFilePattern, store.Id, language.Id, id);
var fullPath = _nopFileProvider.GetAbsolutePath(NopSeoDefaults.SitemapXmlDirectory, fileName);

if (language.Id != workingLanguage.Id)
_actionContextAccessor.ActionContext.HttpContext.Items[NopHttpDefaults.ForcedSitemapXmlLanguage] = language.UniqueSeoCode.ToLowerInvariant();

try
if (_nopFileProvider.FileExists(fullPath) && _nopFileProvider.GetLastWriteTimeUtc(fullPath) > DateTime.UtcNow.AddHours(-_sitemapXmlSettings.RebuildSitemapXmlAfterHours))
{
var fileName = string.Format(NopSeoDefaults.SitemapXmlFilePattern, store.Id, language.Id, id);
var fullPath = _nopFileProvider.GetAbsolutePath(NopSeoDefaults.SitemapXmlDirectory, fileName);

if (_nopFileProvider.FileExists(fullPath) && _nopFileProvider.GetLastWriteTimeUtc(fullPath) > DateTime.UtcNow.AddHours(-_sitemapXmlSettings.RebuildSitemapXmlAfterHours))
{
return new SitemapXmlModel { SitemapXmlPath = fullPath };
}

//execute task with lock
if (!await _locker.PerformActionWithLockAsync(
fullPath,
TimeSpan.FromSeconds(_sitemapXmlSettings.SitemapBuildOperationDelay),
async () => await GenerateAsync(fullPath, id)))
{
throw new InvalidOperationException();
}

return new SitemapXmlModel { SitemapXmlPath = fullPath };
}
finally

//execute task with lock
if (!await _locker.PerformActionWithLockAsync(
fullPath,
TimeSpan.FromSeconds(_sitemapXmlSettings.SitemapBuildOperationDelay),
async () => await GenerateAsync(fullPath, id)))
{
_actionContextAccessor.ActionContext.HttpContext.Items.Remove(NopHttpDefaults.ForcedSitemapXmlLanguage);
throw new InvalidOperationException();
}

return new SitemapXmlModel { SitemapXmlPath = fullPath };
}

/// <summary>
Expand Down Expand Up @@ -912,16 +878,26 @@ var name when name.Equals(nameof(ProductTag), StringComparison.InvariantCultureI
_ => GetUrlHelper().RouteUrl(routeName, values, protocol)
};

//url for current language
var url = await routeUrlAsync(routeName,
Copy link
Contributor

Choose a reason for hiding this comment

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

This code does not need to be removed. By default, the main address is the address without localization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! However, I'd like to clarify why we replaced this part:

var url = await routeUrlAsync(routeName,
    getRouteParamsAwait != null ? await getRouteParamsAwait(null) : null,
    await GetHttpProtocolAsync());

with this one:

var languages = _localizationSettings.SeoFriendlyUrlsForLanguagesEnabled
    ? (await _languageService.GetAllLanguagesAsync(storeId: store.Id))
        .Where(lang => !_sitemapXmlSettings.DisallowLanguages.Contains(lang.Id))
        .ToList()
    : null;

var workingLanguage = await _workContext.GetWorkingLanguageAsync();
var language = languages?.FirstOrDefault(lang => lang.Id == store.DefaultLanguageId)
    ?? languages?.FirstOrDefault()
    ?? workingLanguage;

var url = await routeUrlAsync(
    routeName,
    getRouteParamsAwait != null ? await getRouteParamsAwait(language.Id) : null,
    await GetHttpProtocolAsync());

if (language.Id != workingLanguage.Id)
    url = GetLocalizedUrl(url, language);

Why this change?

Even though the original version uses getRouteParamsAwait(null), that null does not eliminate language context. In fact:

  • For some entities (like News), LanguageId is passed explicitly (e.g., news.LanguageId).
  • For others, like in GetSeoRouteParamsAwait(...), the LanguageId is not provided, and the system falls back to the current language from IWorkContext, resolved in _urlRecordService.GetSeNameAsync(...).

This makes the result dependent on the active user/session, which is undesirable for sitemap generation.

In addition to that, the default IOutboundParameterTransformer implementation in LanguageParameterTransformer.cs also:

  • First checks for the language in RouteValues
  • Then falls back to IWorkContext

This behavior is not fully deterministic and leads to inconsistencies in the resulting URLs.

That’s why we explicitly select a language for sitemap URL generation:

  • Prefer the default store language (if allowed)
  • Fallback to the first allowed language
  • Fallback to the current one (last resort)

And finally, we use this condition:

if (language.Id != workingLanguage.Id)
    url = GetLocalizedUrl(url, language);

to ensure the generated URL reflects the explicitly chosen language, overriding whatever may have been implicitly applied via IOutboundParameterTransformer.

This guarantees that all sitemap entries are generated consistently and intentionally, regardless of the current user context or route state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I understand your idea and the problem you are solving, but it is not related to the ticket itself. Let's create a separate ticket and a separate pull request for this task. I will review everything again. As for the task #7777 itself, we decided not to implement it in the core for now

getRouteParamsAwait != null ? await getRouteParamsAwait(null) : null,
await GetHttpProtocolAsync());

var store = await _storeContext.GetCurrentStoreAsync();

var updatedOn = dateTimeUpdatedOn ?? DateTime.UtcNow;
var languages = await GetAllowedLanguagesAsync(store);
var languages = _localizationSettings.SeoFriendlyUrlsForLanguagesEnabled
? (await _languageService.GetAllLanguagesAsync(storeId: store.Id))
.Where(lang => !_sitemapXmlSettings.DisallowLanguages.Contains(lang.Id)).ToList()
: null;

// select store default language if allowed, fallback to first allowed if needed
var workingLanguage = await _workContext.GetWorkingLanguageAsync();
var language = languages?.FirstOrDefault(lang => lang.Id == store.DefaultLanguageId) ?? languages?.FirstOrDefault() ?? workingLanguage;

//url for current language
var url = await routeUrlAsync(routeName,
getRouteParamsAwait != null ? await getRouteParamsAwait(language.Id) : null,
await GetHttpProtocolAsync());

if (language.Id != workingLanguage.Id)
url = GetLocalizedUrl(url, language);

if (languages == null || languages.Count == 1)
return new SitemapUrlModel(url, new List<string>(), updateFreq, updatedOn);

Expand All @@ -933,7 +909,7 @@ var name when name.Equals(nameof(ProductTag), StringComparison.InvariantCultureI
getRouteParamsAwait != null ? await getRouteParamsAwait(lang.Id) : null,
await GetHttpProtocolAsync());

return GetLocalizedUrl(currentUrl, lang);
return lang.Id != workingLanguage.Id ? GetLocalizedUrl(currentUrl, lang) : currentUrl;
})
.Where(value => !string.IsNullOrEmpty(value))
.ToListAsync();
Expand Down