Type of error to allow for custom error messages

[onlinesid]

Added 'type' to make it easier to detect what the error is and display custom error message.

An example:

{
"status": "error",
"message": "JSON does not validate",
"errors": [
{
"property": "firstName",
"message": "must be at least 1 characters long",
"type": "minLength" // this is what I've added
}
]
}

To do that I had to add an optional argument $type to addError() method.

[keradus]

please don't

[onlinesid]

Hi sorry I'm new to this.

You mean don't change the .gitignore?

[keradus]

not touching .gitignore itself, adding these things.
wanna ignore it then add it to your global ignore list

[bighappyface]

@onlinesid merge conflicts now as of version 1.4.0

I am in agreement with @keradus on ignoring those files globally on your workstation, and would recommend installing composer globally as well so you don't have to ignore the phar at all.

[keradus]

weird indent, ' instead of ", => should be aligned

[onlinesid]

Thanks for the feedbacks, I have done those things now.

[keradus]

why not aligned ?

[bighappyface]

@keradus thanks for the updates.

Your example output:

{
  "status": "error",
  "message": "JSON does not validate",
  "errors": [
    {
      "property": "firstName",
      "message": "must be at least 1 characters long",
      "type": "minLength" // this is what I've added
    }
  ]
}

I don't find the current failure/error messages inadequate or confusing. I am unsure what the goal is. Better i18n?

[onlinesid]

Hi @bighappyface

No issue/confusion with the current error messages, it's mainly about having a way to detect an error type without having to parse the error message.

It's also about making it easier to customize the error messages. For example "does not have a value in the enumeration" isn't really something that any web/app end user should see :)

I'm using json-schema to validate form on client side and reusing it for server side.

[bighappyface]

For the client-side validation, are you making a call back to the server to use this validator, or are you using a client-side validator?

[onlinesid]

No call back to the server. A javascript json schema validator on the client/browser side, and this library for validation using PHP on the server side. So the same json definition is used by client side and server side.

[bighappyface]

Being you are using a client-side validator, why is the messaging format of the server-side validator a concern for the end user as you described? Doesn't the client-side validator facilitate validation messaging for the end user?

[onlinesid]

Hi @bighappyface

First of all, server side validation must always exist, any input from the client side must still be validated regardless if validation occurs on the client side or not. So in reality validation happens twice, on the client side and on the server side.

Traditionally, with framework like Symfony for example, on the server side, we would use a form builder. The form builder can add html5 limited validation on the form (like input is required for example).

However I'm trying to move away from that. I think a better way to build a web app is to develop RESTful API for the server side and use angular (or other js framework like it) on the client side. This is a much cleaner model ... more decoupling (or even better if it's 100%) between client and server side. But to achieve that I still need to do validation on the client as well as server side ... this is why I'm using json schema so that the validation for the client side can be replicated in the server side with very little effort and no duplication

[bighappyface]

@onlinesid,

There is another PR very similar to this, #122. Have you seen it?

I think there is value in what both this and the other PR are trying to achieve; however, considering that #122 was submitted first, and addresses the concerns for more expressive data about the failure than just the constraint name, I think it is fair to determine some kind of reconciliation between them.

Here are a couple of other validators that use the same test suite:

https://github.com/ruby-json-schema/json-schema
https://github.com/geraintluff/tv4
https://github.com/geraintluff/jsv4-php

I believe we should adopt something along the lines of what @geraintluff has done in the error message format. Below is a possible addition to the work you have already done:

JsonSchema\Constraints\StringConstraint

public function check($element, $schema = null, $path = null, $i = null)
{
    // Verify maxLength
    if (isset($schema->maxLength) && $this->strlen($element) > $schema->maxLength) {
        $this->addError($path, "must be at most " . $schema->maxLength . " characters long", 'maxLength', $schema);
    }

    //verify minLength
    if (isset($schema->minLength) && $this->strlen($element) < $schema->minLength) {
        $this->addError($path, "must be at least " . $schema->minLength . " characters long", 'minLength', $schema);
    }

    // Verify a regex pattern
    if (isset($schema->pattern) && !preg_match('#' . str_replace('#', '\\#', $schema->pattern) . '#', $element)) {
        $this->addError($path, "does not match the regex pattern " . $schema->pattern, 'pattern', $schema);
    }

    $this->checkFormat($element, $schema, $path, $i);
}

JsonSchema\Constraints\Constraint

public function addError($path, $message, $constraint = '', $schema = array())
{
    $schema = array_intersect_key((array) $schema, array_flip(array($constraint)));
    $this->errors[] = array(
        'property' => $path,
        'message' => $message,
        'constraint' => $constraint,
    ) + $schema;
}

Sample Output

array (
  'property' => 'example',
  'message' => 'must be at least 5 characters long',
  'constraint' => 'minLength',
  'minLength' => 5,
),
array (
  'property' => 'example',
  'message' => 'does not match the regex pattern ^[\\d]',
  'constraint' => 'pattern',
  'pattern' => '^[\\d]',
)

By leveraging the name of the constraint, we can filter the schema to provide the data used in the constraint. Without this information, the constraint name alone is helpful but very limited, and I see something like this eventually happening when you consider both this PR and #122.

Thoughts?

[onlinesid]

Instead of the suggested:

addError($path, $message, $constraint = '', $schema = array())

I just added optional $error_info:

addError($path, $message, $constraint='', $error_info=null)

For several reasons:

• $constraint doesn't necessarily mean there's $schema->$constraint. We should not be making that assumption
• Longer term I think addError should take an object instead of more and more arguments. When we keep needing to add argument to a method, it's often means we need to turn the arguments into an object instead.

I'm thinking ConstraintErrorInterface and ConstraintError, but I don't have the time to do that at the moment as I have to work on other part of the project, but I will be coming back to this at some stage.

[onlinesid]

Also, I'm not sure what the rebase flag is for, there's 0 commit for me to rebase?

[onlinesid]

I squashed my commits but I made it worse here, it now says 14 commits!

Do I need to 'Close and comment' this pull request then create another one?

[keradus]

After squashing or rebasing you need to push using the --force argument. Instead of that you pull the origin and merge it again.
Do what you need with the branch and push with force, no need for recreating pr

[onlinesid]

Thanks for that, it works!

I can't squash it to 1 though ... I think it's because I merged / rebase after that first one push

[keradus]

Oh... your changes are on master... please, avoid that in future. What is more you work on master branch, that could be the reason of your problems here.
You should not modify original branches on forked repos.

It may help:

git checkout upstream/master
git branch -D master
git checkout -b master
git cherry-pick --no-commit de769e21ecd10919e059ccfdfd619bff8a954ce4
git cherry-pick --no-commit 4cd973e6cbd90876f08c02d968c3867175bf568f
git commit -m "Added optional arguments \$constraint and \$error_info in addError()"
git push -f

[onlinesid]

I've moved my commits to another branch, not on master now, please review them on #141 instead.