Skip to content

Conversation

niconoe-
Copy link
Contributor

Replay of #581

@kerunaru
Copy link

kerunaru commented Oct 3, 2024

Any plans merging this PR?

@DannyvdSluijs
Copy link
Collaborator

Any plans merging this PR?

Short answer: Not yet.

Long answer: We've been putting effort in reviving this repository, which meant a lot of cleanup, triage en updating to current PHP versions. At this point we are working on those items. Adding additional drafts is definitely our goal but not our priority at this point in time.

@MielPoule
Copy link

+1 I would love this functionality ❤️

Comment on lines +176 to +180
// minProperties and maxProperties constraints only applies on objects elements.
if (!is_object($element)) {
return;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary anymore. The library already has this validation.

Comment on lines +12 to +16
const ALWAYS_FAILS = 'alwaysFails';
const ANY_OF = 'anyOf';
const CONDITIONAL_IF = 'if';
const CONDITIONAL_THEN = 'then';
const CONDITIONAL_ELSE = 'else';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constants are public. The visibility has been added in the library. It should be added in this fix as-well.

Comment on lines 28 to 40
],
"require": {
"php": ">=5.3.3",
"ext-json": "*",
"marc-mabe/php-enum":"^2.0 || ^3.0 || ^4.0",
"icecave/parity": "1.0.0"
},
"require-dev": {
"friendsofphp/php-cs-fixer": "~2.2.20 || ~2.19.0",
"json-schema/json-schema-test-suite": "1.2.0",
"json-schema/json-schema-test-suite": "2.0.0",
"phpunit/phpunit": "^4.8.35"
},
"extra": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those changes are not needed since it has been done in the master branch

@DannyvdSluijs
Copy link
Collaborator

DannyvdSluijs commented Sep 15, 2025

@Norda-AD thank you for all the feedback on the PR. In all honestly I’m not sure if the PR in this form will ever get merged. I really like your efforts but also don’t want to disappoint about the PR never making it into main( currently still called master)

The difficulty lies in only adding if then else logic in the current flow. With draft3 and 4 being very alike adding support for the if then else would break those drafts as it would add more keywords then documented.

Thinking ahead I would like to add support for more drafts (including the one that has the if then else support) like this PR which is currently pending for Draft 6

@Norda-AD
Copy link

@DannyvdSluijs Thank you for your feedback. I started digging into this PR since I need this to validate my own code. I understand that you have other priorities. In the meantime, I am trying to decorate the class to fit my needs.

I am currently working on adapting this PR to the new code. Basically fixing my comments.
Are you interested if I continue or should I just not bother?

@DannyvdSluijs
Copy link
Collaborator

@Norda-AD I've just merged and released the draft-6 support. So this makes draft-7 next. I'll close this PR for now as I'll be opening a draft 7 PR in the upcoming days.

Your feedback has been very helpful in this PR. I'll use it when doing the fresh draft-7 PR. Thanks for your help.

@Norda-AD
Copy link

@DannyvdSluijs Can't wait to see it. Thank you for the feedback!

@DannyvdSluijs
Copy link
Collaborator

@DannyvdSluijs Can't wait to see it. Thank you for the feedback!

You can find it under #847

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants