Skip to content

Transliteration Refactoring#19156

Open
hishamco wants to merge 9 commits intomainfrom
hishamco/transliteration-refactor
Open

Transliteration Refactoring#19156
hishamco wants to merge 9 commits intomainfrom
hishamco/transliteration-refactor

Conversation

@hishamco
Copy link
Copy Markdown
Member

@hishamco hishamco commented Apr 15, 2026

Related to #19009

/cc @urbanit

@hishamco hishamco requested a review from sebastienros April 15, 2026 20:33
Comment thread src/OrchardCore.Modules/OrchardCore.Localization/Startup.cs
Comment thread src/OrchardCore.Modules/OrchardCore.Liquid/Startup.cs
Copy link
Copy Markdown
Member

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

A few comments but I don't disagree with the idea

@hishamco hishamco requested a review from sebastienros April 16, 2026 18:15
Comment thread src/OrchardCore.Modules/OrchardCore.Localization/Liquid/TransliterateFilter.cs Outdated
@sebastienros
Copy link
Copy Markdown
Member

Waiting for filters to be split

@hishamco
Copy link
Copy Markdown
Member Author

Waiting for filters to be split

Could you please elaborate?

BTW, we can combine the filters

{{ "Ελληνικά" | transliterate | slugify }}

That means there's no need for slugify arguments transliterate, so no dependency between the filters. @sebastienros, is that what you mean above?

{
var transliterateArg = arguments["transliterate"];
var transliterate = transliterateArg.IsNil() || transliterateArg.ToBooleanValue();
var slug = transliterate
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should keep the two filters separate. I looked at the implementation of the service and the slugification logic is not impacted by the transliteration, these are two distinct operations. We should just do | transliterate | slugify

    public string Slugify(string text, bool transliterate)
    {
        if (transliterate && !string.IsNullOrEmpty(text))
        {
            text = text.Transliterate();
        }

        return Slugify(text);
    }

For the same reason this was extracted from the service in this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same as I mentioned in my above comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants