use Symfony HttpClient as client

[garak]

This should solve issue #246

[Jean85]

I'm sorry but I can't approve this; as the CI shows, this would break support for Symfony < 4.3, so it's a no-go at this level, it should be done by the end user. When we will drop 3.4 we could do this.

[garak]

CI is wrong, HttpClient is fine with Symfony 3.4 too.
I'll change travis configuration to avoid failing

[Jean85]

No, it won't work with any project that requires symfony/symfony with a lower version.

Also, you're forcing this choice on the end user, cutting him off from the possibility to replace the client, or at least without the possibility of dropping that dependency.

[garak]

I can't see any conflict between symfony/http-client and symfony/symfony.
About forcing the choice, you already did that when you required sdk. As mentioned in related issue, I'm for allowing the broader choice and not forcing user to any implementation, but it looks like "simplicity" is more important than choice

[Jean85]

You're right, symfony/symfony will not conflict as it is. But we're still missing the necessary package to inject the client in the HTTPlug discovery process, right?

[garak]

Latest version of php-http/discovery is able to discover it
(BTW, failing tests in travis look unrelated)

[Jean85]

Latest version of php-http/discovery is able to discover it

Then you should lock against it (^1.7 seems to be the right version)

[Jean85]

I fear that CI failures are not unrelated... It's halting on Symfony Request signature, which is NOT coming from 3.4, but 4.4:

https://travis-ci.org/getsentry/sentry-symfony/jobs/632285363#L776

• Downgrading symfony/http-foundation (v4.4.2 => v4.4.0): Loading from cache

[garak]

Can you try to run tests on a local environment? On mine, all tests are green 🤔

[garak]

I see "changes requested" but without any note. Is there something left to do?

[Jean85]

I still don't like this change very much. You're forcing an opinionated client on the users, without a way to opt out. With sentry/sdk instead you can always replace that, and avoid installing the suggested package.

[garak]

But you can replace this client too, I can't see any difference. The only difference is that this client is a Symfony component, so it's likely already required in a Symfony project.

[Jean85]

No, you can't replace it as a dependency, in this way it will be always installed, you can only leave it unused; that is a problem if it creates a conflict with a third package, for example.

And, since Flex, you can no longer give that for granted as installed.

[garak]

I don't get you, what is the reason that keeps you from replacing?

[Jean85]

With sentry/sdk you have a meta package that you can replace in your composer.json to avoid installing the suggested transport altogether; with your approach, you do not have that meta package to replace anymore.

[garak]

And can't you just replace symfony/http-client in the same way?

[Jean85]

Technically you would be lying, because you would be telling to Composer that you have something that works like symfony/http-client, but you actually don't. You could break future installation of other packages that would rely on that client.

[garak]

Technically you would be lying, because you would be telling to Composer that you have something that works like symfony/http-client, but you actually don't. You could break future installation of other packages that would rely on that client.

But that exactly what you're doing by replacing anything.
If you're replacing, it's because you expressly don't want to install that package. If you actually need that package, you can just avoid replacing it.

But, again, since we're in a Symfony bundle, I don't see a reason to exclude a Symfony component, that you'll likely need for something else, since you're using Symfony after all.
Such likelihood is greater than the likelihood of Guzzle need.

So, in the end, if you want to force an implementation, at least force a Symfony one (given that not forcing anything would be always the best option)

[tristanbes]

I'm also in favor of taking advantage of a Symfony component if we must choose between Symfony HTTP client vs Guzzle :)

[Jean85]

But that exactly what you're doing by replacing anything.
If you're replacing, it's because you expressly don't want to install that package. If you actually need that package, you can just avoid replacing it.

That's only half of what you do with replace; if you use it, Composer do not install it, but believes that the code that it is able to autoload (src or vendor) is enough to satisfy the same requirement. So, if you try to install another package that requires the Symfony HTTP Client, it will happily install, only to fatal at runtime later. This is not advisable, since we would break stuff for our users in a dangerous and undetectable way.

Instead, sentry/sdk does not install any code by itself, it's just a metapackage, so if you require sentry/sentry directly alongside other packages that provide the HTTP transport as a replacement, you're not breaking anything for you nor other users.

[garak]

So I guess that Sentry organisation should offer other meta packages, one for each possible client.
Or, at least, one for Symfony client.
Do you think it's feasible?

[Jean85]

Sentry offers an opinionated metapackage, creating others is not so useful and complicates maintenance; you can do it yourself, with another metapackage that replaces the original one and suggests the Symfony client.

[garak]

Of course I could do that way, but then I'm afraid that it would be hard to get accepted as a replacement, being in a situation of "official vs unofficial"

[dvdoug]

@Jean85 on the subject of putting "replace": {"sentry/sdk": "*"} into composer.json for those of us who prefer not to use Guzzle (that bit isn't documented btw, only the ability to additionally install the HTTP client of our choice), are there any guarantees from Sentry that in the future additional required packages or source code won't be added into sentry/sdk?

Using composer's replace mechanism for this does feel kinda hacky to me, but my main concern is that usually bumped versions of a package's own dependencies aren't included in the definition of semver compatibility so * feels unsafe, but limiting the replacement to ^2.0 of SDK would mean applications are in effect being required to chase the internal deps of the Symfony bundle if they don't want Guzzle installed?

[edited initial message]

[Jean85]

@dvdoug that is a fair question. I'll have to investigate that issue.

[Jean85]

Closing as solved by #327