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

Skip auto-installing when the root package's extra.discovery is enough #237

Closed
nicolas-grekas opened this issue Apr 28, 2023 · 4 comments · Fixed by #239
Closed

Skip auto-installing when the root package's extra.discovery is enough #237

nicolas-grekas opened this issue Apr 28, 2023 · 4 comments · Fixed by #239

Comments

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Apr 28, 2023

Here is an idea that popped up in #236 and that builds on #232.

When the root package configures this:

    "extra": {
        "discovery": {
            "psr/http-factory-implementation": "My\\Psr17Factory"
        }
    }

then we shouldn't need auto-installing any extra packages for this virtual package.

Same when listing individual interfaces:

    "extra": {
        "discovery": {
            "Psr\\Http\\Client\\ClientInterface": "My\\Psr18Client"
        }
    }

In this case, we should verify that all the interfaces required to fulfill one of the supported *-implementation are listed.

@dbu
Copy link
Contributor

dbu commented Apr 28, 2023

thanks for the discussion in #236 . a workaround for now would be to do the composer config allow-plugins.php-http/discovery false from the readme, correct? and have the internal package provide the required *-implemenation information for composer.

I agree we can't check just for the *-implementation of installed packages because that still does not tell which classes to use. and scanning the code for implementing interfaces is too fragile (and a performance problem when no caching mechanism is available). while afaik java does support this sort of self-registration and autodiscovery mechanisms, i agree with not going that far.

the change suggested here would be that the existing configuration to chose which class to use for which psr factory also accepts unknown classes in addition to the well-known ones, right? that seems consistent with what we have and non-surprising.
(btw there is some doc that is only in the readme but missing from https://docs.php-http.org/en/latest/discovery.html, we should sync the 2 places when we do this change)

@nicolas-grekas
Copy link
Collaborator Author

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

correct!

the existing configuration to chose which class to use for which psr factory also accepts unknown classes in addition to the well-known ones

This works already!
The only thing missing is skipping the installation of well-know packages when all interfaces of the virtual package are listed.

@dbu
Copy link
Contributor

dbu commented Apr 28, 2023

oh even better. yeah then its not much left to be done 👍

and i just realized that you just added the pinning recently. neat, thanks!

@nicolas-grekas
Copy link
Collaborator Author

Implemented in #239

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

Successfully merging a pull request may close this issue.

2 participants