search

zf-fr / zfr-oauth2-server

PHP library for creating an OAuth 2 server (currently proof of concept)
BSD 3-Clause "New" or "Revised" License
36 stars 13 forks source link

Refactoring middleware #44

Closed basz closed 8 years ago

basz commented 8 years ago

what do you think

bakura10 commented 8 years ago

This seems to include also the previous changes from the previous PR. Can the other be closed?

basz commented 8 years ago

i'm done, please review...

basz commented 8 years ago

ping @bakura10 (@danizord)

basz commented 8 years ago

ping

bakura10 commented 8 years ago

Sorry for the delay :(. I was pretty busy and @danizord will be too but he told me today he will try to review this asap. Sorry sorry sorry :(.

danizord commented 8 years ago

Hey @basz, I did some reading on your code yesterday. Since I don't know very well how it worked previously and how it works now, it's a bit hard for me to understand the flow and provide feedback. So I'll ask @bakura10 to explain me how it works in details before I comment anything else.

But in onverall it looks very good, seems like you brought some blood to this anemic models :D

bakura10 commented 8 years ago

This PR is actually way too big to properly review now.

What I'd say is that you address the few comments first, then run a bit of optimize imports using PHPStorm tool as I suspect there are a lot of unused imports now, then I'll merge along the Doctrine one, so you can start testing the various use cases in your app (and if you want to add support for JWT token, go on!!).

Once you fix the various bugs you can spot in your app, then we'll try to do another final review @danizord and I on the code locally, as it's a bit unmanageable right now.

What do you think? :)

basz commented 8 years ago

Agreed is has become a monster PR. The thing has been completely rewritten...

Thanks for the feedback, you too @danizord. I'll address the point in it asap and hopefully we can go to smaller more manageable ones.

Just in time for integration in my app :) yes I am going to use this. I'm very happy with the test I have done with it. There are still a few thing we need to address. My hope is you are able to steer me in the correct direction for it. I'll do the work.

Bas

bakura10 commented 8 years ago

I'm already very happy to the direction it taking. I like that we were able to remove the depeendency to Doctrine and it will make @weierophinney happy :D.

basz commented 8 years ago

and it will make @weierophinney happy :D

Nice side effect - that won't hurt no matter

basz commented 8 years ago

:-bliep-:

bakura10 commented 8 years ago

:D.

So I think I can merge the Doctrine stuff now? As said, with this merge I think you can spend a few time trying your app and do smaller PR for bug fixes.

Once stabilized, we'll be able to do more architectural changes. Seems like the simplest path :)