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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Problem with items being an array of schema #521

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wlalele
Copy link

@wlalele wlalele commented Jul 2, 2018

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 馃槩

wlalele and others added 2 commits July 2, 2018 18:27
I think this test should be working, just like the 20 or the 21, it is
quite similar but still produces a wrong result.
@wlalele
Copy link
Author

wlalele commented Jul 2, 2018

My colleague @alex-pex found the bug and it fits perfectly with the other test cases
We are waiting for the merge 馃槂

@wlalele wlalele changed the title Add a failing test about items being an array Bugfix: Problem with items being an array of schema Jul 2, 2018
Copy link
Collaborator

@erayd erayd left a comment

Choose a reason for hiding this comment

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

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.

@wlalele
Copy link
Author

wlalele commented Jul 3, 2018

@erayd Hi, I changed the number of the test case

Are you talking about this one ? #417

@alex-pex
Copy link

@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

Choose a reason for hiding this comment

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

Solves #521 (review)

@alex-pex
Copy link

@erayd we still have the problem and no update on this issue, can you look into it?

@erayd
Copy link
Collaborator

erayd commented May 31, 2019

@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...

@shmax
Copy link
Collaborator

shmax commented May 31, 2019

Sure, I'll look into it tomorrow...

@shmax
Copy link
Collaborator

shmax commented Jun 2, 2019

@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 $path->seFromDefault() than I do and I'm unlikely to be able to catch up before next weekend.

@COil
Copy link

COil commented Nov 20, 2020

Same issue: #632

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.

None yet

6 participants