Extract the code to create a transport from the client builder to a transport factory

[RageZBla]

This should ease the process of registering a custom transport.

Related to #787

[stayallive]

Looks clean. Thanks! I do agree with @Jean85 but after that LGTM!

[ste93cry]

I don’t like to expose such method from the builder as it seems we’re leaking some internal behavior of the builder. I didn’t investigate if it’s possible, but we should probably instead wrap the HTTP client internally

[RageZBla]

@ste93cry I can see your point but in that case the documentation to register custom transport should have all the boilerplate to basically reproduce the registerDefaultPlugin behavior.

[ste93cry]

The problem arises because if someone sets the transport then we don't wrap the HTTP client because the transport may not need it and so doing it would be useless

sentry-php/src/ClientBuilder.php

Lines 309 to 313 in 98a07e7

	private function createTransportInstance(): TransportInterface
	{
	if (null !== $this->transport) {
	return $this->transport;
	}

What I was suggesting was to always wrap the client regardless of the transport set. A clearer solution imo would be to have a setTransportFactory method in place of the setHttpClient, setTransport, setMessageFactory, etc methods that you can use to set your own transport so that the builder does not need to care about instantiating the HTTP client based on what transport you want to use. This would not solve the fact that you would have to wrap your HTTP client with all the needed plugins, but I don't really get why if you decide to use your own transport then the SDK should be responsible to partially "configure" it

[RageZBla]

@ste93cry thanks for the feedback. I'll try to update my pull request over the week end.

[RageZBla]

@ste93cry may I ask for your opinion?

This is not the complete thing, I still need to write the tests but before that I would like you to acknowledge if the solution looks good enough for you.

Regarding the testGetClientTogglesCompressionPluginInHttpClient, we should probably move it to the DefaultTransportFactory test when I write it.

[ste93cry]

I think that the direction may be good. Probably it makes sense to passo to the create method of the factory the options of the client. I would remove the setters from the factory, once it's initialized I don't see any reason to let users change its state.

[RageZBla]

@ste93cry let me know if it looks like something that could be merged or if I need to work on it a little bit more.

[ste93cry]

I did some changes to your PR, may I ask if I can push them to your branch? In that case please ensure that maintainers can edit your PR by checking the checkbox in the right sidebar

[RageZBla]

@ste93cry the checkbox is ticked so you should be able to push. Thanks!

[ste93cry]

I pushed the changes and there is still a lot of work that must be done, from documentation to unit tests, but this should be a good start to provide the flexibility you were looking for at the beginning of this PR to create a custom HTTP client without having to also think about the plugins needed to make it work. I didn't have the chance to try it with a real Sentry server yet, so it may be that everything doesn't work anymore. It's just a starting point, please let me know if you're ok with them and then we can interate over this if you agree and see what cames out. The only downside to all of these changes is that we are polluting the codebase with deprecated things and the complexity is increasing rapidly and this makes me wonder if this is a good way to solve the issue

[RageZBla]

@ste93cry Thanks, I see you went all the way to create a dedicated factory for the HttpClient.

Yes, it is covering my original use case 👍

I'll try it out with real server tomorrow at work. Given that we moved code around, I think it should work.

In my opinion, it looks good and add some modularity and good extension points.

Do you want me to fix the broken tests and PHPStan? On the other hand what kind of test you had in mind for the HttpClientFactory?

[ste93cry]

This PR needs to be rebased but since it's scheduled for 2.3 and there are still things left to be merged it may be that it will need to be done again. We should also find a way to still support adding custom plugins to the HTTP client to not break BC. Since this is new code we should finally avoid using the deprecated assertAttribute methods of PHPUnit and find a better way to test the things that use them at the time of writing. As said before, as a last step we should document all added methods, classes and interfaces

[ste93cry]

Still a few changes, mostly missing documentation and a rebase to start with. Also missing is the handling of the addHttpClientPlugin and removeHttpClientPlugin methods that should still work with the new approach based on factories to not break BC. It would be also good to rework all the tests to stop using assertAttribute which is deprecated by PHPUnit

[RageZBla]

@ste93cry can you review and give us feedback?

Regarding this:

It would be also good to rework all the tests to stop using assertAttribute which is deprecated by PHPUnit

I am fine with that but that would require to add getters only for testing, are you OK with that?

[ste93cry]

I am fine with that but that would require to add getters only for testing, are you OK with that?

I reworked some of the newly added tests to not use the reflection to test internal state of the objects and I also created an additional HTTP client factory to support wrapping a client with the plugins. I decided to go that way because in 3.0 I would like to stop supporting out-of-the-box this feature and leave to the user the choice of how to do it, so it made more sense to not tie the HttpClientFactory::create method with the list of plugins. I also fixed some code style issues and polished a few things. WDYT?

[ste93cry]

Thank you for your contribution, all changes seem fine so LGTM