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

Update README.md #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update README.md #15

wants to merge 4 commits into from

Conversation

AJRepo
Copy link

@AJRepo AJRepo commented Oct 17, 2021

Expand documentation on token generation. See Issue #14

Expand documentation on token generation.
README.md Outdated
Comment on lines 45 to 46
// A getAccessTokenFromYourDataStore() example.
$existingAccessToken->getAccessToken('password', ['username'=>'USERNAME', 'password' => 'PASSWORD']);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this improves clarity of the documentation - I think it actually makes it worse. This example appears to suggest that:

  1. $existingAccessToken appears out of thin air
  2. $existingAccessToken exposes a getAccessToken method similar to the provider itself

I think there is room to improve the documentation but I don't think this is it.

getAccessTokenFromYourDataStore() is meant to serve as a placeholder for some function your application implements that retrieves an AccessToken instance from your own application persisted storage - whatever it may be.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reply to 1. Yes - that should have been

$existigAccessToken = $provider->getAccessToken(...)
with your definition of $provider above in that same file.

My bad. I was copying from actual code that works but with different variable names into the README.md and trying to keep the variable name consistent with the old README.md. I updated the branch AJRepo:patch-1 to fix.

In reply to 2.

Your library requires league/oauth2-client which provides the function getAccessToken()
which calls Salesforce OAuth API for automatically getting an access token using a username/password. I'm just seeing the PHP coding side of this Salesforce API to download/upload Salesforce data, so the statement "some function your application implements ... your own application persisted storage" doesn't make sense to me. As far as I can tell there's no token function my application has to natively implement and no persisted storage created.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are still missing the point. The purpose of this section of the documentation is to provide an example of how to take an EXISTING token (from your database) and use the refresh token to update the access token. This section of the documentation is not related to using the provider to get a NEW access token.

If your application does not have a database or data persistence, then this section of the documentation may not apply to you.

Your proposed changes will be misleading to other developers who use this package.

Copy link
Author

@AJRepo AJRepo Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see the confusion.

Your documentation assumes a token already exists but does not specify how to generate that token. Not having a part of the documentation that states HOW one gets a token is what's confusing.

When one looks up HOW to get a token the current Salesforce OAuth API documentation for REST ( https://developer.salesforce.com/docs/atlas.en-us.api_rest.meta/api_rest/quickstart_oauth.htm ) states

Using [the Salesforce] REST API requires an access token to successfully send requests. You can get an access token using the username-password authorization flow.

That's not a NEW access token. That's AN access token.

I can see that since I re-used the variable $existingAccessToken it would lead to that confusion. I've updated the README.md to perhaps clarify how your assumption that a token already existed was confusing to me and my reusing of the variable name might be confusing to others.

Expand documentation on token generation.
README.md Outdated
Comment on lines 49 to 52
// If you don't have a pre-saved token, use the Salesforce OAuth API to provide a token.
if empty($thisAccessToken) {
$thisAccessToken = $provider->getAccessToken('password', ['username'=>'USERNAME', 'password' => 'PASSWORD']);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section has nothing to do with "Refreshing a Token" - the heading under which you are proposing this documentation to exist.

The documentation you have proposed here is an example of obtaining a brand new token. Something that is better suited in the previous section, currently described as "Authorization Code Flow." If you think that would be useful, perhaps the previous section should be renamed to:

### Obtaining a new Access Token

#### Authorization Code Flow

[current documentation remains here]

#### Password Flow

[your example]

Keep in mind that the base package includes significantly more detailed documentation that is better to reference rather than copy/paste, creating a need to keep it in sync.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the headers to clear that up. Thanks for the feedback.

To be clear:
There isn't "Password Flow" . Salesforce states that the password is required to get an access token. That means the code using the username/password is part of obtaining a new Access Token. So there's no section called "Password Flow" but instead "Getting and/or Refreshing a Token"

The existing README.md documentation states a token exists in the programmer's local data storage (somehow, but not stated how or where that token is obtained). That is confusing given the documentation on salesforce's site states that you use the username/login/client_id to get a token. Some say the token is good for that session only (I've not tested).

Given that (1) the interface I was asked to look at had NO locally stored tokens and (2) the salesforce documentation specified no locally stored tokens and (3) the existing README.md file makes no mention of how to get a token; the existing documentation is made clearer by having a section that talks about how to get a token from salesforce.

Adding more comments for clarity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants