#240 Fix RefResolver and make it compatible with draft-04 #245
Conversation
| @@ -7,3 +7,4 @@ coverage | |||
| .settings | |||
| composer.lock | |||
| docs-api | |||
| .idea | |||
There was a problem hiding this comment.
You should add this to a global ignore: http://stackoverflow.com/questions/7335420/global-git-ignore
There was a problem hiding this comment.
I agree. I will update the code and revert this change!
|
@araines what do you think of this alternative approach to resolving the ref issue (pun intended)? |
jojo1981
force-pushed
the
feature/ref-resolver
branch
6 times, most recently
from
5358e30 to
ae08d20
Compare
|
@bighappyface last weekend a good friend of mine also a great programmer was at my home. We both have studied again on my solution and have improved the code. I have squashed the commits and did a new push. Can you please re-check this code? |
|
@jojo1981 sorry I didn't chime in on this sooner. There is a set of ref test provided by the JSON Schema test suite that are currently skipped because of the missing functionality. You can enable those tests by omitting this line: I pulled this PR down and ran the test suite with those ref tests enabled and ran into a lot of failures. I would expect those tests to pass first and foremost to improve our draft 4 coverage. |
|
@bighappyface I'm happy to review this once its working with the tests, also I'm not precious about which solution gets used moving forward - so long as the library is improving, thats all that matters! |
|
@bighappyface Good point I will try to fix the code so the draft 4 tests will also work. |
jojo1981
force-pushed
the
feature/ref-resolver
branch
from
ae08d20 to
4a2805b
Compare
|
@bighappyface You're right, those tests also needed to be executed and of course pass successfully. I will fix it and push it again. |
jojo1981
force-pushed
the
feature/ref-resolver
branch
from
4a2805b to
351bffd
Compare
|
Updated the code and enabled multiple draft 3 and draft 4 tests. |
|
@bighappyface I have ommitted more than only this line: https://github.com/justinrainbow/json-schema/blob/61a95a151aac86eaa9bbd31311d3d12a78c971b8/tests/JsonSchema/Tests/Drafts/Draft4Test.php#L21 |
jojo1981
force-pushed
the
feature/ref-resolver
branch
from
351bffd to
604d4d0
Compare
|
Rebased from |
|
@araines I agree! Same for me I don't care which solution is moved forward as long as the library improves. |
|
Picking it up and taking a look :) |
|
@jojo1981 - Interesting how we independently have approached the problem in a very similar manner - I guess that is probably a positive thing! However, I think this solution has the edge - it feels a bit cleaner and neater than the one I did in #210 . The only downside to this one is that we are changing the documented way of using the library, which means it will break for more people (rather than those who had custom-extended it). However, having looked over this and weighed it up, I think this represents the better solution, even though it will cause slightly more pain for people using this library elsewhere. @bighappyface I'd personally recommend taking this PR over #210 |
jojo1981
force-pushed
the
feature/ref-resolver
branch
from
604d4d0 to
6e57bc5
Compare
|
@mirfilip would you mind taking a look at this? |
|
@araines I agree it's not backwards compatible and can only be used in the next major (I think this is a good step for this library). Thanks for your positive remarks! For now I will leave this PR and do no forced pushed anymore. I'm waiting for review comments. So people let's start reviewing! |
| foreach ($jsonPointer->getPropertyPaths() as $path) { | ||
| if (is_object($refSchema) && property_exists($refSchema, $path)) { | ||
| $refSchema = $refSchema->{$path}; | ||
| } elseif (is_array($refSchema) && array_key_exists($path, $refSchema)) { |
There was a problem hiding this comment.
How does this work with getting the last array element (a tilde)? Spec here: https://tools.ietf.org/html/rfc6901#section-4
I couldn't see a test for it, and I'm trying to wrap my head around how this bit works.
There was a problem hiding this comment.
The JsonSchema\Entity\JsonPointer should implement the logic as describe in: https://tools.ietf.org/html/rfc6901#section-4
There was a problem hiding this comment.
I see that is the intention, but as I understand it the JsonPointer only splits the path elements, meaning you'd end up with a list of the various parts. That is fine, and is used here, but it looks like at that point the $refSchema would be an array and the code highlighted would be executed, looking for the path of ~ in the reference schema - I don't understand how that would ensure we end up with the last element of the array (I may well be missing something).
Either way, I would suggest adding a test for this particular test case to ensure it works and in future the functionality is not broken when further work is done.
There was a problem hiding this comment.
I agree I have to re-check this part. I have re-runned all currently existing PHP Units tests and see this part is not fully covered (lines 108, 109 and 110 are not hit by any test).
Also I think It's better to throw an exception when $refSchema can not be fully resolved. Now the current $schema will be returned (see line 110).
Also I will try to provide a test which coverage line 108.
There was a problem hiding this comment.
After some more investigation is see line 108 is dead code. So I will fix this.
|
@jojo1981 given the Interfaces are now defined and are nice and simple, does that mean the other public methods of UriResolver and UriRetriever should potentially be made protected/private - or perhaps the interfaces should be extended to include these if they are necessary? If my memory serves me correctly, there is a fair bit of duplication between the UriResolver and UriRetriever which we can perhaps slim out whilst you are making public interface breaking changes anyway? |
|
@araines I agree, but suggest doing that in a separate issue. This PR is unintentional getting bigger than I at first expected. Because this is a library it's always a good idea to use simple and clean interfaces. Only have some public methods which are having a clear purpose. Maybe after the complete refactoring we should use annotations on public methods like: |
|
@jojo1981 - fair enough, i'm sure it can be handled as a separate matter. |
|
Updated the code but with an extra commit. When this review is finished and can be merged I will squash all the commits and force push again. For now while active in the review process I think is handier to have separate commits. |
|
@mirfilip would you mind also taking a look at this, just as @bighappyface asked? |
|
@bighappyface did you re-check this PR? |
|
@jojo1981 sorry, I have been monitoring the thread but haven't jumped into the full review quite yet. I'll jump on it this weekend for sure. |
|
@bighappyface No problemo! |
| } | ||
|
|
||
| protected function resolveRefSegment($data, $pathParts) | ||
| private function getReservedKeysWhichAreInFactSubSchemas() |
There was a problem hiding this comment.
Interesting name here. Very matter-of-fact. I don't have a problem with it, just commenting on the choice of words/phrasing.
There was a problem hiding this comment.
:-) Yeah I know. A better name is welcome. So if you have a suggestion?
|
Processed the remarks and added an extra commit. |
|
@bighappyface When we good to go just let me know so I can squash the commits. |
|
@jojo1981 go ahead and squash the commits. I'm slammed today but I will work on the merge and bump this evening. |
jojo1981
force-pushed
the
feature/ref-resolver
branch
from
cf3e9ec to
e265e9f
Compare
|
@bighappyface I've squashed the commits as you can see. |
|
+1 |
|
@bighappyface Thx! |
The public API of the RefResolver will be changed. Suggesting to take this PR in a new Major version of this library.