Array properties with array items listed in enums does not validate properly

[malinink]

Approve example

close #547

[malinink]

@erayd There is two more ways to fix issue:

• call 'enum' check directly from CollectionConstraint perfomanse optimisation block
• remove optimisation block at all

If any of above ideas are correct, just approve me and I will update PR.

[malinink]

@justinrainbow @erayd Is there any chance that PR will be applied? Could you please add some reaction for that?

[erayd]

@malinink I missed this PR sorry - will code review this weekend. Once that's done, @bighappyface will need to merge it.

@justinrainbow is no longer involved with this project, and hasn't been for years, even though it's hosted on his GitHub account.

[rbchasesc]

@bighappyface, @erayd, Any chance of this getting approved and merged soon? We have several occurrences now where this problem is impacting our validation.

[rbchasesc]

The following change works as an alternative approach to the changes to NumberConstraint and StringConstraint as the alternative approach suggested by @erayd in the pull-request comments:

diff --git a/src/JsonSchema/Constraints/CollectionConstraint.php b/src/JsonSchema/Constraints/CollectionConstraint.php
index 49841a4..b8200f7 100644
--- a/src/JsonSchema/Constraints/CollectionConstraint.php
+++ b/src/JsonSchema/Constraints/CollectionConstraint.php
@@ -84,6 +84,9 @@ class CollectionConstraint extends Constraint
                     $typeValidator->check($v, $schema->items, $k_path, $i);

                     $validator->check($v, $schema->items, $k_path, $i);
+                    if (isset($schema->items->enum)) {
+                        $this->checkEnum($v, $schema->items, $k_path, $i);
+                    }
                 }
                 unset($v); /* remove dangling reference to prevent any future bugs
                             * caused by accidentally using $v elsewhere */

[elliotchance]

I've also run into this issue, with:

    "broadcast_level": {
      "type": "array",
      "items": {
        "type": "string",
        "enum": [
          "now",
          "order"
        ]
      },
      "minItems": 1
    },

Always validates with an invalid enum value. However, if I change the type string to any it works.

[erayd]

@rbchasesc I'm not happy with this PR as currently written (because it calls general validation from a type-specific area, and has redundant code - both of which are things which I've been trying to clean out of this library for a while). I'm looking at this now and will propose an alternative shortly - stand by.

[erayd]

@rbchasesc Could you please confirm whether you are seeing this bug in 6.0.0 (the current master branch)? I've applied your test cases to it, and they seem to pass correctly, indicating that this bug is probably already fixed and we just need to backport something.

As a general rule, new PRs should not be opened against the 5.x.x branch. Bugfixes are sometimes an exception - depending on whether it's a fix that's also relevant to master, or if it's exclusively a 5.x.x bug.

[erayd]

@rbchasesc Upon further investigation, I think this whole issue is being caused by the optimisation block, which I eliminated in 6.0.0 in order to resolve a number of other bugs. However, I didn't backport that to 5.x.x for some reason - am looking into why... Apparently I just haven't gotten around to rolling a backport release since I made that change. Stand by.

[rbchasesc]

@rbchasesc Could you please confirm whether you are seeing this bug in 6.0.0 (the current master branch)? I've applied your test cases to it, and they seem to pass correctly, indicating that this bug is probably already fixed and we just need to backport something.

As a general rule, new PRs should not be opened against the 5.x.x branch. Bugfixes are sometimes an exception - depending on whether it's a fix that's also relevant to master, or if it's exclusively a 5.x.x bug.

@erayd I only tested this as a fix to the 5.2.7 tag as we are currently using that version as our "stable" deployed version of this library. We haven't utilize 6.0.0-dev or master as there is no officially tagged 6.0.0 release....yet 😄 . Standing by for what ever comes out of your backport effort.

[erayd]

@rbchasesc Fair call :-). 6.0.0 is missing enough stuff (mainly documentation and a couple of significant bugfixes) that it's not properly ready to release, but it should be fairly stable, and does have a number of advantages that may make it preferable to 5.x.x in some cases. However, I'll continue to backport stuff into 5.x.x until we have an actual release, so if you'd rather stick with that, go for it 👍.

I've backported the relevant changes to 5.2.8 for you - just waiting on @bighappyface to merge it.

Could you please create a new PR (or change this one) against the master branch, which includes the test cases from this PR? The tests as you have them are fine, although a couple of brief comments would be useful to clarify what is being tested. The logic changes in this PR are unnecessary, and can be discarded.

[rbchasesc]

@erayd Awesome news on getting this into 5.2.8! Will keep an eye out for the merge and the new tagged release version.

I'll see if I can get to the tests PR tomorrow as it's getting later over here.

Thanks again for jumping on this one.

[rbchasesc]

@erayd Per your request last week, went with a new PR #559 that has several test cases and is targeting master. You and @bighappyface can decide where you want to go from here.

Huge thanks to you and @bighappyface for getting that backport done, merged and v5.2.8 released.

[erayd]

@rbchasesc Awesome - thanks for that 👍.