SpigotMC / XenforoResourceManagerAPI

Exposes resource/author information via a simple JSON REST API
BSD 3-Clause "New" or "Revised" License
79 stars 7 forks source link

close a bunch of issues #38

Closed jacobsandersen closed 3 years ago

jacobsandersen commented 4 years ago

Intends to:

jacobsandersen commented 3 years ago

Going to do some work on this soon. Have had too much school stuff going on recently.

jacobsandersen commented 3 years ago

@md-5 This is ready for review. For your reference:

ADDITIONS

CHANGES

And that should be it!

EXTRA The DB fields needed have slightly changed for resource/author and there are some extra queries to look at. If you have any questions or need clarification please just reach out.

Thanks, simple

md-5 commented 3 years ago

Nice work

md-5 commented 3 years ago

Looks good as far as my php skills go. Trying to find an extra pair of eyes or two would probably be helpful, but otherwise I'm ok with merging

One thing probably worth double checking though, does this need php 8? We currently run 7.3 as that's the supported debian version https://packages.debian.org/buster/php

jacobsandersen commented 3 years ago

Looks good to me. Great work!

Thank you for reviewing Marc, I appreciate it!

Looks good as far as my php skills go. Trying to find an extra pair of eyes or two would probably be helpful, but otherwise I'm ok with merging

One thing probably worth double checking though, does this need php 8? We currently run 7.3 as that's the supported debian version https://packages.debian.org/buster/php

Does not need php 8, nope. I just updated the docker stuff to use 7.3 to match Spigot's env. I tested every route and all works as expected!

I asked Marc to review and I'm looking for one more person, or you can merge if you feel okay with it already.

Thanks both!

md-5 commented 3 years ago

Excellent stuff. Thanks very much Marc, likewise I really appreciate it.

All good with this from me and can merge if you wish. Unfortunately can't deploy right now as otherwise preoccupied, but I will make sure it is deployed ASAP.

Thanks again for your hard work Simple, also really appreciate it.

jacobsandersen commented 3 years ago

Ready to merge!

Thanks a lot md! Good luck with the 1.17 update 😎

jacobsandersen commented 3 years ago
jacobsandersen commented 3 years ago

Okay, now it's really ready to go. Just had some itches to scratch there. Not touching anymore 👍🏻

md-5 commented 3 years ago

Merged and will work on deploying