Validation messages translation

[Erindi]

Hi, I want to add translations to your validation rules messages, but I can't find a right way to do it more or less "acceptable". Can you help me a bit with this problem?

As far as i can see almost all your validation rules has static methods with messages which return dynamic text. And almost all this static methods used by their own class objects in non static methods using self:: .
For example:

class ValuesOfCorrectType extends ValidationRule
{
    public function getVisitor(ValidationContext $context)
    {
            // ...

                $context->reportError(
                    new Error(
                        self::unknownFieldMessage($parentType->name, $node->name->value, $didYouMean),
                        $node
                    )
                );

            // ...
    }

    // ...

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
        return sprintf('Field "%s" is not defined by type %s', $fieldName, $typeName) .
            ($message ? sprintf('; %s', $message) : '.');
    }

    // ...
}

I can extend any ValidationRule class give it proper $name and rewrite it in DocumentValidator::$defaultRules array before validation using DocumentValidator::addRule() method. But i can't only redefine ValidationRules static methods with error mesages and do something like

class ValuesOfCorrectType extends \GraphQL\Validator\Rules\ValuesOfCorrectType
{
    private static TranslatorInterface $translator;

    protected $name = 'GraphQL\Validator\Rules\ValuesOfCorrectType';

    // ...

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
        return sprintf(static::$translator->trans('Field "%s" is not defined by type %s'), $fieldName, $typeName) .
            ($message ? sprintf('; %s', $message) : '.');
    }
}

because this methods called via self:: and not via static:: (but even if static:: was used, this is also not the best way to add translations to messages) and static methods is not comfortable to translate if you don't have a static translator (i use symfony/translation 5.0).

All i can do in this situation for now, as i can see it, is to make a full copy of every validation class that i want to translate to my project, make all necessary changes and rewrite it via DocumentValidator::addRule(). But that is not a proper way to do it, because of updates and etc.

Maybe there is another way to solve this problem? Or maybe you working on some changes for validation messages (like some messages storage class where i can redefine any validation messages)?

Thank you in advance!

[vladar]

Can you elaborate a bit more on what is your use-case? Why do you want to translate validation messages? These messages are not supposed to be shown to end-users. They are mostly for developers (those who write API clients).

[Erindi]

Our API used as backend for a few frontend applications and for admin panel wich is used by non developers. And API by itself used by some specialists who know how it works, but they are not developers. Some of our frontend applications use custom made graphQl query builders and all this non developer users not always understand returned errors. For example if they entered wrong type, or something wrong in their query if they work with API directly or this is just frontend app query builder problem for developers. And also we have another validators running on this API which returns translated errors and users see some translated errors and some not.

In short words we have API, and it used by some other people, we not always know how they handle all our errors on their side, and where they show them, and they ask us for full translation. So we need to translate all our errors.

[vladar]

I don't want to complicate things in this library by introducing the translation layer (as this is not intended usage of those errors) but we can allow overriding message templates. Something along the lines:

class ValuesOfCorrectType extends ValidationRule
{
    static protected $messages = [
        'unknownFieldMessage' => 'Field "%s" is not defined by type %s',
    ];

    // This should be probably common in ValidationRule class
    public static function setMessage($id, $textTemplate)
    {
        static::$messages[$id] = $textTemplate;
    }

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
        return sprintf(static::$messages['unknownFieldMessage'], $fieldName, $typeName) .
            ($message ? sprintf('; %s', $message) : '.');
    }
}

Will it work for you?

[vladar]

Additionally, we can add interpolate method and use it in place of sprintf and also allow setting a custom implementation for it.

[spawnia]

@vladar how about we just use public properties and prefix them all with $message:

class ValuesOfCorrectType extends ValidationRule
{
    static public $messageUnknownField = 'Field "%s" is not defined by type %s';

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
        return sprintf(static::$messageUnknownField, $fieldName, $typeName) .
            ($message ? sprintf('; %s', $message) : '.');
    }
}

That way, autocompletion and static validation work to ensure the correct property is changed.

[Erindi]

I don't want to complicate things in this library by introducing the translation layer (as this is not intended usage of those errors) but we can allow overriding message templates. Something along the lines:

class ValuesOfCorrectType extends ValidationRule
{
    static protected $messages = [
        'unknownFieldMessage' => 'Field "%s" is not defined by type %s',
    ];

