Skip to content
This repository was archived by the owner on Apr 20, 2021. It is now read-only.

Fix json schema validation #90

Closed
wants to merge 1 commit into from
Closed

Fix json schema validation #90

wants to merge 1 commit into from

Conversation

EmmanuelVella
Copy link
Contributor

Currently Sanpi\Behatch\Json\Json and ```Sanpi\Behatch\Json\JsonSchemaobjects are given to theJsonSchema\Validator::check` method.

It think this is incorrect as it should be the content value of this objects that should be used.

This PR fixes this.

@sanpii
Copy link
Member

sanpii commented Oct 29, 2014

It think this is incorrect as it should be the content value of this objects that should be used.

This is done by the __toString method. I prefer this solution instead of expose the content attribute.

@EmmanuelVella
Copy link
Contributor Author

Ok. However, it currently doesn't work. I will take time to find another solution.

@sanpii
Copy link
Member

sanpii commented Oct 29, 2014

However, it currently doesn't work.

I have no error with the current test suite. Have you an example?

@EmmanuelVella
Copy link
Contributor Author

In my project it doesn't throw errors even if there are some (for ex if a required field is missing).

@EmmanuelVella
Copy link
Contributor Author

Let me take a look again, i will give you more details asap ;)

@EmmanuelVella
Copy link
Contributor Author

At this point https://github.com/justinrainbow/json-schema/blob/1.3.7/src/JsonSchema/Constraints/Undefined.php#L119 the schema object I have is equal to someting like that :

Sanpi\Behatch\Json\JsonSchema {
  content: {
    "type": "object"
    "$schema": "http://json-schema.org/draft-03/schema"
    "required": true
    "properties": {
      "id": {
        "type": "integer"
        "required": true
      }
   ...

$schema->required is not defined, so it doesn't validate the required properties.

@sanpii
Copy link
Member

sanpii commented Oct 29, 2014

"required": true

required is a array of required properties: http://json-schema.org/examples.html

@EmmanuelVella
Copy link
Contributor Author

Ho ok, I will update it. However, it's still not accessible, as it's a child of the content attribute !

@stephpy
Copy link
Contributor

stephpy commented Oct 31, 2014

👍 , i'm having same behavior.

@ecentinela
Copy link

Same problem here and this modification is working 👍

@ecentinela
Copy link

Please, merge this ASAP.

@stephpy
Copy link
Contributor

stephpy commented Nov 13, 2014

ping @sanpii.

Please. :)

@gmorel
Copy link

gmorel commented Nov 21, 2014

👍

@stephpy
Copy link
Contributor

stephpy commented Dec 2, 2014

@sanpii May be you need some help to maintain this library ?

@stephpy
Copy link
Contributor

stephpy commented Dec 2, 2014

I guess https://github.com/sanpii/behatch-contexts/pull/87/files is more complete than this one. Could we close this one and merge the #87 ?

@stephpy
Copy link
Contributor

stephpy commented Dec 2, 2014

And it has to be done on master and dev-behat2.x

@sanpii
Copy link
Member

sanpii commented Dec 3, 2014

Merged, sorry for the delay.

@sanpii May be you need some help to maintain this library ?

Help is welcome, you can follow (and contribute) to #91

@sanpii sanpii closed this Dec 3, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants