feat: Add support for Draft 6 in NumberConstraint

[llupa]

I am opening this PR because I first encountered it as reported in api-platform/core#6041. I have read and seen either via commit history or issues (such as #699) that this repository has very limited people capabilities.

I tried to keep this as short as possible to offer a Draft 4 and Draft 6 compliance for Numbers->Range case without changing too much. Ref: Draft 6 Backward-incompatible changes.

I don't know if an is_bool is fine vs filter_var, I am more than happy to apply changes as per your feedback.

[erayd]

Thank you very much for this PR! As you've noticed, we are indeed extremely short on people with available time.

A few points:

1. Could you please set this PR against the master branch, not 5.x.x. The 5.x.x branch is draft-04 only, and draft-06 stuff goes in master.
2. Draft-06 and draft-04 are mutually exclusive regarding the exclusiveMinimum and exclusiveMaximum keywords unfortunately, as both versions of the spec mandate a type, but the type differs.
3. I appreciate what you are trying to do with the version inference thing (detecting draft-4 vs draft-06 behaviour by looking at the type). It's not really appropriate (because it results in things passing validation which shouldn't), but given that we do this elsewhere already with other keywords, I'm inclined to say that we should just accept it here too. Users that want strict validation of the type against the spec version can enable CHECK_MODE_VALIDATE_SCHEMA. With that said, this behaviour does need to be documented in the README, as it's clearly not spec-compliant.
4. I'm unsure whether we should also backport this into 5.x.x, and currently leaning towards not, particularly noting that the master branch is essentially feature-complete for draft-06 - it can be (and is) used in production now for most use-cases. My primary concern is that backporting this could result in validation behaviour changing for existing deployments in violation of the v4 spec, and I really don't want to risk that on something as widely-used as this library is. Happy to be convinced otherwise if you have a good argument for why we should backport it regardless.

