Goal

This change unifies the SessionTracker and Client WRT how they use Guzzle. They now both use the HttpClient abstraction, which also allows them to share a Guzzle instance. This means that we can't use the base URI feature of Guzzle anymore because the notify and session endpoints are on separate subdomains. However we always POST to the root so weren't really taking advantage of this anyway

Changeset

BC breaks

• Removal of getSessionClient method in Client and Configuration
  This doesn't make sense to keep given the session client is now the same as the notify Guzzle client. Keeping this would either mean changes to the "session" client also propagate to the notify client or changes to this do nothing. Either of which could lead to things being delivered to the wrong endpoint and expected Guzzle changes not being reflected in the actual requests

• We no longer use the Guzzle base URI for requests
  This means if a Guzzle client is passed in the constructor with a pre-defined base URI, we will still send requests to the notify URI. Client::setNotifyEndpoint now needs to be called manually instead. This changed because the Guzzle base URI is ambiguous now given that it is shared between notifications and sessions

• Removal of SessionTracker::setConfig
  This was unused internally and doesn't seem to be needed as the configuration can be changed via the Client or Configuration instance itself. Having the possibility for there to be two different Configuration instances could be dangerous as changes may not propagate as expected

• Client::ENDPOINT
  This is ambiguous as we have three separate endpoints
  Use Configuration::NOTIFY_ENDPOINT instead

• HttpClient::PAYLOAD_VERSION
  This is ambiguous as there is a session payload version too
  Use HttpClient::NOTIFY_PAYLOAD_VERSION instead

• Report::PAYLOAD_VERSION
  As above. This was also unused by the notifier
  Use HttpClient::NOTIFY_PAYLOAD_VERSION instead

• SessionTracker::$SESSION_PAYLOAD_VERSION
  Use HttpClient::SESSION_PAYLOAD_VERSION instead

• Client::makeGuzzle has been made private
  Use the $guzzle parameter of Bugsnag\Client::__construct instead

Tests

The existing tests have been modified to avoid using the removed methods and the SessionTracker mocks updated to use HttpClient rather than Guzzle. Otherwise this is covered by existing tests

Discussion

Outstanding Questions

In the original PR for this (#521) we pulled the base URI out of the Guzzle instance that can be passed to the Client constructor and set the notify endpoint based on that

I'm not convinced whether that's a good idea — we can't use the base URI feature of Guzzle as we're sharing a single Guzzle client for two different URIs so they have to be absolute. That means if a base URI is set then it will be ignored anyway, because the absolute URI makes it irrelevant (the idea is you set base_uri => 'https://example.com and then can request https://example.com/foo by passing foo to Guzzle)

Linked issues

Supersedes #521
Fixes #558

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language