-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add support for language exclusion in sitemap.xml #7778
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -878,18 +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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
This makes the result dependent on the active user/session, which is undesirable for sitemap generation. In addition to that, the default
This behavior is not fully deterministic and leads to inconsistencies in the resulting URLs. That’s why we explicitly select a
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 This guarantees that all sitemap entries are generated consistently and intentionally, regardless of the current user context or route state. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = _localizationSettings.SeoFriendlyUrlsForLanguagesEnabled | ||
? await _languageService.GetAllLanguagesAsync(storeId: store.Id) | ||
? (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); | ||
|
||
|
@@ -901,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(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To configure it, you will have to add UI similar to RobotsTxtSettings.DisallowLanguages
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now there is no UI for administrating the SitemapXmlSettings so I will add new section for all settings, not just for this new property if you are ok with that?