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

Issue 1965 - Exceptions option fallback to http_errors on guzzle 6 or 7 #1966

Merged
merged 3 commits into from
Oct 20, 2020
Merged

Issue 1965 - Exceptions option fallback to http_errors on guzzle 6 or 7 #1966

merged 3 commits into from
Oct 20, 2020

Conversation

sylvain-msl
Copy link
Contributor

Misc

Fix for #1964

Changes

Updates

  • default options exceptions changed to http_errors when guzzle version is 6 or 7. (The deprecation of exceptions was implemented in guzzle 6 and enforced in guzzle 7).

@sylvain-msl sylvain-msl requested a review from a team as a code owner October 19, 2020 07:35
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 19, 2020
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I cleaned up the submission a bit to make it more readable w/o changing the functionality. If you can make these changes then we can merge this.

@@ -1162,6 +1162,9 @@ protected function createDefaultHttpClient()
} elseif (6 === $guzzleVersion || 7 === $guzzleVersion) {
// guzzle 6 or 7
$options['base_uri'] = $this->config['base_path'];

$options['http_errors'] = $options['exceptions'];
unset($options['exceptions']);
Copy link
Contributor

@bshaffer bshaffer Oct 19, 2020

Choose a reason for hiding this comment

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

Seems like we should just remove the setting of exceptions above and do the following instead for more obvious/straightforward logic:

if (5 === $guzzleVersion) {
  $options = [
    'base_url' => $this->config['base_path'],
    'defaults' => ['exceptions' => false],
  ];
  if ($this->isAppEngine()) {
    // set StreamHandler on AppEngine by default
    $options['handler'] = new StreamHandler();
    $options['defaults']['verify'] = '/etc/ca-certificates.crt';
  }
} elseif (6 === $guzzleVersion || 7 === $guzzleVersion) {
  // guzzle 6 or 7
  $options[
    'base_uri' => $this->config['base_path'],
    'http_errors' => false,
  ];
} else {
  throw new LogicException('Could not find supported version of Guzzle.');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It makes more sense indeed. Thanks for your feedback.

@bshaffer bshaffer merged commit e6d64be into googleapis:master Oct 20, 2020
@bshaffer
Copy link
Contributor

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants