Repository navigation
Forward ValidationAttribute.ErrorMessage to EntryValidation - #1454
Poseidonas wants to merge 2 commits into
Conversation
CreateValidation reads each validation attribute for its bounds, pattern or data type and discards everything else, so a message written on the attribute never reaches the consumer and the entry editor has only its generic wording to fall back on. EntryValidation gains an ErrorMessage which stays null when no attribute supplied one, keeping the generic messages as the fallback asked for in PHOENIXCONTACT#1450. The first message wins: a member carrying several attributes has no obvious winner, and picking one beats concatenating them. Fixes PHOENIXCONTACT#1450
|
|
| if (validation.ErrorMessage is null && attribute.ErrorMessage is not null) | ||
| validation.ErrorMessage = attribute.ErrorMessage; |
There was a problem hiding this comment.
Thanks for your first contribution! EntrySerialize is full localizable. The ErrorMessage is used for non-localizable error messages. I would prefer also keep the localization consistent and use result of localized ErrorMessageResourceName with ErrorMessageResourceType like we already do that in DisplayAttribute.
There was a problem hiding this comment.
While I did not explicitly say that, it was definitely the intend to cover the localized properties as well (or primarily even) 😅
Reading ErrorMessage directly returns null whenever the message was given as a resource, which is the case the localized properties exist for. FormatErrorMessage resolves ErrorMessageResourceName and ErrorMessageResourceType and falls back to the literal string, the same way DisplayAttribute.GetName does for the display name. It is only called when the attribute declares a message. Without that guard every member would carry the framework's own English default and the consumer would lose the choice of fallback.
|
You are both right, and the localized case was the more important half of it. Fixed.
The third row is what I was reading, so a message given as a resource arrived as nothing at all. One decision I would like checked
if (validation.ErrorMessage is null &&
(attribute.ErrorMessage is not null || attribute.ErrorMessageResourceName is not null))
validation.ErrorMessage = attribute.FormatErrorMessage(attributeProvider.GetDisplayName() ?? string.Empty);Without that guard, row one applies to every member that never asked for a message, and each one would arrive carrying the framework's own English default. That would take the choice of fallback away from the consumer and hand it a string that ignores the current culture. Leaving it null keeps that decision where it was. If you would rather have the framework default there, it is one condition to remove. The display name comes from Tests Added a case for the resource path alongside the literal one, with a static class standing in for a generated resource. Worth noting for anyone reading it later: the resource type has to expose a static property, not a
|
Fixes #1450.
DefaultSerialization.CreateValidation()walks the validation attributes on a member and reads each one for its bounds, pattern or data type. Everything else on the attribute is dropped, so a message written by the author never leaves the server and the entry editor has only its generic wording to show.EntryValidationnow carries anErrorMessage, filled fromValidationAttribute.ErrorMessageand left null when no attribute supplied one — which keeps the generic messages as the fallback you asked for rather than replacing them.The one decision I had to make
A member can carry several validation attributes, and there is no obvious winner among their messages. I kept the first one that has a message rather than concatenating, on the grounds that a joined string reads badly in a form field. If you would rather have all of them, or the message belonging to the rule that actually failed, that is a different shape — say the word and I will rework it.
Verification
dotnet build src/Moryx/Moryx.csproj: 0 errors.Moryx.Testssuite: 356 passed, 2 skipped.ValidationTests: one asserting the message reachesEntryValidation, one asserting it stays null for an attribute without a message, so the fallback path is guarded as well.ValidationDummygains one property carryingErrorMessage, and the sub-entry count inSetupmoves from 8 to 9.devwithout the change (EntryValidationhas noErrorMessage), which is the intended dependency.Clone()copies the new field along with the rest.I have not touched the consuming side.
ngx-moryx-webwould need to prefervalidation.errorMessageover its generic string where it renders the message, and I have not looked at what that involves — happy to follow up there if it is wanted.