Enhancement: oauth2 support for stack-oauth #9
Conversation
till commented Jul 3, 2014
till commented - quick and dirty 'hack'
- thoughts?
* quick and dirty 'hack' * thoughts?
There was a problem hiding this comment.
Why did you opt for doing everything with HttpFoundation\* rather than importing classes directly? Especialy with HttpFoundation\Request this looks kinda clumsy.
There was a problem hiding this comment.
@simensen Personal preference. I like to group imports vs. having too many of them. But I can I revert it if need be. :)
There was a problem hiding this comment.
Don't worry about it just yet. We can circle back to it if needed. :)
If it were for anything other than HttpFoundation I might not care. :) For consistency, I think it would be better for Request to be used everywhere rather than HttpFoundation\Request to be used in just a handful of places. :)
| It might be a bit over-engineered, and I know you said it was a quick hack, but I feel like I'd rather see some sort of strategy/handler system in here to figure out which thing should be done based on OAuth1 or OAuth2 rather than if/else blocks. In general, though, I think it is a nice start! |
| @simensen I was wondering if I should do something based on the |
There was a problem hiding this comment.
Is this missing a return value? $token = ... or somesuch? I'm not super familiar with the oauth package so I'm not sure if there is something else that needs to go on here.
There was a problem hiding this comment.
It doesn't, requestAccessToken calls the storage internally.
The original code ($userId) is never used/returned. It could be removed.
* also introduced 'service_scopes' and 'service_base_url' in configuration
| @till Here are two ways I was thinking about it. Hard to say if either of these would actually be better or not. :) |
| @simensen I think I like the second one better. |
2 participants
