Bugfix: Problem with items being an array of schema #521
Conversation
I think this test should be working, just like the 20 or the 21, it is quite similar but still produces a wrong result.
|
My colleague @alex-pex found the bug and it fits perfectly with the other test cases |
wlalele
changed the title
There was a problem hiding this comment.
Please number the test correctly - the numbers refer to the actual order of the test cases, and are there to facilitate debugging. They aren't just decorative.
I would also like to do some additional testing regarding recursive schemes and interaction with the default function before this gets merged - it's somewhat complex, and if I recall correctly not quite fully covered by test cases yet, so need to make sure this doesn't break anything. There is an open bug report regarding defaults within array items.
|
@erayd did you make any progress solving this issue? |
| @@ -169,6 +164,11 @@ public function getValidTests() | |||
| '{"items":{"default":"b"}, "minItems": 3}', | |||
| '["a","b","b"]' | |||
| ), | |||
| array(// #24 items is an array of schema | |||
|
@erayd we still have the problem and no update on this issue, can you look into it? |
|
@alex-pex I've looked into some of it, but not all yet - I'm woefully short of free time to work on this library. It'll happen at some point, but I can't promise when - my paid work takes priority. @shmax I don't suppose you have a spare moment to investigate this? You reviewed all the original stuff for the defaults feature, so hopefully will know what to look at... |
|
Sure, I'll look into it tomorrow... |
|
@erayd I spent the day working on this, but wasn't very productive (I'm currently living out of a suitcase, and the laptop I brought with me isn't set up for development, so I burnt up all my time setting up my environment). I haven't gotten as far as understanding how the recursive default stuff works, but I read through your comments on #417 and if nothing else was able to confirm that this proposed fix doesn't seem to help with that issue. I'm really crushed for time. If you're worried about #417, maybe we can see if @wlalele would be willing to add a test for it to this PR, and see if he can account for it? At this point he certainly knows more about |
|
Same issue: #632 |
|
The changes seems good for this PR. However I feel the same sentiment as @erayd about #417 / #632 On a side note the actions for this PR seems to be lost due to time passing by. Perhaps a |
DannyvdSluijs
added
Open discussions
Hey guys @justinrainbow @bighappyface @shmax @martin-helmich,
I think this test should be working, just like the 20 or the 21, it is
quite similar but still produces a wrong result.
How could we resolve this case ? We need your help on this 😢