Disable http plug discovery when using custom transport

[ghost]

I'm using this SDK (the 2.0 beta version) inside an asynchronous ReactPHP application, but due to the broken http-plug reactphp client I've made my own sentry transport which uses the already available HTTP client inside my application. But due to the way how the client is built inside the client builder, even when I set a custom transport, the client builder tries to find a http plug HTTP client using the http plug discovery mechanism.

As such I have to set a custom strategy to mitigate this.

$class = \get_class((new class() {
    static function getCandidates() {
        return array(array('class' => function () {}));
    }
}));

\Http\Discovery\ClassDiscovery::setStrategies(array($class));

Would you be interested in a simple patch to not do the discovery if a transport is set? From what I've seen the HTTP client is only used when building a transport. So the patch would be to simply check whether a transport is set or not.

sentry-php/src/ClientBuilder.php

Lines 260 to 268 in 5ec1fbe

	public function getClient(): ClientInterface
	{
	$this->messageFactory = $this->messageFactory ?? MessageFactoryDiscovery::find();
	$this->uriFactory = $this->uriFactory ?? UriFactoryDiscovery::find();
	$this->httpClient = $this->httpClient ?? HttpAsyncClientDiscovery::find();
	$this->transport = $this->transport ?? $this->createTransportInstance();
	return new Client($this->options, $this->transport, $this->createEventFactory());
	}

The simple patch would be to change

sentry-php/src/ClientBuilder.php

Lines 264 to 265 in 5ec1fbe

	$this->httpClient = $this->httpClient ?? HttpAsyncClientDiscovery::find();
	$this->transport = $this->transport ?? $this->createTransportInstance();

to

if($this->transport === null) {
    $this->httpClient = $this->httpClient ?? HttpAsyncClientDiscovery::find();
    $this->transport = $this->createTransportInstance();
}

Of course you're welcome to do the simple change yourself. :)

[ste93cry]

I could be missing something, but looking at the source code if you want to use a custom HTTP client you can just set it using the ClientBuilder::setHttpClient() method. Whichever instance set will be used by the configured transport so it should work

$httpClient = new CustomHttpClient();

$clientBuilder = ClientBuilder::create(['dsn' => '__PUBLIC_DSN__']);
$clientBuilder->setHttpClient($httpClient);

// $client will use the default transport HttpTransport with the configured CustomHttpClient client
$client = $clientBuilder->getClient();

[ghost]

I'm not using http plug at all. It's a completely custom HTTP client with no integration of http plug.

[ste93cry]

I still don't get how it's using the HttpClient from HTTPlug. If you set the transport yourself no call will be made to either the ClientBuilder::createHttpClientInstance() or ClientBuilder::createTransportInstance methods, so even though the PSR-7 factories and the HTTP client are discovered by the *Discovery classes they won't be used at all

[ghost]

The issue is that there is no HttpClient from HTTPlug and thus an exception is thrown by HttpAsyncClientDiscovery::find().

By making the proposed changes, we don't make any unnecessary discoveries when they are not used at all.

[ste93cry]

Got it now, I misunderstood your first message sorry. I see the problem now and agree with you we should not autodiscover anything if a custom transport is set, but probably I would think about if it really makes sense to expose the ClientBuilder::setHttpClient() method instead of just the ones related to the PSR-7 factories and the transport itself. This way cases like the one you described will not happen and if someone wants to customize the HTTP client then they have to set the transport themselves too. Does it makes sense?

[ghost]

That does make sense. I'd also say that these three lines could be moved into createTransportInstance()

sentry-php/src/ClientBuilder.php

Lines 262 to 264 in 5ec1fbe

	$this->messageFactory = $this->messageFactory ?? MessageFactoryDiscovery::find();
	$this->uriFactory = $this->uriFactory ?? UriFactoryDiscovery::find();
	$this->httpClient = $this->httpClient ?? HttpAsyncClientDiscovery::find();

Since they're related to the used transport and a custom transport won't use them in any way.