@Seldaek Do you have an opinion on (4)? If this violation would trouble composer (or if you aren't sure), then I won't backport it. You're too important to break ;-)

[llupa]

Hey! Thank you for looking at this so soon. Since you used numbers, I will use numbers too 😄

1. I first encountered this issue in API Platform, their schema generator moved on to greener pastures and suddenly my regression tests started breaking. The tests broke in a dependabot PR and that had me look deeper. I don't know if you read the linked issue or not, but I can also just override my property to Draft 4 and be done with it. That did not feel like the right solution.
2. The reason I sent it to 5.x.x is because API Platform does not require master. To add I have been working with Symfony for 10+ years now so my first instinct was to make it backward compatible and forward compatible but as I am writing this I am noticing that I did not trigger any deprecation for Draft 4 checks. I have no strong opinion where this should go and I will do whatever is your final decision 👌
3. I can open a different PR for CHECK_MODE_VALIDATE_SCHEMA too.

From @erayd comment above it looks like it is safe to require master as a version rather than ^5, would this be too much for API Platform @soyuka ?

[Seldaek]

I'm really not familiar enough with the draft 04 vs 06 changes, but I doubt this impacts Composer. We only have a few integer types defined and none of them have additional restrictions.

From a "requiring master" perspective, it'd be good if we changed https://github.com/justinrainbow/json-schema/blob/master/composer.json#L41 to be 6.x-dev @erayd - that way projects requiring the draft 06 could require this package as ^6@dev which is much better than pinning to dev-master, as dev-master will eventually become v7. ^6@dev will require dependents of dependents to also allow this project in a dev version until there is stable release, so it's also not super convenient, but at least it's semantically correct from a versioning point of view.

[soyuka]

I'd be really nice to move forward on this and trying to align this software versions with each draft would be a must! Just target master for this patch.

As you've noticed, we are indeed extremely short on people with available time.

Do you need help for maintenance? From what I see the 6.x-dev (master) branch could add the draft 6 features and php 8 as a minimum requirement?

[erayd]

Do you need help for maintenance?

Always. Much to do, and no time to do it in.

From what I see the 6.x-dev (master) branch could add the draft 6 features...

The master branch already implements draft-06, and should be essentially feature-complete. This PR caught something that both we and the unit tests missed, but is an exception rather than the rule.

...and php 8 as a minimum requirement?

Absolutely not. This library needs to be backwards-compatible with much older versions of PHP unfortunately - we support all the way back to PHP 5.3. Requiring PHP 8 as a minimum version for the v6 branch will never happen; it would break too many dependent projects.

[llupa]

@erayd I can update this PR against master now. Do you want to keep both Draft 4 and 6 or just move to 6 and never look back?

[erayd]

Do you want to keep both Draft 4 and 6 or just move to 6 and never look back?

Both, please. Many people use v4 still.

[Seldaek]

Requiring PHP 8 as a minimum version for the v6 branch will never happen; it would break too many dependent projects.

@erayd I am sure this is about more than Composer, but from my/Composer perspective, PHP 7.2+ is alright now, as long as v5 is kept maintained for critical fixes (but I really don't expect much of that, and I can do it myself if needed), and most likely the next jump we would do would be PHP 8.1+ to be able to use Symfony components in their 6.4.* instead of pinning to 5.4.*. I don't expect this to happen for another year or so tho as it would leave too many people stranded right now, there's quite a bit of PHP 7.4 usage still :/

[soyuka]

My opinion on this is that the Draft 6 version of this package should move on at least php 7 or 8 version, I don't understand why supporting older versions would be a benefit (as it's a maintenance burden anyway). You could always use the 4.x version of this package with older php versions right?

To follow @llupa's enhancement here, you should target master for this patch, where master would be implementing Draft 6. If you want Draft 4, just install 4.x or 5.x version of this package right?

[erayd]

...from my/Composer perspective, PHP 7.2+ is alright now, as long as v5 is kept maintained for critical fixes (but I really don't expect much of that, and I can do it myself if needed)

If you are happy to do backports, then I'm OK with moving the minimum for master to PHP 7.2 at some point. I don't want to leave those still using legacy PHP 5.x applications out in the cold though, as I'm aware of a number of them that use the master branch in order to obtain draft-06 support - so while the 5.x.x branch might be enough for composer's legacy requirements, it isn't the full picture.

If we go down that path, I'd like to see an additional "fixes only" branch forked off of master that will maintain PHP-5.6+ support (perhaps as a v6 release branch, with master becoming v7). Somebody will need to take on the responsibility of backports for that branch, as I don't have the time for more than the occasional item.

If there is appetite to pick up significant development work on this library that necessitates a higher minimum version, I'm open to persuasion. However the reality is that it's currently on life-support, so such a change would be a rather arbitrary restriction - I'm not seeing that the gains are worth it at this point. Feel free to convince me otherwise 🙂.

...you should target master for this patch, where master would be implementing Draft 6. If you want Draft 4, just install 4.x or 5.x version of this package right?

No. The master branch was always intended to support both, and has been used accordingly. Suddenly dropping support for draft-04 isn't acceptable. Better separation between versions of the spec would be a good thing though, ideally making proper use of the $schema keyword to set the version, rather than relying on kludgy heuristics to infer the correct behaviour as it does currently.

[erayd]

You could always use the 4.x version of this package with older php versions right?

v4 of this library has been unmaintained for many years. It should be avoided; there are a number of known significant problems with it. For users who require an older version of PHP, and for whom draft-04 is acceptable, then the v5.x.x release branch is usually the right choice.

For users who require draft-06, regardless of PHP version, v6.x.x (master branch) is currently their only option.

[llupa]

@erayd I moved the PR to master, but GitHub Actions did not pick up my force push. I wanted to have Continuous Integration at least run once. Should I close this PR and open a new one to trigger it? Add a fake commit?

Alternatively here is the commit and GA over my fork.

[DannyvdSluijs]

@llupa thanks for your efforts and sticking strong with all the rebasing and other comments. In an attempt to revive this library I'm doing some triage on the issues and pull requests. With regards to your commit not being picked up by GitHub workflow I can recommend doing a git commit --amend --no-edit which will create a new commit hash which you can then do a git push --force-with-lease on. That should do the trick for the workflow.

@erayd I see you mentioning:

The master branch already implements draft-06, and should be essentially feature-complete. This PR caught something that both we and the unit tests missed, but is an exception rather than the rule.

Does this perhaps mean that the json schema test suite is really do for some updating? Also since the version 1.2.0 which is set in composer.json explains that is has full coverage on draft3 and draft 4. Version 2.0.0 mentions draft 6 and 7 as well.

[llupa]

@DannyvdSluijs funny you wrote today. I was thinking about this PR over the weekend 😅

Workflows waiting to be started 👍