Replace php-http/message-factory

[mrsombre]

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

[cleptric]

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.

[michaelarnauts]

Hmm, can't we keep this issue open to track this? I'm trying to avoid using abandoned package dependencies.

[nicogominet]

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".

[cleptric]

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.

[nicogominet]

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).

[rintdev]

Hmm, can't we keep this issue open to track this? I'm trying to avoid using abandoned package dependencies.

[gndk]

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.

[cleptric]

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.

[gndk]

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.

[devicenull]

Our security/compliance department is unhappy with us for having an abandoned package as a dependency.

[cleptric]

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.

[binaryfire]

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.

[ste93cry]

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)

[binaryfire]

@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.

[ste93cry]

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.

[abhibeckert]

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...

[cleptric]

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.

[ste93cry]

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

[cleptric]

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.

[ste93cry]

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...

[cleptric]

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.

[ste93cry]

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
+}

[cleptric]

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.

[ste93cry]

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.

[cleptric]

I did consider using an abandoned package less severe than causing an exception.

[ste93cry]

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 😃

[cleptric]

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.

[enumag]

@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.

[ste93cry]

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.

[cleptric]

@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.

[ste93cry]

Sentry's stance here is not to crash, even it the issue is technically not our fault.

It's quite difficult to account for all possible bugs in technical implementations of third party packages to live up with this premise... Sometimes, being a bit more flexible is the best way to go forward 😉

[cleptric]

Agreed, but sadly not in this case.

[cleptric]

1576 might solve this quicker, but there are still some concerns about the solution.

If you want to help us verify this change, you can install sentry/sentry: dev-support-symfony-psr18-http-client and let us know if you run into any issues. Maybe do not ship this straight to production 🙂

[dakujem]

If the only issue is the use of abandoned package, then follow these 4 simple steps instead of uninstalling Sentry 🤦‍♂️:

1. fork php-http/message-factory
2. use the replace feature of Composer
3. redistribute under your vendor name
4. install your new package

Done in 15 minutes.

---

But otherwise, I would also like to get rig of the warning, that's how I ended in this thread.

[mbrodala]

So it would not be possible to have the next release of sentry/sdk depend on "symfony/http-client": "^4.3|^5.0|^6.3"? This would not be a breaking change, existing Symfony 6.2 installations will continue to work since the update won't be installed. Symfony 6.3 installations would get the update without noticing any difference.

Also see What should I do if I update my own dependencies without changing the public API?

[nexik]

FYI: The next version of Composer (2.7) composer audit (often used in CI) will fail by default because of abandoned packages:

The new audit.abandoned setting (currently defaulting to "report" will default to "fail" 
in Composer 2.7, make sure to set it to "report" or "ignore" explicitly by then if you do not want this.

[dakujem]

@nexik Do they have some kind of a roadmap? I can't see the information anywhere.

UPDATE: I will answer myself: https://getcomposer.org/changelog/2.6.3

[c-o-m-m-a-n-d-e-r]

any progress here? :-) thanks

[cleptric]

Yep, this was already fixed in the next major version of the SDK.

[abhibeckert]

Yep, this was already fixed in the next major version of the SDK.

What version is that? And when will it be released?

Also, what is the workaround while we wait? Obviously removing sentry from my project would work but that seems like throwing the baby out with the bath water...

[nicwortel]

FYI for anyone who cannot or does not want to ignore the abandoned package warnings from Composer while waiting for the next major release of the Sentry SDK:

If you are not using Symfony < 6.3, and you have no other packages or code depending on php-http/message-factory, I believe the easiest way to prevent the package from being installed is by adding the following lines to the root composer.json of your application (so no need to fork any packages):

    "replace": {
        "php-http/message-factory": "*"
    },

Next, run:

$ composer update php-http/message-factory

This uses Composer's replace feature to let Composer conclude that the dependency on php-http/message-factory is fulfilled by another package (your application), and will remove the package from your vendor directory.

Obviously this will break things if your code (or any of your other dependencies) actually uses any of the interfaces provided by php-http/message-factory, so use this with caution.

To make sure none of your dependencies depend on the package use composer why:

$ composer why php-http/message-factory

If only sentry/sentry is listed and you know you are not using these interfaces in your own code, you should be safe.

Nevertheless, don't forget to remove the replace snippet when you upgrade to the new major version of the Sentry SDK as it is released.

[cleptric]

@abhibeckert We aim for a release later this week/early next week. This will, however, only affect users who don't use our Symfony/Laravel SDK, as we will need to cut new majors there as well.

[cleptric]

4.0.0 shipped and removed php-http/message-factory.

[ste93cry]

I still think you should release the PR I prepared for version 3.x of the SDK. Not all people will immediately upgrade to the latest major and will face an error as soon as Composer starts blocking the installation of deprecated dependencies (it will happen soon). Also, as explained in my first comment, the change should be backwards compatible, so there's no reason to insist on not merging it.

[cleptric]

this PR should be backwards compatible

This is exactly the reason why I haven't merged this yet.

[ste93cry]

I was cautious, but if you read the description of the PR you will see that it's not possible to install the SDK with a version of the framework that does not support the required HTTP client.

[cleptric]

We released version 4.0.0 of our Laravel SDK, which includes the version bump to 4.0.0 of the PHP SDK, hence it does not require php-http/message-factory anymore.

[stayallive]

Sorry for the confusion, we shall keep this open until getsentry/sentry-symfony#783 is finished.

[cleptric]

With the release of version 5.0.0 of our Symfony SDK, all current versions of our PHP SDKs do not longer require php-http/message-factory or any other of packages of this kind.