-
Notifications
You must be signed in to change notification settings - Fork 30
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
#51 Support access token auth #55
Conversation
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.
Hello @LeJeanbono, first of all, thanks for your contribution!
I like how you've handled the token and the small refactoring.
Nevertheless I have one small suggestion below, what do you think?
protected function postFile($file, $username = null, $password = null) | ||
{ | ||
$url = $this->getUrl() . '.' . pathinfo($file, PATHINFO_EXTENSION) . '?properties=composer.version=' . $this->getConfiguration()->getVersion(); | ||
|
||
$options = [ | ||
'debug' => $this->getIO()->isVeryVerbose(), | ||
'body' => fopen($file, 'r') | ||
]; | ||
|
||
$options = []; | ||
if (!empty($username) && !empty($password)) { | ||
$options['auth'] = [$username, $password]; | ||
} | ||
|
||
$this->apiCall($file, $options); | ||
} | ||
|
||
/** | ||
* Post with access token auth | ||
*/ | ||
protected function postFileWithToken($file, $token) | ||
{ | ||
$options = []; | ||
$options['headers']['Authorization'] = 'Bearer ' . $token; | ||
$this->apiCall($file, $options); | ||
} |
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'm under the impression that both postFile
and postFileWithToken
's body should be moved to the AbstractProvider.php
since they share exactly the same code, now that you've pushed the options into apiCall
.
What do you think?
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 agree !
Here we are @Elendev ! |
@LeJeanbono sorry I forgot to activate the workflow for this PR earlier, there is a small code sniffing issue. Thanks! |
Elendev#51 Support access token auth
Can you tag for #hacktoberfest please @Elendev ?