New way of handling references

[psafarov]

I had issues compiling schemas containing circular references with RefResolver, so the way of handling references was changed, now Validator contains SchemaStorage object inside, which contains all schemas and can resolve references when it's needed. The pros of this approach are:

• lazy loading, if some schemas are not used, they won't be loaded
• no infinite loops, thanks to lazy evaluation again
• api becomes kind of simpler

<?php
// You can pass reference which will be handled by SchemaStorage::resolveRef
$validator->check($data, (object)['$ref' => 'file://path/to/your/shcema'])

[bighappyface]

Please omit this change and add this to a global .gitignore https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

[bighappyface]

@psafarov please rebase this on master

[bighappyface]

This approach prevents reuse of an instance of SchemaStorage by only allowing a user to set the retriever or resolver only during instantiation. The goal of simplifying instantiation SchemaStorage is achieved but I am not sure that is a problem nor am I sure it is germane to the main goal of handling circular references.

I am not sure I think this is an improvement but I won't hold up this PR for this if the community supports this move.

[psafarov]

@bighappyface you're right, it's not only about circular references. I also wanted to simplify the interface. Now there is no RefResolvers you need to know about, until you have to

[ElectroLutz]

Any info on this one? I would like to see this in a release.

[bighappyface]

@FlorianSW @steffkes @mirfilip would you mind giving this PR a review? This looks to be all but certainly a major update that would push us to 3.0.0.

[steffkes]

wouldn't it be kinda easier to move $schemaStorage to the end of the list? if we'd append it to the existing list, we wouldn't introduce a BC-Break at this point.

[FlorianSW]

+1 This seems like an unnecessary breaking change. However, if this is already a breaking change (and in this core functionality), it probably doesn't make sense to reorder arguments, if it makes sense. Unfortunately, I don't see a good reason to do it here? :)

[psafarov]

@steffkes @FlorianSW I think, you're right, it's better to be in the end

[steffkes]

just had a first glance over the changes - did add a comment where i thought it's need. will have a more detailed look during this week - given this PR is still open and not already merged

[FlorianSW]

We've a getter method for $uriRetriever, wouldn't it make sense to have one for $schemStorage, too?

[FlorianSW]

Added some comments. For now it looks sane and reasonable, but someone (:P) should test it in a real life example, just to make sure it's working like expected :)

[psafarov]

@FlorianSW I can't say It has no bugs for sure, but this code is used in production in a company, no problems so far

[bighappyface]

@psafarov I have merged a few PRs this morning and I put this PR out of sync. Would you please rebase on master?

[psafarov]

@bighappyface done

[bighappyface]

@psafarov sorry for the confusion. It looks like you merged master in okay but rebasing is different than merging master in. There should only be one commit in this PR.

[psafarov]

@bighappyface fixed.

[bighappyface]

With community support I'll merge it into master. The plan is to merge it and then bump to 3.0.0 so indicate BC-breaking changes.

Other PRs I have merged have probably already put us in a position to bump to 3.0.0 anyway.

[bighappyface]

Based on the passing tests, the account of this code being in production, the need for verified handling of circular references, and what feels like community approval and support, I am going to move this along and am going to cut release 3.0.0.

+1

[neoascetic]

👍