-
Notifications
You must be signed in to change notification settings - Fork 98
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: make Forms OpenAPI compliant #2358
base: main
Are you sure you want to change the base?
Conversation
4ec727e
to
1527af4
Compare
1527af4
to
442818e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2358 +/- ##
=======================================
Coverage ? 42.88%
Complexity ? 851
=======================================
Files ? 72
Lines ? 3211
Branches ? 0
=======================================
Hits ? 1377
Misses ? 1834
Partials ? 0 |
5f0053f
to
190bc42
Compare
f9e4dc4
to
0e0dd0f
Compare
4b1572c
to
700fe5e
Compare
@Chartman123 as it seems you are pretty much free to change the API however you like, would you like to have a review by me of the whole API to point out improvements? |
@provokateurin yes, that would be nice :) Would you recommend to first do this review and then make it OpenAPI compliant? |
I think it is easier to first make it all work and then review and refactor it. |
Ok, I think I should be ready by the weekend |
6bbddcb
to
aadf1d3
Compare
9c1f80a
to
841aed3
Compare
@provokateurin I've fixed most of the psalm errors now... but I just can't seem to find a way to fix the remaining ones.
how can I do this? do I have to apply the patch manually to the files in the vendor-bin directory? |
I can try to take a look, but I'm already busy with similar problems this week so I might lack the headspace for it this week.
You can clone the repo, do a |
I fixed everything. The main problem was that you didn't type the lower-level methods and then psalm wasn't inferring the types correctly anymore. Now you should take a look if everything is to your liking and change anything you want and then I can give this an API design review 🎉 |
Uh ok I made the tests unhappy, can you fix that for me? |
To be more precise: I had to change some of your type declarations to match the real code. Please check that everything you want to have exposed through the API is as intended (both missing information and too much information e.g. for security). |
Thank you very much 😇 I'll have a look at it tomorrow or on the weekend. |
ee70205
to
96954fd
Compare
@provokateurin I was able to fix the unit tests (switched back to associative array in Question.php). Now there's only one error left in the integration tests, that might be related to this \stdClass / array thing for the extraSettings...
|
I'll take a look. I think my mistake was to do the conversion too early, it should only take place in the Controllers. |
Signed-off-by: Christian Hartmann <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
96954fd
to
12df9c5
Compare
Hi, I just wanted to say that I haven't forgotten this PR. |
Everything fine, I also don't have much time at the moment :) |
This fixes #2356