sentry-php
sentry-php copied to clipboard
getsentry
Replace php-http/message-factory
How do you use Sentry?
Sentry Saas (sentry.io)
Version
3.19.0
Steps to Reproduce
Execute composer require sentry/sentry.
Expected Result
I expect psr/http-factory is used and no warning appears.
Actual Result
./composer.json has been updated
Running composer update sentry/sentry
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Writing lock file
Installing dependencies from lock file (including require-dev)
Nothing to install, update or remove
Package php-http/message-factory is abandoned, you should avoid using it. Use psr/http-factory instead.
Generating autoload files
59 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
No security vulnerability advisories found
Using version ^3.19 for sentry/sentry
We currently rely on php-http/message-factory and can't easily remove this dependency without a likely breaking change. Closing as won't fix.
Hmm, can't we keep this issue open to track this? I'm trying to avoid using abandoned package dependencies.
Package php-http/message-factory is abandoned, you should avoid using it. Use psr/http-factory instead. I also would like to get this fixed.
It looks like this is a temporary solution anyway as @stayallive said, so I don't understand why the "won't fix" end-of-discussion answer here. Unless I misunderstood the other PR, if anything this will be fix "soon".
For some more context:
The Symonfy HTTP Client version 6.2 and below will throw an exception if php-http/message-factory is absent. See https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/HttpClient/HttplugClient.php#L48-L50
While this behaviour was removed in 6.3 and up, we can't force people to upgrade to the latest version via the SDK. We already ran into this issue in https://github.com/getsentry/sentry-php/issues/1533, which was caused by php-http/client-common removing this dependency.
Thank you @cleptric for the explanation, I think I understand why this is happening now.
In my case, I believe this will be fixed when 6.3 will be released (currently latest is RC2).
Hmm, can't we keep this issue open to track this? I'm trying to avoid using abandoned package dependencies.
While this behaviour was removed in 6.3 and up, we can't force people to upgrade to the latest version via the SDK.
You are not forcing anybody to do anything. People using outdated Symfony versions are free to also keep using older sentry versions that still rely on the abandoned package.
We currently support Symfony HTTP client version 4, 5 and 6. https://github.com/getsentry/sentry-php-sdk/blob/master/composer.json#L25
As I already explained, we cannot enforce the version of the Symfony HTTP client people use, hence versions 4, 5 and all versions prior 6.3 will throw an exception during runtime if we remove php-http/message-factory.
https://github.com/php-http/message-factory contains five interfaces and ships no actual implementation. This is a compromise I'm willing to accept for the time being.
I'm happy to review a PR if anyone has a better idea.
Can't people just require php-http/message-factory on their own if they need it?
You even suggested this yourself here.
This seems like a much better solution than forcing the abandoned package on everybody else, just because somebody misread the error message.
You cannot use "Symfony\Component\HttpClient\HttplugClient" as the "php-http/message-factory" package is not installed. is pretty self-explanatory.
Our security/compliance department is unhappy with us for having an abandoned package as a dependency.
We might cut a new major version in the coming weeks that would resolve this. Big emphasis on might, as there are a few constraints that would come with this.
Our security/compliance department is unhappy with us for having an abandoned package as a dependency.
Same. Unfortunately we've been forced to remove Sentry because of this.
Honestly, I can't understand why a security/compliance department would be so interested in complaining about the deprecation of a package that contains only interfaces and no code to maintain. Anyway, I also don't understand why we can't remove this dependency: from what I've read (I haven't followed the discussion much in the past), the problem was the initialization of the Symfony HTTP client which required the above package to be installed . The best solution in that case was to handle the initialization gracefully, not add a package that we're not even using directly (unless I'm blind)
@ste93cry Most security scanning is automated nowadays. Personally I think this should be taken care of by the package that's using the deprecated dependency, rather than asking consumers of the package to add exceptions to their security pipelines. Just my opinion though.
Removing a package just because it depends on a set of deprecated interfaces seems a bit exaggerated, but in general I agree with your thoughts, no doubts at all. Since we're not using those interfaces, unless I missed some references in the code, I don't understand @cleptric's choice of forcing the installation of that package instead of just handling the HTTP client initialization using a try/catch statement.
Honestly, I can't understand why a security/compliance department would be so interested in complaining about the deprecation of a package that contains only interfaces and no code to maintain.
The package has no problems right now... but it's no-longer maintained and if a problem is found in the future the lack of maintenance means nobody is going to fix it.
As to the issue in this specific case... I agree, there's not much real threat. But I'm not willing to make an exception and thus it's I'm committing myself and my team to waste time investigating periodically to check what's going on with this.
That's the real problem here - the wasted time caused by Sentry having an unmaintained dependency. And here I am, commenting on this issue, when I should be doing productive work. And everyone watching this will read my comment. More wasted time.
While it's a minor issue it's also one that affects literally everyone who uses PHP and Sentry, so it should be fixed sooner rather than later. It's now been over two months already. And the deprecation warning has crossed my radar hundreds of times (more than once a day).
If I wasn't so busy, I would have already forked sentry-php. But I'm really hoping I don't need to do that...
We'll kick off working on the new major this week, so an update that solves the mentioned issue will be not too far out.
I'm not willing to make an exception
If the php-http/message-factory repository had not been archived, you would have never known that those interfaces were not maintained anymore. World is full of compromises, it's up to you to discern which makes sense to accept and which not.
If I wasn't so busy, I would have already forked
sentry-php. But I'm really hoping I don't need to do that...
I would like to remind you that open source software lives on the contribution of people willing to dedicate their time to help develop it. While it is true that this package is backed by a company and there is a legitimate expectation that it will be maintained over time, this SDK has been developed primarily by open source contributors, so if you are concerned about this particular issue you can always step up and contribute to improve the situation.
We'll kick off working on the new major this week, so an update that solves the mentioned issue will be not too far out.
This still doesn't explain why you decided to add the package to the composer.json instead of just wrapping the client initialization in a try/catch block. I have nothing against releasing a new major version, but I'm failing hard to see why it's needed.
Note: I'm not affiliated with Sentry in any way, I'm just a contributor of this package with his own personal opinion
I explained this in https://github.com/getsentry/sentry-php/issues/1541#issuecomment-1568068745. Swallowing the exception doesn't help much, as in this case, no events will not be sent to Sentry, which is hard to surface to users.
How can swallowing the exception not help? The next HTTP client will be tried, so I don't see a particular issue with this. If none of the clients can be instantiated, there is nothing Sentry can do...
Requiring the package directly was done to solve issues like https://github.com/getsentry/sentry-php/issues/1533 and https://github.com/getsentry/sentry-laravel/issues/694. We opted to rather not crash users' applications.
Wouldn't a try/catch around the initialization of the Symfony HTTP client just catch the error and let the factory try the other HTTP clients?
+try {
if (class_exists(SymfonyHttplugClient::class)) {
...
return new SymfonyHttplugClient(SymfonyHttpClient::create($symfonyConfig));
}
+} catch (Throwable $exception) {
+ // Do nothing on purpose, so that the next HTTP client can be tried
+}
Using sentry/sentry-sdk and catching the raised exception will result in the same LogicException exception being thrown, as HttpAsyncClientDiscovery will resolve to SymfonyHttplugClient yet again.
You're right, if we assume that the user has no other HTTP client installed besides symfony/http-client. But is this enough to justify your choice of forcing an obsolete package for everyone else? I also wonder why php-http/message-factory isn't required in sentry/sentry-sdk. I realize this just moves the issue from one place to another, but at least the core SDK which does not depends at all from those interfaces remains clean.
I did consider using an abandoned package less severe than causing an exception.
Considering the amount of people complaining here and "+1" the first comment, maybe this decision should be reconsidered. What about my suggestion of moving the requirement of the abandoned package to the meta-package? If someone wants to require just the core SDK, at least they are fine 😃
The same can happen if someone uses the base SDK and requires the HTTP client manually or if it was installed via php-http/discovery.
@cleptric Would it be possible to make Sentry work even when php-http/message-factory is not installed? It can still be a dependency and used by default but that way those who need to avoid having an abandoned package in their stack due to company policies could simply add it to a replace block in composer.json and force it to not install.
The same can happen if someone uses the base SDK and requires the HTTP client manually or if it was installed via php-http/discovery.
If you're using the base SDK (and not the metapackage), then it's up to you to choose the HTTP client and install it. If you get an exception because there's a missing package that should have been required by that HTTP client, they should not complain here. What's happening here is that you're trying to solve an issue not of this package, causing complaints by all the rest of the people that are not impacted by that issue. The same can be said if you required php-http/message-factory in the metapackage, but if we can find a middle-ground solution, we should go for it.
@enumag The problem is that if we remove the package again, some people will run into the mentioned exception. So currently, we sadly have to live with the lesser evil. As said, this will be resolved soon 🙂
@ste93cry I already know you disagree, but Sentry's stance here is not to crash, even it the issue is technically not our fault.