Skip to content

Do not expose culture we do not accept for languages#16265

Merged
nikolajlauridsen merged 1 commit intov14/devfrom
v14/bugfix/do-not-expose-culture-we-do-not-accept
May 13, 2024
Merged

Do not expose culture we do not accept for languages#16265
nikolajlauridsen merged 1 commit intov14/devfrom
v14/bugfix/do-not-expose-culture-we-do-not-accept

Conversation

@bergmania
Copy link
Member

Description

This PR ensures we do not expose any cultures we do not accept in languages.

Furthermore, I moved it to a service to people can replace the logic

Test

  • Ensure you cannot create custom cultures
  • Ensure what is returned from the culture endpoint actually works for languages

…uthermore, I moved it to a service to people can replace the logic
@ronaldbarendse ronaldbarendse changed the title Do not expose culture we do not accept for lanauges Do not expose culture we do not accept for languages May 10, 2024
@ronaldbarendse
Copy link
Contributor

Am I correct to say that ICultureService should only be used when creating a new language (e.g. to configure what cultures are selectable in the dropdown) and IIsoCodeValidator should be used when you only want to validate the ISO code (e.g. before saving the language)?

If that's the case, the ICultureService can't return any CultureInfos that aren't valid according to the registered IIsoCodeValidator, but that's currently not enforced (e.g. you can override ICultureService and return 'invalid' cultures, although I guess that's why the method is called GetValidCultureInfos()). The ICultureService also can't return CultureInfos that aren't recognized by .NET, so can't we simply use CultureInfo.GetCultures(CultureTypes.AllCultures).Where(_isoCodeValidator.IsValid) (and grouping/ordering) in the AllCultureController?

@bergmania
Copy link
Member Author

@ronaldbarendse, I do not follow. Why would you ever put that business logic in a controller? The idea is you can replace it, with whatever, but Umbraco do not support weird cultures out of box. Cultures are one of the things the still differ a lot, based on the underlying OS.

@ronaldbarendse
Copy link
Contributor

The idea is you can replace it, with whatever, but Umbraco do not support weird cultures out of box.

But doesn't .NET already provide the plumbing to replace/change the cultures returned by CultureInfo.GetCultures() (by using NLS or opting-in to app-local ICU)? And other parts of the codebase (even in ASP.NET Core, like RequestLocalizationMiddleware) get the required culture using new CultureInfo() or CultureInfo.GetCultureInfo(), so the new ICultureService is only used when trying to create a new language. Wouldn't it then make more sense to add this method to the ILanguageService (which already has a dependency on IIsoCodeValidator)?

Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nikolajlauridsen nikolajlauridsen merged commit 962beda into v14/dev May 13, 2024
@nikolajlauridsen nikolajlauridsen deleted the v14/bugfix/do-not-expose-culture-we-do-not-accept branch May 13, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants