OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.4k stars 2.39k forks source link

Support both .resx and .po files #2547

Open Skrypt opened 5 years ago

Skrypt commented 5 years ago

From @jrestall

@tomlindhe I assume you need to keep the PO localization working for other modules, otherwise you could just disable the OrchardCore.Localization module and it'll go back to using the ResourceManagerStringLocalizerFactory by default. I'm no expert but one idea to support both could be to create a custom IStringLocalizerFactory (also IHtmlLocalizerFactory) that delegates to either the ResourceManagerStringLocalizerFactory or PortableObjectStringLocalizerFactory depending on some logic.

The OrchardCore.Localization modules calls AddPortableObjectLocalization() in OrchardCore.Localization.Core.ServiceCollectionExtensions and replaces the service that is harvesting localization files (.po) when we enable it. We do this so that the OrchardCore.Localization.Core module be used as standalone solution for using .po file. We replace the service in OrchardCore.Localization module for being able to have .po files in each modules by specifying a ModularPoFileLocationProvider() :

Orchard.Localization.Startup.cs

            // Override the default localization file locations with Orchard specific ones
            services.Replace(ServiceDescriptor.Singleton<ILocalizationFileLocationProvider, ModularPoFileLocationProvider>());

So as as soon as the OrchardCore.Localization module is enabled it breaks the .resx ressources because we are changing the folder and files that it's supposed to harvest and also because we are changing these in the AddPortableObjectLocalization() "this is where we lose .resx support" method.

Orchard.Localization.Core.ServiceCollectionExtensions.cs

            services.AddSingleton<IStringLocalizerFactory, PortableObjectStringLocalizerFactory>();

            services.AddSingleton<IHtmlLocalizerFactory, PortableObjectHtmlLocalizerFactory>();

One solution would be that if only one module needs to use .resx files ; in his startup.cs file then I think he could do something like :

        //This basically remove all Orchard Core custom implementations for .po files
        public override int Order => 99999; //a really high number

        public override void ConfigureServices(IServiceCollection services)
        {
            var serviceDescriptor = services.FirstOrDefault(s => s.ServiceType.FullName.Contains("IStringLocalizerFactory") && s.ImplementationType.FullName == "OrchardCore.Localization.PortableObject.PortableObjectStringLocalizerFactory");
            if (serviceDescriptor != null) { services.Remove(serviceDescriptor); };

            serviceDescriptor = services.FirstOrDefault(s => s.ServiceType.FullName.Contains("IHtmlLocalizerFactory") && s.ImplementationType.FullName == "OrchardCore.Localization.PortableObject.PortableObjectHtmlLocalizerFactory");
            if (serviceDescriptor != null) { services.Remove(serviceDescriptor); };

            services.RemoveAll<ILocalizationFileLocationProvider>(); //maybe not necessary but why not
         }

I think this would reset it all to default.

Skrypt commented 5 years ago

James Restall @jrestall

It only uses the second one because the default IStringLocalizer implementation injects a single IStringLocalizerFactory and the last registered dependency will win in the case there's multiple registered in the container. https://github.com/aspnet/Localization/blob/release/2.2/src/Microsoft.Extensions.Localization.Abstractions/StringLocalizerOfT.cs#L22 That's why one solution I recommend is to replace the default StringLocalizer with a version that accepts an IEnumerable.

jrestall commented 5 years ago

Seems the angle brackets got lost, so just to be clear one solution I recommended was replacing the generic StringLocalizer\.

The replacement of the asp.net IStringLocalizerFactory is where ResX support is lost, so a custom CompoundStringLocalizerFactory() or replacing the default implementations of IViewLocalizer, IStringLocalizer\, IHtmlLocalizer\ with versions that take an IEnumerable\ and that loop through all registered IStringLocalizerFactory or IHtmlLocalizerFactory would be my first approaches.

However I have doubts about this being necessary in the core framework since it doesn't seem like it would be common for an application to use both ResX and PO at the same time for their localization.

Skrypt commented 5 years ago

No but at least let's make the PO an option that is not opt-in to the Localization module directly. Let's leave people having the choice to PO or not. An other point that makes me not really into this is that if we support also .resx we will have people asking us for the resource files. The reason we decided to use PO files is because they have advantages over .resx files. Then why would we want .resx support? What's the advantage of using .resx over PO and vice versa.

One thing I'm sure is that .resx files needs to be compiled as part of an assembly. Though .PO files don't need to. Which means that on runtime we can edit those files and change the text displayed for a string. With a .resx file you would need to recompile and restart the app pool. So, best solution is using .PO files only and to convert your .resx file to .PO files with tools found for this on Github @tomlindhe.

dschmid commented 5 years ago

My opinion is that PO is only about localizing text. But that is not the whole story about localization. So if one needs to localize images, binary data and so on, he or she needs to use .resx files anyway.


Von: Jasmin Savard notifications@github.com Gesendet: Sunday, October 21, 2018 5:52:59 AM An: OrchardCMS/OrchardCore Cc: Subscribed Betreff: Re: [OrchardCMS/OrchardCore] Support both .resx and .po files (#2547)

No but at least let's make the PO an option that is not opt-in to the Localization module directly. Let's leave people having the choice to PO or not. An other point that makes me not really into this is that if we support also .resx we will have people asking us for the resource files. The reason we decided to use PO files is because they have advantages over .resx files. Then why would we want .resx support? What's the advantage of using .resx over PO and vice versa.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/OrchardCMS/OrchardCore/issues/2547#issuecomment-431636690, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AMkJx3vyW5UrmeYfKn-oysA6ghFtT73fks5um--bgaJpZM4XyCY0.

Skrypt commented 5 years ago

Without having the image itself in the .PO file as binary data you can still have a url path.

sebastienros commented 5 years ago

I would also suggest to use custom urls in the PO files. And a module can also contain static assets, so it would contain a po file and assets.

I will still keep this issue open because we could support both.

hishamco commented 5 years ago

Though .PO files don't need to. Which means that on runtime we can edit those files and change the text displayed for a string. With a .resx file you would need to recompile and restart the app pool

@Skrypt Are you sure that you don't require to restart the app?!!

I will still keep this issue open because we could support both.

I'm with this option, but I wanna ask @sebastienros do you want to configure the localization per tenant or for entire application?

Configuring localization per tenant may goes similar to what is done in database access, while configuring localization for the entire application may need to change the code manually or provide sort of settings to add the required localization middleware

Skrypt commented 5 years ago

Yes right now you need to recompile the Theme or Module to get the proper translations from a .po file. Still need to try if the .po file would be in the main web project. Localization is per tenant.

hishamco commented 5 years ago

I see .. recompile still needed in both cases, nothing but .po can be enhanced to be edited and the changes dynamically appears on fly without any sort of recompilation for the module, by implementing notification on change if the translation has been changed

Skrypt commented 5 years ago

Well those .po files are embedded in the modules or themes .dll files... so if you want to refresh the .po translation you need to recompile the module or theme. But the .po file itself doesn't require a compilation. But a solution that could work is one that would just require resetting the tenant since it will restart the Localization module.

hishamco commented 5 years ago

I will still keep this issue open because we could support both.

Any plans to support both? IMHO the user should have the option to choose whatever he/she like whenever he/she like

sebastienros commented 5 years ago

I still want to see a good scenario for that.

hishamco commented 5 years ago

Assume someone created a module for any reason using resx because this is the one supported out-of the-box in ASP.NET Core. He/She want the module play nicely with other modules that come up with orchard without the need to replace resx with po. This is not possible right now because we disable the ResourceManagerStringLocalizer

The Identity system for ASP.NET Core is uses the resx which a good scenario.

We can support both resx and po while it's out-of-the-box in the ASP.NET Core, we can disable resx by default like what we did right now and make an option to enable it from with the admin site

sebastienros commented 5 years ago

Assume someone created a module for any reason using resx because this is the one supported out-of the-box in ASP.NET Core

Ok, let's assume that, I bet there will be a single language in it. How do you provide the other ones?

hishamco commented 5 years ago

Each tenant will have a set of cultures stored onto its database, so the provider will look for resx files that inside the resource path that defined on the localization options, if they are there it will pick them, similarly for po if the translations are not there it will use the default one. I don't it's an issue or many be I miss understood you