OrchardCore icon indicating copy to clipboard operation
OrchardCore copied to clipboard

PO files don't work well with DataAnnotations that have multiple parameters

Open rjpowers10 opened this issue 2 years ago • 11 comments

Describe the bug

I'm trying to use a PO file to override some of the built-in .NET DataAnnotation messages, such as "The {0} field is required." This works well when the msgid only has one parameter. It doesn't seem to work with more than one parameter, and I'm not sure how to proceed.

To Reproduce

Example view model

public class Review
{
    [Required]
    [MinLength(5)]
    [MaxLength(50)]
    [Display(Name = "Title")]
    public string Title { get; set; }
}

This should use three DataAnnotation messages:

  1. Required - The {0} field is required.
  2. MinLength - The field {0} must be a string or array type with a minimum length of '{1}'.
  3. MaxLength - The field {0} must be a string or array type with a maximum length of '{1}'.

I want to override the MinLength and MaxLength messages, so I provide the following PO entries:

msgid "The field {0} must be a string or array type with a minimum length of '{1}'."
msgstr "{0} must be at least {1} characters."

msgid "The field {0} must be a string or array type with a maximum length of '{1}'."
msgstr "{0} must be {1} characters or fewer."

The problem is in LocalizedValidationMetadataProvider.

image

The problem seems to be in the block where it localizes the unparameterized error message. But this block only handles the first parameter. So we end up passing the string "The field {0} must be a string or array type with a minimum length of '5'." into the string localizer. But what I really want to pass is the string "The field {0} must be a string or array type with a minimum length of '{1}'.". But I'm not sure how the code would know the replace the '5' with '{1}'. What I really want is the fully unparameterized version of the string, but it seems a little difficult to reproduce that without making a lot of assumptions.

cc @hishamco

rjpowers10 avatar Apr 03 '23 17:04 rjpowers10

I can workaround this with the following PO entries, but it's not ideal since it hardcodes the lengths.

msgid "The field {0} must be a string or array type with a minimum length of '5'."
msgstr "{0} must be at least {1} characters."

msgid "The field {0} must be a string or array type with a maximum length of '50'."
msgstr "{0} must be {1} characters or fewer."

rjpowers10 avatar Apr 03 '23 17:04 rjpowers10

The crux of this seems to resolve around reconstructing the unparameterized version of the string, but that seems difficult without making a lot of assumptions.

rjpowers10 avatar Apr 03 '23 19:04 rjpowers10

@rjpowers10 after a lot of experiments I think you need to disable the line

options.ModelMetadataDetailsProviders.Add(new LocalizedValidationMetadataProvider(localizer));

I think data annotation localization has been fixing at the MVC level, do you remember when we introduce this is it because localization doesn't work properly or there's an issue on DisplayName?

hishamco avatar Apr 07 '23 19:04 hishamco

@rjpowers10 after a lot of experiments I think you need to disable the line

options.ModelMetadataDetailsProviders.Add(new LocalizedValidationMetadataProvider(localizer));

I think data annotation localization has been fixing at the MVC level, do you remember when we introduce this is it because localization doesn't work properly or there's an issue on DisplayName?

I think MVC will localize anything that is explicitly specified on the attribute, such as

[Display(Name = "My Name")]

or

[Required(ErrorMessage = "This is a custom required message")]

I think the reason for OC's LocalizedValidationMetadataProvider is to handle the built-in error messages like this

[Required] // This will default to "The {0} field is required."

If I try to override "The {0} field is required." in a PO file, MVC doesn't handle it unless I either

  1. Re-enable OC's LocalizedValidationMetadataProvider
  2. Duplicate the error message on the attribute like [Required(ErrorMessage = "The {0} field is required.")]

In summary, I think LocalizedValidationMetadataProvder is needed to handle everything listed in DataAnnotationsDefaaultErrorMessages.

rjpowers10 avatar Apr 10 '23 17:04 rjpowers10

The issue for multiple parameter can be fixed, but it needs cases for each ValidationAttribute, I'm thinking about a custom one :)

hishamco avatar Apr 10 '23 21:04 hishamco

The issue for multiple parameter can be fixed, but it needs cases for each ValidationAttribute, I'm thinking about a custom one :)

Yeah, I think the problem is that you can get the original, unformatted string, which is what you want for the IStringLocalizer. But then you don't have a good way to format that result with the validation arguments. I wonder if you need to do something like this, which unfortunately duplicates the logic of the FormatErrorMessage method for each attribute.

string unformattedErrorMessage = GetUnformattedErrorMessage(validationAttribute);
string localizedErrorMessage = _stringLocalizer[unformattedErrorMessage];
string formattedErrorMessage;

if (validationAttribute is RequiredAttribute)
{
    formattedErrorMessage = string.Format(CultureInfo.CurrentCulture, localizedErrorMessage, displayName); // Same logic was RequiredAttribute.FormatErrorMessage
}
else if (validationAttribute is MinLengthAttribute minLengthAttribute)
{
    formattedErrorMessage = string.Format(CultureInfo.CurrentCulture, localizedErrorMessage, displayName, minLengthAttribute.Length); // Same logic as MinLength.FormatErrorMessage
}
// etc.

There's also the case of a custom validation attribute, which isn't handled in my example.

rjpowers10 avatar Apr 11 '23 13:04 rjpowers10

There's also the case of a custom validation attribute, which isn't handled in my example.

That's what I mean in my above comment, we might need to think well about it, @sebastienros your though if you are interested

hishamco avatar Apr 11 '23 14:04 hishamco

@rjpowers10 Did you made any progress on this on your side?

Skrypt avatar Nov 13 '24 13:11 Skrypt

I haven't looked at this in some time, no progress.

rjpowers10 avatar Nov 14 '24 18:11 rjpowers10

Hope to get my hands dirty on this in the upcoming week

hishamco avatar Nov 14 '24 19:11 hishamco

I can workaround this with the following PO entries, but it's not ideal since it hardcodes the lengths.

msgid "The field {0} must be a string or array type with a minimum length of '5'."
msgstr "{0} must be at least {1} characters."

msgid "The field {0} must be a string or array type with a maximum length of '50'."
msgstr "{0} must be {1} characters or fewer."

Currently I'm still using this workaround.

rjpowers10 avatar Nov 14 '24 19:11 rjpowers10