-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add provider for OpenStreetMap, using OAuth 1.0a #869
Conversation
Thank you! Can I ask you to also provide basic documentation? |
Hum, I don't see where I should add basic documentation on this provider.
Can you provide an example of a provider that has this kind of documentation ?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues which should be fixed before merging 👍
Thank you for your effort. 🌞
function getUserProfile() | ||
{ | ||
try{ | ||
$apiUrl = 'https://api.openstreetmap.org/api/0.6/user/details'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be just 'user/details'
because $this->api->api_base_url
will be added automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true - I've add several issue when making this provider, so I tried several things. The real fix was in fact to switch to a XML decoder instead of using json (which seems to be the default decoding)
/** | ||
* Hybrid_Providers_OpenStreetMap (openstreetmap.org) | ||
*/ | ||
class Hybrid_Providers_OpenStreetMap extends Hybrid_Provider_Model_OAuth1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no coding standards at all. Can you fix it according to PSR2 standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've fixed indentation, and functions calls
return $this->user->profile; | ||
} | ||
|
||
function httpRequest( $url ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not used at all. Please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this unused function httpRequest().
|
||
$response = @ new SimpleXMLElement( $response ); | ||
|
||
$this->user->profile->identifier = (string) $response->user["id"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's impossible to have object and array in one instance like here $response->user
, so pls use only array style or only object-properties style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what you mean ?
Should I replace $response->user["id"]
by $response->user->id
?
Do you have any example of unit tests for Providers ? |
This is v2 so we can skip that part :( |
@ApacheEx Looking good now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, now it looks more clean and can be merged. Thanks!
Summary
Goal
Addition of OpenStreetMap authentication against v2 branch
Description
I've added OpenStreetMap authentication on wordpress plugin, and thought it would be better to add it on hybridauth git repository instead of wordpress-social-login. I've only tested the plugin through Wordpress.