FIX handle non breakable spaces

[lekoala]

Fixes issue #11162
and very likely will also deal with #10626

I've targeted this to 5.1 but it probably also apply to 4.x but i don't know if it's worth it to apply the patch there as well

Description

See relevant issue

Manual testing steps

See relevant issue

Issues

• NumericField doesn't valid properly numbers > 1000 with locale fr-Fr #11162
• NumericField formatting error in certain locales #10626

Related

also see https://www.php.net/manual/en/class.numberformatter.php#129192

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[GuySartorelli]

Looks like it might be a sensible solution, can you please add a test so we don't get a regression of this in the future?

[michalkleiner]

We were dealing with something similar recently in Matomo core and we are replacing U+202F and U+2009, so perhaps worth adding that one in here as well, some numeric formats may use that as well.

[lekoala]

diving into the unit tests, the issue was already known but somehow added back by this commit
029a8b9

it also seems that the test was never updated and was doing it's own little cleaning up, which was hiding the issue. I've refactored the cleaning method into a function (and added U+2009 as @michalkleiner suggested) and updated the tests so that they test properly formatted values (meaning, including the narrow non breaking space... i added a comment next to each line, this is really easy to miss if the editor doesn't outline somehow the utf8 character)

[michalkleiner]

Nice work @lekoala! Also good you tracked it's a regression. Can you rebase and retarget at 4.13, please?

[lekoala]

@michalkleiner if you have an easy way to do that i'm happy to, but changing the target branch always ends up adding tons of commits and most of the time, it's actually faster to create a new PR rather than updating this one.
:-) so either I need to find a decent workflow to rebase and retarget a PR, or I'm happy to create a new one to replace this

[michalkleiner]

@michalkleiner if you have an easy way to do that i'm happy to, but changing the target branch always ends up adding tons of commits and most of the time, it's actually faster to create a new PR rather than updating this one. :-) so either I need to find a decent workflow to rebase and retarget a PR, or I'm happy to create a new one to replace this

I can rebase it for you if you've enabled the option for maintainers to update the code.

Usually the easiest way is to squash the commits locally into a single good one, create a temp branch with it, reset the PR branch to the top of 4.13 and then cherry pick the commit. Should be no conflicts or easy to resolve. Then force push the updated branch and change PR target. Delete the temp branch.

(You can do it without the temp branch if you force-push twice and use the remote as your temp, there's multiple ways.)

[lekoala]

@michalkleiner ok, if you are up to do it, i'll let you do that :-) you should be able to edit the PR

[GuySartorelli]

@michalkleiner were you going to take a look at retargetting this PR?

[michalkleiner]

Ah, missed Thomas's comment last week, will do it today.

[GuySartorelli]

CI might try to install from the 5.1 branches since you changed the base branch after pushing changes - if it does, you may need to force push again to get it running correctly.

[michalkleiner]

@GuySartorelli re-pushed after the target change and CI still ✅

[GuySartorelli]

Sweet, LGTM.
Thanks for fixing this @lekoala and thanks for the help @michalkleiner