Zn4rK / php-withings

PSR-FIG Compatible Withings Body metrics Services API written in PHP.
17 stars 14 forks source link

Remove socialite requirement and suggest socialiteproviders/withings #26

Closed u1735067 closed 3 years ago

u1735067 commented 6 years ago

After my test yesterday (#25), I wrote a socialiteproviders' provider based on your code : https://github.com/Alex131089/socialiteproviders-withings. So after following the SocialiteProviders installation instructions, you can use somethings like:

use Socialite;
use Paxx\Withings\Api as WithingsApi;

Route::get('withings', function() {
    return Socialite::driver('withings')->redirect();
});
Route::get('withings/callback', function() {
    $user = Socialite::driver('fitbit')->user();
    save_user_credentials($user);
});
Route::get('withings/test', function() {
    $config = [
        'identifier' => env('WITHINGS_KEY'),
        'secret' => env('WITHINGS_SECRET'),
    ] + get_user_credentials();
    $api = new WithingsApi($config);
    $measures = $api->getMeasures();
    dd($measures);
});

As Socialite doesn't support extension officialy (if I got it right), maybe this should be suggested instead of socialite ? I haven't made a pull request on the https://github.com/SocialiteProviders/Providers repo yet: while it is mentioned the code is mostly (Servers's User field a bit modified) from Paxx\Withings (and is a suggestion), should your name be added somewhere ?

Zn4rK commented 6 years ago

I'm fine with merging this pull request, if you require my package in your package, instead of copying the Server and Provider (ie - change suggest to require and use my namespaces in WithingsExtendSocialite) - This way we don't need to keep two sets of almost the same code in two places. This package provides both a server and a provider that seems to be compatible.

You can make the changes needed to the server's field and update this PR instead.

The reasoning behind it is that your package is incomplete without this package, but this package does not necessarily need your package (although it provides a better user experience if used).

This way people can extend Socialite the way you showed - or use SocialiteProviders.

Zn4rK commented 6 years ago

Regarding attribution - If you just pull in my package I'm fine without it :)

u1735067 commented 6 years ago

if you require my package in your package, instead of copying the Server and Provider (ie - change suggest to require and use my namespaces in WithingsExtendSocialite)

I wanted to do this at first, but classes have to inherit from https://github.com/SocialiteProviders/Manager/blob/master/src/OAuth1/AbstractProvider.php and https://github.com/SocialiteProviders/Manager/blob/master/src/OAuth1/Server.php because they add some properties. It would have been great if they used traits. Eventually you could inherit your Provider and Server from thoses and make the same change as me in the Provider and Server (https://github.com/Alex131089/socialiteproviders-withings/commit/4f5bb9fdb4490d3ab30cd8732a6601e63a5ba03e is the only change for the server I think), but that would forbid to "manually" extend Socialite like in #25 without the whole SocialiteProviders.

The reasoning behind it is that your package is incomplete without this package

Hum, no, it is independent, but you can't do much without Paxx/Withings or another package though. It only allows to retrieve user informations. For the rest you'd have to write you own methods.

Zn4rK commented 6 years ago

Hm, I just checked actually. The last time I used this package for myself, I went with SocialiteProviders, and it works right out of the box - without having to extend from the Manager' classes. Has this changed?

<?php namespace App\Providers\Socialite\Withings;

use SocialiteProviders\Manager\SocialiteWasCalled;

class WithingsExtendSocialite
{
    public function handle(SocialiteWasCalled $socialiteWasCalled) {
        $socialiteWasCalled->extendSocialite(
            'withings',
            'Paxx\Withings\Provider\Withings',
            'Paxx\Withings\Server\Withings'
        );
    }
}

And then you just follow the instructions for the SocialiteProviders regarding EventServiceProvider.

u1735067 commented 6 years ago

When I tried, I got stuck at $providerClass::additionalConfigKeys(); in https://github.com/SocialiteProviders/Manager/blob/master/src/SocialiteWasCalled.php#L149 because not using SocialitePrividers' abstract.

Zn4rK commented 6 years ago

Hmm, I see. SocialiteProviders does seem to have a Trait for the config though. If that works, you would probably only have to replace the server and extend my server class, and add the trait...

u1735067 commented 6 years ago

Using the copied Provider and testing this Server :

namespace SocialiteProviders\Withings;

use Paxx\Withings\Server\Withings as WithingsServer;
use SocialiteProviders\Manager\ConfigTrait;

class ServerDev extends WithingsServer
{
    use ConfigTrait; 
}

=>

(1/1) FatalThrowableErrorCannot use object of type League\OAuth1\Client\Credentials\TokenCredentials as array

in AbstractProvider.php (line 43)

=> They overload Server.php/getTokenCredentials() too

u1735067 commented 6 years ago

The Withings provider was added to https://github.com/SocialiteProviders/Providers in https://github.com/SocialiteProviders/Providers/pull/101. They removed any reference to this project in https://github.com/SocialiteProviders/Providers/commit/acdf9f902ee971fdc45a5e1c65289cb84f6ed8fd and https://github.com/SocialiteProviders/Providers/commit/4de0d557210beb1f301dabbbea55d2c920a75f01 though ... I don't know if they would accept a provider with dependence on another package ; I'll let you test that :)

Zn4rK commented 6 years ago

Ah, it doesn't really matter. As long as people find a use for it... I would've liked to be on the authors list, but it's not a big deal to be honest.

I've actually realised that this package does indeed need the socialite-package. The example code confirms it.

Zn4rK commented 6 years ago

If you could update the PR to keep "laravel/socialite": "^2.0 || ^3.0" I'll merge it :)

u1735067 commented 6 years ago

As another suggestion you mean, as it is not strictly required ?

u1735067 commented 6 years ago

Oh, I missed your previous comment, the example code is only needed when you want to integrate with socialite (and use Provider.php), and in that case you already have socialite in your requirement, but the example code can live by itself, right ?

Zn4rK commented 6 years ago

Yep, the example code can live by itself. Nothing more than this package is needed for it...

u1735067 commented 6 years ago

So .. what do you plan to do with this ? I'm not sure we understand each other ? From what I see, php-withings is a lib on its own (as in example/basic.php), it doesn't needs socialite to work (Paxx/Withings/Provider/Withings.php being used only for socialite, right ?). If users plan to use it directly with socialite, then the require dependency is not needed as they obviously already use it, and should be removed.

For the rest, suggesting either socialiteproviders/withings or socialite itself (or both ?) is up to you. I just don't want to leave PR opened when it's not useful :)