Skip to content
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

OAuth2.0 documentation feedback #185

Closed
boryn opened this issue Mar 18, 2023 · 5 comments
Closed

OAuth2.0 documentation feedback #185

boryn opened this issue Mar 18, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@boryn
Copy link
Contributor

boryn commented Mar 18, 2023

First, I just must say this is an exceptionally great package, perfectly thought out and with great scalability. WOW!

I have some feedback on the documentation, in the places where I got blocked during the implementation. When looking at https://docs.saloon.dev/digging-deeper/oauth2-authentication#refreshing-access-tokens, there is:

$authenticator = $user->auth; // Your authenticator class.

which makes me think, what the heck is $user->auth??? Is this a field? Is this a relationship? What have I missed?

I bet it would be more clear if the field was named authenticator and comment was something like this:
// $authenticator stored in the model using OAuthAuthenticatorCast or EncryptedOAuthAuthenticatorCast

Later, the docs at https://docs.saloon.dev/digging-deeper/oauth2-authentication#building-a-method-to-create-an-authenticated-instance-of-your-sdk show the spotify() method example. But within this, there should be no $user-> but $this-> as we are already in the User class.

PS. I would as well suggest splitting this method into two for better readability:

public function spotify(): SpotifyApiConnector
{
    $authenticator = $this->refreshAccessToken($this->spotify_authenticator);

    $spotify = new SpotifyApiConnector();
    $spotify->authenticate($authenticator);

    return $spotify;
}

public function refreshAccessToken(
    OAuthAuthenticator $authenticator,
    $forceRefresh = false
): OAuthAuthenticator {
    if ($authenticator->hasExpired() || $forceRefresh) {
        $authenticator = SpotifyAuthConnector::make()->refreshAccessToken($authenticator);

        $this->spotify_authenticator = $authenticator;
        $this->save();
    }

    return $authenticator;
}
@Sammyjo20
Copy link
Collaborator

Hey @boryn

Many thanks for your suggestions around the documentation - I totally understand that it looks a little confusing. I'm going to amend the docs so they read better and don't feel so cluttered.

I'm also going to be writing the docs for the client credentials grant so I will give this whole section a reread and work out where else we can improve 🔥

@boryn
Copy link
Contributor Author

boryn commented Mar 30, 2023

Hi @Sammyjo20!

Found another place, where it would be good to add some details. After looking at source code, I have found out that we can pass key name while fetching response as an array or collection:
$response->json('data')
$response->collect('data')

The documentation at https://docs.saloon.dev/the-basics/responses does not mention this.

BTW. ->json() is a little bit misleading, as actually we receive an array. Maybe it would be good to have an alias ->array() to this method? Or even let json return a json string and array an array, but then it would be a breaking change.

@boryn
Copy link
Contributor Author

boryn commented Apr 1, 2023

The examples at https://docs.saloon.dev/digging-deeper/pagination contain $paginator->setCurrentOffset(1000); but this method seems to be not implemented.

Examples should not contain semicolon at the end of methods,
e.g.: public function paginate(Request $request, mixed ...$additionalArguments): OffsetPaginator;

@Sammyjo20 Sammyjo20 added the enhancement New feature or request label Apr 2, 2023
@Sammyjo20
Copy link
Collaborator

Hey @boryn just wanted to let you know that I've re-written the section on the authorization code grant: https://docs.saloon.dev/digging-deeper/oauth2-authentication/oauth2-authentication

Let me know your feedback and if it reads better :D I'm going in to write the documentation on the client credentials grant.

@Sammyjo20
Copy link
Collaborator

I'm going to close this for now, please can we discuss future documentation improvements as discussions as they are better suited (with replies and threads)

Cheers @boryn as ever!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants