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

Default CharCollection for url replacing does not get loaded properly for the DefaultShortStringHelper used to generate urls #9454

Closed
sniffdk opened this issue Nov 25, 2020 · 5 comments
Labels
state/needs-more-info We don't have enough information to give a good reply status/stale Marked as stale due to inactivity

Comments

@sniffdk
Copy link
Contributor

sniffdk commented Nov 25, 2020

When creating the config for the default DefaultShortStringHelper it tries to get a collection of url replacement chars:

public DefaultShortStringHelperConfig WithDefault(IUmbracoSettingsSection umbracoSettings)
        {
            UrlReplaceCharacters = umbracoSettings.RequestHandler.CharCollection
                .Where(x => string.IsNullOrEmpty(x.Char) == false)
                .ToDictionary(x => x.Char, x => x.Replacement);

However, that collection ends up being empty, so the only replacement of the danish characters like æøå happens in the Utf8ToAsciiConverter.ToAsciiString method when the default CleanStringType.TryAscii is set. This means that chars are incorrectly replace, ø -> o (insteda of oe), å -> a (instead of aa), æ gets replaced correctly to ae.

If I manually enter the replacement collection in the umbracoSettings.config it works correctly, eg:

<urlReplacing toAscii="try">
      <char org=" ">-</char>
      <char org="'"></char>
      <char org="?"></char>
      <char org="æ">ae</char>
      <char org="ø">oe</char>
      <char org="å">aa</char>

Tested on Umbraco 8 versions 8.7.0 and 8.8.0

@nul800sebastiaan
Copy link
Member

Thanks @sniffdk, where do you notice the problem?

Note: changing this now will be a breaking change as the default behavior changes and that's going to catch a lot of people out I guess.. so not sure if we can/should do something about it at this point. 😬

@nul800sebastiaan nul800sebastiaan added the state/needs-more-info We don't have enough information to give a good reply label Dec 14, 2020
@sniffdk
Copy link
Contributor Author

sniffdk commented Dec 14, 2020

When generating urls eg for the backoffice, basically everything that generates urls from the DefaultShortStringHelper:
image

It's a huge breaking change for sure for all existing urls, seo wise, but it's also pretty annoying to have Umbraco ship with a faulty url generator 😅

@umbrabot
Copy link

Hiya @sniffdk,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant
This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@umbrabot umbrabot added the status/stale Marked as stale due to inactivity label Jul 19, 2022
@sniffdk
Copy link
Contributor Author

sniffdk commented Aug 1, 2022

@umbrabot still relevant
The default URL replacement seems to still replace capitalized ÆØÅ wrong.
There are only lowercased letters in the default replacement collection: https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Core/Configuration/Models/RequestHandlerSettings.cs#L20
So my best guess is that the url's are replaced before they are converted to lower case.
Tested on a new install of Umbraco 10.0.1

@sniffdk
Copy link
Contributor Author

sniffdk commented Nov 16, 2022

@nul800sebastiaan I think the bot forgot this one? 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/needs-more-info We don't have enough information to give a good reply status/stale Marked as stale due to inactivity
Projects
None yet
Development

No branches or pull requests

3 participants