    // This should be probably common in ValidationRule class
    public static function setMessage($id, $textTemplate)
    {
        static::$messages[$id] = $textTemplate;
    }

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
        return sprintf(static::$messages['unknownFieldMessage'], $fieldName, $typeName) .
            ($message ? sprintf('; %s', $message) : '.');
    }
}

Will it work for you?

Yes, that will help alot, but maybe somethig like

class ValuesOfCorrectType extends ValidationRule
{
    static public $messages = [
        'unknownFieldMessage' => 'Field "%s" is not defined by type %s',
    ];

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
        return sprintf(static::$messages['unknownFieldMessage'], $fieldName, $typeName) .
            ($message ? sprintf('; %s', $message) : '.');
    }
}

or add some getAllMessages() method, because we need to get all message templates from ValidationRule, translate and set to SomeValidationRule::$messages

[vladar]

Sure, a PR is welcome! We can continue this discussion there.

[vladar]

@spawnia Works for me and looks simpler 👍

[Erindi]

Sure, a PR is welcome! We can continue this discussion there.

Okay, thank you! Then I'll make a PR with something like

   static public $messages = [
       'unknownFieldMessage' => 'Field "%s" is not defined by type %s',
   ];

   public static function unknownFieldMessage($typeName, $fieldName, $message = null)
   {
       return sprintf(static::$messages['unknownFieldMessage'], $fieldName, $typeName) .
           ($message ? sprintf('; %s', $message) : '.');
   }

No seters, no getters, just public static $messages array. Okay?

Variant with static public $messageUnknownField = 'Field "% s" is not defined by type% s'; is also suitable, but not entirely convenient, because you will need to extract and rewrite errors for list of validation classes, and for this you will first need to extract all static properties with the $message prefix from every class, translate and then set each message back.

[spawnia]

Variant with static public $messageUnknownField = 'Field "% s" is not defined by type% s'; is also suitable, but not entirely convenient, because you will need to extract and rewrite errors for list of validation classes, and for this you will first need to extract all static variables with the $message prefix from every class, translate and then set each message back.

I can not exactly follow why the indirection of using an array is necessary. What's wrong with just setting the properties for each class:

ValuesOfCorrectType::$messageUnknownField = '...';
AnotherValidationRule::$messageSomeCondition = '...';

Another idea to allow for dynamic messages:

ValuesOfCorrectType::$messageUnknownField = static function($typeName, $fieldName, $message) {
    return TranslationService::message(...)
};

class ValuesOfCorrectType extends ValidationRule
{
    static public $unknownFieldMessage = 'Field "%s" is not defined by type %s';

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
         if (is_callable(self::$unknownFieldMessage)) {
             return self::$unkownFieldMessage($typeName, $fieldName, $message);
        }
        ...
    }
}

[vladar]

My intention was to allow fetching all messages and their keys. But prefix will probably allow this too if actually required

[Erindi]

Variant with static public $messageUnknownField = 'Field "% s" is not defined by type% s'; is also suitable, but not entirely convenient, because you will need to extract and rewrite errors for list of validation classes, and for this you will first need to extract all static variables with the $message prefix from every class, translate and then set each message back.

I can not exactly follow why the indirection of using an array is necessary. What's wrong with just setting the properties for each class:

ValuesOfCorrectType::$messageUnknownField = '...';
AnotherValidationRule::$messageSomeCondition = '...';

Another idea to allow for dynamic messages:

ValuesOfCorrectType::$messageUnknownField = static function($typeName, $fieldName, $message) {
    return TranslationService::message(...)
};

class ValuesOfCorrectType extends ValidationRule
{
    static public $unknownFieldMessage = 'Field "%s" is not defined by type %s';

    public static function unknownFieldMessage($typeName, $fieldName, $message = null)
    {
         if (is_callable(self::$unknownFieldMessage)) {
             return self::$unkownFieldMessage($typeName, $fieldName, $message);
        }
        ...
    }
}

Because for this variant we need to store some of your lib data (like names of every class message variables or something) in our code, or we need to extend all your validation classes, make translations inside and then replace them using DocumentValidator::addRule(), or extract all $message prefix properties.

But with public static array we can do in our code something like

        foreach ($listOfValidationRulesClasses as $validationRuleClass) {
            
            $translatedMessages = [];
            foreach ($validationRuleClass::$messages as $messageKey => $message) {
                $translatedMessages[$messageKey] = $translator->trans($message);
            }
            $validationRuleClass::$messages = $translatedMessages;
        }

