Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] refactor FormatConstraint, use factory approach #280

Closed
wants to merge 2 commits into from
Closed

[WIP] refactor FormatConstraint, use factory approach #280

wants to merge 2 commits into from

Conversation

steffkes
Copy link
Contributor

Based on our discussion at #266 a very quick & dirty shot to show the approach i'd thought of.

@steffkes
Copy link
Contributor Author

even w/o waiting for the build to end, i'm pretty sure it will fail on at least one version - i've put that together on a system that runs PHP 7. which allows a bit more / newer feature than the 5.3 release we need per our requirements

@steffkes
Copy link
Contributor Author

didn't rerun the tests after the latest change, which explains why it failed entirely -.- my bad, fixed pushed already.

@narcoticfresh
Copy link
Contributor

@steffkes sorry for the late checking..

yea, that looks feasable.. the only thing is that you create many throw-away instances, don't know if this could have much impact and how GC handles it..

maybe make a local $instanceCache array where you instanciate (and put them there) the first time, then always call validate() if it exists (which then needs an Interface for the Validator class) in the instance in the array..

We have call graphs that even call ObjectConstraint->check() around 2000 times - Format checks more by factors then of course - so small stuff like this quickly sums up.

@digitalkaoz
Copy link
Contributor

@narcoticfresh check again with the latest PRs merged...should be much better now

@bighappyface
Copy link
Collaborator

@steffkes is this PR still in play?

@steffkes
Copy link
Contributor Author

@bighappyface thanks for asking, that's a valid question: i guess not. not actively working on a project anymore that uses json-schema that much. so there's currently not much time to work on this PR - i'm sorry for that.

@bighappyface
Copy link
Collaborator

@steffkes no worries. Thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants