FIX handle non breakable spaces

lekoala commented 4 months ago

Fixes issue https://github.com/silverstripe/silverstripe-framework/issues/11162 and very likely will also deal with https://github.com/silverstripe/silverstripe-framework/issues/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

Pull request checklist

[x] The target branch is correct
- See picking the right version
[x] 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.
[x] The commit messages follow our commit message guidelines
[x] The PR follows our contribution guidelines
[x] Code changes follow our coding conventions
[x] This change is covered with tests (or tests aren't necessary for this change)
[x] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
[x] CI is green

GuySartorelli commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

diving into the unit tests, the issue was already known but somehow added back by this commit https://github.com/silverstripe/silverstripe-framework/commit/029a8b9586888cff44f621b7e4299eddd0f6528d

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 commented 4 months ago

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

lekoala commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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

GuySartorelli commented 3 months ago

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

michalkleiner commented 3 months ago

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

GuySartorelli commented 3 months ago

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 commented 3 months ago

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

silverstripe / silverstripe-framework