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.

Sign up for GitHub

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 GithubOAuth.php #123

Closed
wants to merge 1 commit into from
Closed

Update GithubOAuth.php #123

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion src/Libraries/GithubOAuth.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,51 @@ protected function fetchUserInfoWithToken(): object
],
'http_errors' => false,
]);

$userInfo = json_decode($response->getBody());
Comment on lines +88 to +89
Copy link

Choose a reason for hiding this comment

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

Check for API request success before processing the response.

The response from the GitHub API should be checked for success before attempting to decode the body. This ensures that the code handles errors gracefully.

+ if ($response->getStatusCode() !== 200) {
+     throw new Exception('Failed to fetch user info from GitHub API');
+ }
  $userInfo = json_decode($response->getBody());
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$userInfo = json_decode($response->getBody());
if ($response->getStatusCode() !== 200) {
throw new Exception('Failed to fetch user info from GitHub API');
}
$userInfo = json_decode($response->getBody());


/**
* Note: When making a call to /user, the API will only return the email address the user's explicitly set publicly visible
* (or null if the user has not specified a public email address in their profile).
*/
if (empty($userInfo->email)) {

/**
* The email address is mandatory we have no other choice but to dig deeper.
*
* To retrieve the user's email addresses regardless if their status is set public or not, we need to call /user/emails.
* @see https://docs.github.com/en/rest/users/emails?apiVersion=2022-11-28#list-email-addresses-for-a-user
*/
$response = $this->client->request('GET', self::$API_USER_INFO_URL.'/emails', [
'headers' => [
'User-Agent' => self::$APPLICATION_NAME . '/1.0',
'Accept' => 'application/vnd.github+json',
'Authorization' => 'Bearer ' . $this->getToken(),
],
'http_errors' => false,
]);
Comment on lines +103 to +110
Copy link

Choose a reason for hiding this comment

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

Check for API request success before processing the response.

The response from the GitHub API should be checked for success before attempting to decode the body. This ensures that the code handles errors gracefully.

+ if ($response->getStatusCode() !== 200) {
+     throw new Exception('Failed to fetch user emails from GitHub API');
+ }
  $emailAddresses = json_decode($response->getBody());
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$response = $this->client->request('GET', self::$API_USER_INFO_URL.'/emails', [
'headers' => [
'User-Agent' => self::$APPLICATION_NAME . '/1.0',
'Accept' => 'application/vnd.github+json',
'Authorization' => 'Bearer ' . $this->getToken(),
],
'http_errors' => false,
]);
$response = $this->client->request('GET', self::$API_USER_INFO_URL.'/emails', [
'headers' => [
'User-Agent' => self::$APPLICATION_NAME . '/1.0',
'Accept' => 'application/vnd.github+json',
'Authorization' => 'Bearer ' . $this->getToken(),
],
'http_errors' => false,
]);
if ($response->getStatusCode() !== 200) {
throw new Exception('Failed to fetch user emails from GitHub API');
}
$emailAddresses = json_decode($response->getBody());


$emailAddresses = json_decode($response->getBody());

/**
* The response can contain multiple email addresses
* so, we'll choose the one marked as primary
* or at least get the first one just to be covered
*/
$userInfo->email = $emailAddresses[0]->email;
Copy link

Choose a reason for hiding this comment

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

Check for empty email addresses array.

Before accessing the first element of the $emailAddresses array, ensure it is not empty to avoid potential errors.

+ if (empty($emailAddresses)) {
+     throw new Exception('No email addresses found for the user');
+ }
  $userInfo->email = $emailAddresses[0]->email;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$userInfo->email = $emailAddresses[0]->email;
if (empty($emailAddresses)) {
throw new Exception('No email addresses found for the user');
}
$userInfo->email = $emailAddresses[0]->email;


foreach ( $emailAddresses as $emailAddress ) {
if ($emailAddress->primary) {
$userInfo->email = $emailAddress->email;
break;
}
}
Comment on lines +121 to +126
Copy link

Choose a reason for hiding this comment

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

Optimize email address selection logic.

The logic for selecting the primary email address can be optimized by using array_filter and array_shift.

- foreach ( $emailAddresses as $emailAddress ) {
-     if ($emailAddress->primary) {
-         $userInfo->email = $emailAddress->email;
-         break;
-     }
- }
+ $primaryEmail = array_filter($emailAddresses, fn($email) => $email->primary);
+ $userInfo->email = !empty($primaryEmail) ? array_shift($primaryEmail)->email : $emailAddresses[0]->email;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
foreach ( $emailAddresses as $emailAddress ) {
if ($emailAddress->primary) {
$userInfo->email = $emailAddress->email;
break;
}
}
$primaryEmail = array_filter($emailAddresses, fn($email) => $email->primary);
$userInfo->email = !empty($primaryEmail) ? array_shift($primaryEmail)->email : $emailAddresses[0]->email;

}
Copy link

Choose a reason for hiding this comment

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

Handle potential exceptions more gracefully.

Using exit for exception handling is not recommended. Instead, consider logging the error and throwing an exception.

- exit($e->getMessage());
+ log_message('error', $e->getMessage());
+ throw new Exception('Error fetching user info with token');

Committable suggestion was skipped due to low confidence.

} catch (Exception $e) {
exit($e->getMessage());
}

return json_decode($response->getBody());
return $userInfo;
}

protected function setColumnsName(string $nameOfProcess, $userInfo): array
Expand Down
Loading