Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto require package I do not need #236

Closed
bobzhangyong opened this issue Apr 27, 2023 · 8 comments
Closed

Auto require package I do not need #236

bobzhangyong opened this issue Apr 27, 2023 · 8 comments

Comments

@bobzhangyong
Copy link

bobzhangyong commented Apr 27, 2023

PHP version: x.y.z (hint: php --version)

PHP 7.4.11 (cli) (built: Mar 4 2022 15:04:46) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies

Description

My project need a private package petrel/opentelemetry, this package require open-telemetry/opentelemetry version 0.0.17

open-telemetry/opentelemetry require php-http/discovery, php-http/async-client-implementation, psr/http-factory-implementation

In my project ready have private package petrel/http-client provide php-http/async-client-implementation and psr/http-factory-implementation

But when I run composer update, this packages will add to my project automatic, and change my composer.json

        "guzzlehttp/promises": "^1.5",
        "nyholm/psr7": "^1.7",
        "php-http/message-factory": "^1.1",
        "symfony/http-client": "^5.4"

I think the problem is php-http/discovery do not check private provide packge.

Any idea to fix this problem? I can only change petrel/opentelemetry, can not change open-telemetry/opentelemetry

Thank you

@nicolas-grekas
Copy link
Collaborator

The role of the plugin is to provide some nice DX so that e.g. this line works out of the box:
https://github.com/open-telemetry/opentelemetry-php/blob/5f318cdb7ac299054c23996b3276801849551538/src/SDK/Common/Adapter/HttpDiscovery/PsrClientResolver.php#L27

If you don't want that behavior, you should disable the plugin by running:

composer config allow-plugins.php-http/discovery false

@bobzhangyong
Copy link
Author

We have more than 100 projects use petrel/opentelemetry package, it's not easy to inform all user add config in their project composer.json conf
Do you have any other idea?

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Apr 27, 2023

I don't think there is a solution that belongs to the petrel/opentelemetry package: it's very possible, in theory at least, that open-telemetry/opentelemetry would be required by another dep that doesn't rely on the wiring provided by petrel/opentelemetry, so that the discovery mechanism would still need a package it knows how to use to work properly.

This means that to me, only the root package can know how to solve this. One way is to disable the plugin. You could very well document that in the readme of your private package. If people don't read it, it's not a big deal either. They'd just get a maybe-unused dep.

The alternative, but I would reserve it to the root package also, would be to build on #232 so that when all interfaces required to provide a virtual *-implementation are listed in the extra.discovery entry, then the plugin shouldn't need to install anything. (But this wouldn't solve your concern.)

@bobzhangyong
Copy link
Author

Each package that implements php-http/async-client-implementation will set the provide field in their composer.json file. When php-http/discovery looks for dependencies, it can check all the provide fields and if there is an implementation for php-http/async-client-implementation, there is no need to add a new package.

Can we do like this ?

@nicolas-grekas
Copy link
Collaborator

I thought about that, but there are two issues:

  • it can lead to conflicts, when two packages declare the same *-implementation, which one should be chosen?
  • and more critically, it can lead to security issues, because a package could declare a *-implementation and be hooked into the runtime flow of an app without the dev knowing...

@bobzhangyong
Copy link
Author

Your concerns are reasonable. Well, I don’t have a better solution to this problem either.
I will close this issue.
Thank you for your reply.

@nicolas-grekas
Copy link
Collaborator

Thanks. I created #237 to keep track of the proposal in #236 (comment)

@nicolas-grekas
Copy link
Collaborator

See #239 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants