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.41k stars 2.39k forks source link

msgctxt should be configurable to be independent of class name #13610

Open sostrich opened 1 year ago

sostrich commented 1 year ago

Is your feature request related to a problem? Please describe.

In OrchardCore, when using a stringlocalizer in a service, "The context string must match the full name of the type the localizer is injecting in". In our understanding, this is not always how the msgctxt in .po files works, or at least this means that not all spec-compliant uses of a msgctxt are possible.

The way we do globalisation is that we have a certain type of class in our source code containing all the information needed (msgid, plurals, etc.) to extract a .pot file. The .pot file is then translated, etc., and once we merge a .po file with translations back to our code, we use OrchardCore's PortableObjectStringLocalizer for localization based on the .po file.

However, the services we inject the localizer in have little to do with the classes from which we extract the strings previously, and sometimes the same string is translated in multiple services, etc. This means we cannot use msgctxt in .pot files

Describe the solution you'd like

when using StringLocalizer from a service, msgctxt should by default be the full name of the type the localizer is injecting in, as it was before. (or it should be configurable that by default the context is null). It should be possible to specifiy the msgctxt for each method call to the localizer ( e.g. by use of a MsgctxtArgument similar to PluralizationArgument).

A method call could look like this

portableObjectStringLocalizer["my_msgId", new MsgctxtArgument("my_msgctxt")]

Describe alternatives you've considered

Tough, since by default context is inject by the factory. Any other suggestions on how to make the msgctxt independent of the service which uses a PortableObjectStringLocalizer are very welcome.

sostrich commented 1 year ago

I would be able to create a PR for this (making sure not to break anything with the current PluralisationArgument implementation) if we end up using orchard core for our localization, but before we put in the effort, would like to know if this feature/implementation is something that would be met with approval.

Any feedback is well appreciated :)

hishamco commented 1 year ago

May I know the issue of using the default message context with your project?

sostrich commented 1 year ago

Sure, thanks for coming back to me on this! Our use case is as follows: We are not using Views, but we have an API that for various reasons needs to provide translated values to a client.

So, here's an example that hopefully illustrates our use case:

msgctxt small_file_upload
msgid "Fileupload failed"
msgstrg "We could not upload this small file"
msgctxt large_file_upload
msgid "Fileupload failed"
msgstrg "We could not upload this large file"

And we we want to reuse the same string in different message contexts and in different classes, which therefore would require localization to work like this:

 class ErrorCodes
  {
    public const string FileUploadFailed = "Fileupload failed";
  }

  public class Class1 
  {
    private readonly IStringLocalizer localizer;
    // ...
    public Result DoStuff()
    {
      try {
          //...
      } catch {
        return ErrorResult(errorMessage: localizer.GetString(ErrorCodes.FileUploadFailed, new MessageContextArgument("large_file_context");
      }
    }

    public class Class2
    {
      private readonly IStringLocalizer localizer;
      public Result DoDifferentStuff()
      {
        try {
          //...

        } catch {
          return ErrorResult(errorMessage: localizer.GetString(ErrorCodes.FileUploadFailed, new MessageContextArgument("small_file_context")));
        }
      }

      public class Class3
      {
        private readonly IStringLocalizer localizer;
        // ...
        public Result DoEvenMoreDifferentStuff()
        {
          try {
            //...

          } catch {
            return ErrorResult(errorMessage: localizer.GetString(ErrorCodes.FileUploadFailed, new MessageContextArgument("small_file_context")));
          }
        }
      }

My understanding is this right now this does not work, because the default context is the class name. But we have two different classes, trying to retrieve the same translation for the same string. We also cannot NOT use a context, because in our .po file, we have two different msgcontexts where this string comes up -> that's why we would require a MessageContextArgument or other way to pass the context to the localizer on a case-by-case basis.

In another application, we have one class MessageLocalizer that does localization of every string and does various things (e.g. transforming the strings before they are translated) before calling its injected IStringLocalizer. In this application, we cannot use msgctxt because it would always be the name of that one class.

We also don't want to pass the names of our classes to the translators to which we give the po file, but rather have a different, more meaningful msgctxt in there.

Our applications are more complex than my example. We have many use cases and classes with strings where the same string needs to be translated the same in different classes, and but also in a different way in other classes. So while I'm certain we could refactor this code above to work with the current OrchardCore implementation, that's not the case for our actual application.

I hope this explains our problem, if not, please let me know!

hishamco commented 1 year ago

And we we want to reuse the same string in different message contexts and in different classes, which therefore would require localization to work like this:

Let me take the first part, you might see many common localization strings on OC across the different module, which require the translators to translate them everywhere which is the thing I dislike, but I think the reason for this is the PO extractor tool scan each project on the solution

So, in your case, I suggest putting the common localization strings in a shared class so the context will be unique and there's no need to duplicate them across the project. This is quite similar to Shared resources in ASP.NET Core

sostrich commented 1 year ago

Ah, I see. So your suggestion would be

public class LargeFileContextMessages {
  public IStringLocalizerLocalizer localizer; // necessary to have it injected here here, because the class it's injected in will determine the 
  //context.
  //...
  public const string LargeFileUploadFailed = "Fileupload failed";
}
public class SmallFileContextMessages {
  public IStringLocalizerLocalizer localizer; // necessary to have it injected here here, because the class it's injected in will determine the 
  //context.
  //...
  public const string SmallFileUploadFailed = "Fileupload failed";
}

And then call it like


public class MyClass {
  LargeFileContextMessages largeFileContextMessages;
  SmallContextMessages smallFileContextMessages;
  public void DoSmallStuff() {
    return ErrorResult(errorMessage: smallFileContextMessages.localizer.GetString(smallFileContextMessages.SmallFileUploadFailed);
}

for various reasons, this is not possible for us, namely that the strings are contained in their own class with one class per string (it makes sense in context) and we even sometimes create instances of these classes during runtime (again, it makes sense in context). But I understand that OrchardCore is not my personal playground and can't cater to my every need and support every use case there people have :)

Your suggestion prompted me to have another look at the code, and we will probaby workaround this by not injecting the IStringLocalizer, but by injecting the IStringLocalizerFactory and calling its IStringLocalizer Create(string baseName, string location); like this factory.Create("our_desired_context", ""); everytime we need a msgctxt.

hishamco commented 1 year ago

public class LargeFileContextMessages { public IStringLocalizerLocalizer localizer; // necessary to have it injected here here, because the class it's injected in will determine the //context. //... public const string LargeFileUploadFailed = "Fileupload failed"; } public class SmallFileContextMessages { public IStringLocalizerLocalizer localizer; // necessary to have it injected here here, because the class it's injected in will determine the //context. //... public const string SmallFileUploadFailed = "Fileupload failed"; }

Again why do you need to duplicate the Fileupload failed in multiple classes? Just put all the common strings into Shared.cs. I remembered I did something similar in OqtaneFramework here or you can have a look at this https://stackoverflow.com/questions/42647384/asp-net-core-localization-with-help-of-sharedresources

sostrich commented 1 year ago

The PO Files docs say "The context serves to disambiguate messages with the same untranslated-string", so if I have the same string with two different translations, I need two different contexts. In OrchardCore, this means using two different classes.

In addition, we have the problem that while it might be common string (same translation required) for some parts of our application, it also might require a different translation for other parts of the application.

so let's say Class1 and Class2 want to translate "Fileupload failed" with one translation, and Class3 wants to translate "Fileupload failed" with a different translation. (And there might be many, many, many classes, not just 3, so it's not really an option to just put a string into every Class1, Class2, Class3).

So putting the string into different classes is the way to achieve a different msgctxt (and therefore a different translation) for the string where required. So in the above codeexample, Class1 and Class2 would have to call smallFileContextMessages.localizer.GetString(smallFileContextMessages.SmallFileUploadFailed);

and Class3 would call largeFileContextMessages.localizer.GetString(largeFileContextMessages.largeFileUploadFailed); they would have to call different classes because the the localizer with the correct context is there.

I guess you wouldn't have to duplicate the actual "FileUpload failed" string, but could reference it from Shared.cs in LargeFileContextMessages and SmallFileContextMessages, but still the problem remains.

I hope it makes it clear what I mean, and like I said, it's not an option for us unfortunately due to the somewhat arcane reasons I mentioned in my comment above (and I understand if Orchard doesn't want to support these).

sebastienros commented 1 year ago

The localizer instance, when resolved, it using the context of the class it's resolved in. I think that a solution to your problem would be to create a custom instance with a custom context. This should definitely be doable by doing what the DI is doing internally. If not then we should expose this API.

However the PO extractor might not find these, but maybe you don't need it. Or it could be extended to mark specific strings as localization strings without having to use them with S[] or T[] ...

hishamco commented 1 year ago

However the PO extractor might not find these, but maybe you don't need it. Or it could be extended to mark specific strings as localization strings without having to use them with S[] or T[] ...

I just saw an issue in PO extractor repo, asking for that, I will submit a PR to make this possible, this way you could use the variable names you like

sostrich commented 1 year ago

The localizer instance, when resolved, it using the context of the class it's resolved in. I think that a solution to your problem would be to create a custom instance with a custom context. This should definitely be doable by doing what the DI is doing internally. If not then we should expose this API.

Thanks for having a look! Is this what you mean (my workaround described above):

Your suggestion prompted me to have another look at the code, and we will probaby workaround this by not injecting the IStringLocalizer, but by injecting the IStringLocalizerFactory and calling its IStringLocalizer Create(string baseName, string location); method like this factory.Create("our_desired_context", ""); everytime we need a msgctxt.

Maybe it can be added to the documentation? This seems to work pretty well in my proof of concept.

@hishamco

However the PO extractor might not find these, but maybe you don't need it. Or it could be extended to mark specific strings as localization strings without having to use them with S[] or T[] ...

I just saw an issue in PO extractor repo, asking for that, I will submit a PR to make this possible, this way you could use the variable names you like

We currently do our own extraction and .po file creation, because here we also want something customizable, e.g. we also want to provide comments from the source code as comments in the .po file. Can you post a link to the issue? I can't find it...

hishamco commented 1 year ago

Maybe it's too late, but what have you come up to @sostrich, is extending the PO extractor enough for your case?