Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lack of i18n for error messages #363

Closed
harej opened this issue Feb 25, 2017 · 8 comments
Closed

Lack of i18n for error messages #363

harej opened this issue Feb 25, 2017 · 8 comments

Comments

@harej
Copy link

harej commented Feb 25, 2017

MediaWiki uses this schema validation library for internal matters where non-English support is less important. However, JSON schemas are increasingly being used throughout MediaWiki, and we would like to use this library for all our needs, but lack of error message internationalization is an issue. I volunteer to address this but would like to check with you all first. (I am happy to keep English as a default language of sorts for backwards compatibility.)

@bighappyface
Copy link
Collaborator

Validation errors should provide a general key to indicate the error in a language-agnostic fashion. There has been debate about this in the past and while I understand the goal I believe it is important to keep validation logic separate from UI/content. Maintaining translations for validation failures in multiple languages is simply not a primary function of this package nor the maintainers, and the last thing we need is back and forth over error message semantics in, say, korean.

https://github.com/justinrainbow/json-schema/pulls?utf8=%E2%9C%93&q=is%3Apr%20is%3Aclosed%20i18n

Please see those closed PRs and give me your thoughts.

@harej
Copy link
Author

harej commented Feb 25, 2017

I agree that having language-agnostic error codes is good policy.

However, I look throughout the code, and I do not see language-agnostic error codes, but actual English strings. See this arbitrary example. Having a totally separate mechanism that takes validator errors and outputs localized strings is certainly workable, but first I would need proper error codes to work with. To use the linked example, the code could be something like ERROR_EXCLUSIVEMINIMUM_NOT_MET, which is then fed into something like ErrorLocalization::getErrorString( ERROR_EXCLUSIVEMINIMUM_NOT_MET, 'fr' ), and that would output a localized error message in a way that uncouples validation from UI. (Of course, there would be support for additional parameters so that offending values and the like could be output as part of the localized error message.)

Do you think this approach would work?

@bighappyface
Copy link
Collaborator

From the example (formatted for clarity):

$this->addError(
  $path,
  'Must have a minimum value of ' . $schema->minimum,
  'exclusiveMinimum',
  array('minimum' => $schema->minimum)
);

The addError method signature (formatted for clarity):

public function addError(
  JsonPointer $path = null,
  $message,
  $constraint = '',
  array $more = null
);

Does the $constraint and $more array in the example not provide the relevant data? It defines the constraint that failed and constraint criteria for clarification (e.g. the minimum that was not met in the example).

I used the example schema from the following page in the demo and it happened to demonstrate the example constraint/message:

http://json-schema.org/examples.html

$ php demo.php 
JSON does not validate. Violations:
# var_dump of error array
array(5) {
  'property' =>
  string(3) "age"
  'pointer' =>
  string(4) "/age"
  'message' =>
  string(30) "Must have a minimum value of 0"
  'constraint' =>
  string(7) "minimum"
  'minimum' =>
  int(0)
}

Perhaps not every constraint is pushing out all of the details, but the error output today seems to meet the requirements for a separate mechanism for providing human-readable messages. Am I missing something?

@shmax
Copy link
Collaborator

shmax commented Feb 25, 2017

@bighappyface I agree with everything you say, but perhaps we could make things a little easier for people that want to localize error messages by moving all of the possible values for $constraint to a separate file and referencing constants when calling addError? It wouldn't change the way errors look at all, but it would provide a centralized reference for anyone interested in localizing.

@bighappyface
Copy link
Collaborator

I am game for a PR

@shmax
Copy link
Collaborator

shmax commented Feb 25, 2017

Okie-doke. Comin' up.

@erayd
Copy link
Contributor

erayd commented Feb 26, 2017

I also agree with @shmax that the error handling could use some improvement - thanks for the PR :-).

@bighappyface
Copy link
Collaborator

See #364 - Will be official in 6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants