Fix #93 ($ref to local definition not working) #182
Conversation
|
+1 This is the best start to resolving this (pun intended). @araines thanks a ton for using the standard test suite and getting this most of the way there. With the high demand for this support, I think it is good to move forward. @alecsammon @localheinz @GrahamCampbell would you please review? |
|
It would be really nice to have this working. |
|
@graste may I have your help with code review? I prefer two peer reviews prior to merging a PR. |
|
Okay, I checked out master and applied this PR. It brings down the draft4 ref test failures from 8 to 3 as advertised. What I noticed is, that issue #93 is working fine even without this PR – see this gist. After having a look at the code I must say, that I've got the same feeling as @araines has with his PR: The solution might not be ideal as it feels a bit like not reusing stuff like What I noticed though when testing a |
|
@graste thanks for the review. I am merging and rolling v1.4.5 |
Fix #93 ($ref to local definition not working)
| $validator = new Validator($checkMode); | ||
|
|
||
| $validator->check(json_decode($input), json_decode($schema)); |
There was a problem hiding this comment.
This is a clear sympton of BC Break
There was a problem hiding this comment.
I don't follow. Are you referring to the addition of RefResolver in the lines above or this specific line?
Another attempt at getting issue #93 sorted - thanks to @co3k and @stoiev for putting in the initial leg work. Based my initial work on #119, but actually got the tests running properly with the resolver too and the official test pack. This reduces the 8 failures mentioned by @bighappyface in #119 down to 3.
I'm not sure this is quite the right way to fix it from an architectural point of view, but it does improve the capability of this library, at least.
In order to get the remaining 3 failing tests to pass, I suspect there will need to be a fairly big rewrite. The main problem is around that of recursion - self-referencing documents will simply cause the current design to fail. But that should be the subject of another issue.