and that will be all. No extending, no callbacks, no other interactions or changes.
On lib side is just public static array with messages to every class, on our side is just this code before validation.

At least that is how i see it. Sorry if i got somethig wrong.

[spawnia]

Forgive my stubbornness, but I still do not see how that is useful.

$translatedMessages[$messageKey] = $translator->trans($message);

This trans() function would have to be really magical in order to produce useful, semantically correct translations for all possible messages. Even taking the keys into account, we would have to guarantee they are globally unique? What about custom validation rules?

Setting a property on a specific class is as simple as it gets. If you require multiple language, you could do something like:

if ($languageA) {
    SomeClass::$messageFoo = 'translatation for a';
    ...
} elseif ($languageB) {
    SomeClass::$messageFoo = 'translation for b';
}

My thinking is you have to know which classes and which kinds of messages exist in order to translate them properly. No amount of indirection is going to solve that.

[Erindi]

This trans() function is from symfony framework translation system. Similar translation system has zend framework and some other frameworks. To explain in a simple way, they use translation files (for example messages.en.yaml or messages.uk.yaml) with contents like

'Expected type %s, found %s': 'Some Translation %s Here %s'
'Field "%s" argument "%s" requires type %s, found %s': 'Полю "%s" з аргументом "%s" має бути типу %s, отримано %s'
'Field %s.%s of required type %s was not provided.': 'Поле %s.%s необхідного типу %s не було надано.'

and then translator function requires something like $translator->trans('Expected type %s, found %s', [], 'messages', 'uk'); (template you need to find in file, filename, filename language). It finds template and returns it translation. If translation is not found trans() returns original text without changes.
All we need is to add translation to all required templates in this file, get $translator from framework, use trans() function on some template and voila.

That is why the simplest way to do this translation is to get list of some templates, translate and set this messages back to some "storage". That is why i propose variant with static public $messages array or something similar if you don't mind.Because we need to store just a list of your classes and that is all (or just scan directory with validators from your lib).

Option like

if ($languageA) {
    SomeClass::$messageFoo = 'translatation for a';
    ...
} elseif ($languageB) {
    SomeClass::$messageFoo = 'translation for b';
}

requires to store a list of classes and list of all properties with $message prefix on our side. Or scan dir, get all classes, get all static properties via reflection, translate all of them and then set (this can create some performance problems, because we need to do it on every request). And on your lib side it will be something like 2 - 5 class properties with messages in each class.

Yes both options is suitable as i see it, first is a bit more convenient for us.
I understand that in my version, you may be worried that if in any of the message arrays there is no key that is used in some of the methods, then there will be an error (or an empty message) and this may not be detected immediately.

I will make PR for any of these options, I just want to understand which one will be acceptable for you and also convenient for our needs.

[spawnia]

From a maintenance perspective, we should be able to guarantee stable property names (or array keys), rather than stable message contents. If the goal is to make sure that all messages are translated, depending on the keys seems like a more reliable long term option.

I personally prefer to write code in a way that prevents silly errors, such as misspelling an array key, or maybe missing a dot or other punctuation in the contents of a message. However, that also comes from a tendency of mine to ruthlessly refactor things.

[Erindi]

Okay, I'll fork and do a PR with something like static public $ unknownFieldMessage = 'Field "% s" is not defined by type% s' ; for all validation classes. This is not the most convenient option for us, but I would not like to do PR with an option that does not completely suit you and it will make it easier to solve our problem anyway.

[spawnia]

Is is possible to extend the default validation rules. All of them conveniently have the actual error message generation extracted to separate methods, which you can override:

graphql-php/tests/Validator/CustomRuleTest.php

Lines 19 to 55 in a5d64c7

	public function testAddRuleCanReplaceDefaultRules(): void
	{
	DocumentValidator::addRule(new class extends ExecutableDefinitions
	{
	public function getName(): string
	{
	return ExecutableDefinitions::class;
	}
	public static function nonExecutableDefinitionMessage(string $defName): string
	{
	return "Custom message including {$defName}";
	}
	});
	$this->expectInvalid(
	self::getTestSchema(),
	null,
	'
	query Foo {
	dog {
	name
	}
	}
	type Cow {
	name: String
	}
	',
	[
	ErrorHelper::create(
	'Custom message including Cow',
	[new SourceLocation(8, 12)]
	),
	]
	);
	}