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

Mod setting serialisation is dependent on current culture #18729

Closed
ppy-sentryintegration bot opened this issue Jun 16, 2022 · 3 comments · Fixed by #19221
Closed

Mod setting serialisation is dependent on current culture #18729

ppy-sentryintegration bot opened this issue Jun 16, 2022 · 3 comments · Fixed by #19221
Labels
area:multiplayer priority:0 Showstopper. Critical to the next release. type:online

Comments

@ppy-sentryintegration
Copy link

Note the dotless "i" in ınitial_rate. This is happening because the users are using the Turkish language.

Resolution is not obvious as the offending .ToLower() call is in humanizer, in .Underscore(), which we use for serialising settings:

https://github.com/ppy/osu/blob/master/osu.Game/Online/API/APIMod.cs#L45


Sentry Issue: OSU-1Y2

System.Net.WebException: UnprocessableEntity
  ?, in Task WebRequest.Complete(Exception e)
  ?, in async Task WebRequest.beginResponse(CancellationToken cancellationToken)
  ?, in async Task WebRequest.internalPerform(CancellationToken cancellationToken)

osu.Game.Online.API.APIException: unknown setting for WD (ınitial_rate)

Failed to submit score
@bdach
Copy link
Collaborator

bdach commented Jun 16, 2022

Submitted issue about this upstream to see if we can maybe get this fixed there somehow (Humanizr/Humanizer#1212).

@peppy peppy added the priority:0 Showstopper. Critical to the next release. label Jun 18, 2022
@bdach
Copy link
Collaborator

bdach commented Jun 20, 2022

Upstream is unresponsive for now. There were suggestions of adding our own copies of the affected methods and fixing ourselves - any objections to that? Thoughts on where the copies should live?

I'm also thinking of banning string.To{Lower,Upper}() as there are other usages that look suspect. LocalisableString.To{Lower,Upper}() would stay unbanned, because there the culture-variant behaviour of lower/upper is actually desired.

@frenzibyte
Copy link
Member

frenzibyte commented Jun 20, 2022

Can agree with that, sounds like it would work quite well. For the copies, I would imagine them living in a StringExtensions class, and ban the Humanizer ones as to not conflict with them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:multiplayer priority:0 Showstopper. Critical to the next release. type:online
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants