Reorder priorities of PSR-17 factories

[boesing]

Description

Hey there,
I recently ran into an issue regarding urlencode of userInfo. Due to whatever reasons, nyholm/psr7 does not encode the password and thus, curl returns error code 3 (malformed URI) when executing a request with a password containing characters which need to be urlencode.

Since nyholm/psr7 did not received any updates for quite a long time, I assume it is either not maintained or the pace of fixing issues reduced. Since there are PSR-17 implementations of industry approved libraries, I am not quite sure why nyholm/psr7 has almost the highest priority when detecting PSR-17 factories.

For example:

• guzzle/psr7
• slim/psr7
• laminas/diactoros

When it comes to PSR-18, symfony and guzzle have the highest priorities and there are no doubts. I'd expect guzzle to always overrule nyholm/psr7 as well.

Example

$uri = new \Nyholm\Psr7\Uri('http://example.org');
$uri = $uri->withUserInfo('username', 'password#');

echo (string) $uri; // http://username:password#@example.org => when used with curl, curl error 3 malformed uri

$uri = new \GuzzleHttp\Psr7\Uri('http://example.org');
$uri = $uri->withUserInfo('username', 'password#');

echo (string) $uri; // http://username:password%23@example.org => when used with curl, all good

Additional context

It also seems that @Nyholm is not interested in interoperability of his component.

I just quickly checked PSR7 specification and it does not mention anything about URL encoding.

Nyholm/psr7#202 (comment)

I see no reason to change unless a PSR is updated with clear guidelines.

Nyholm/psr7#99 (comment)

When I understand this discovery component correct, it is expected that every discovered PSR-18/PSR-17 implementation does support the same functionality as if not - it won't be interchangeable.

Since in the mentioned comments above the response was "I dont care unless PSR cares" while the authors of the issues stated that other implementations actually do support these (so not just one), I frankly would prefer having nyholm/psr7 either removed or the priority should be lowered so that other components are picked up first.

[nicolas-grekas]

nyholm/psr7 just happens to be stable. It is well maintained AFAIK. Did you submit an issue there? What about a fix?

[nicolas-grekas]

Looks like I missed following the links you submitted 😬
Let's revive those.

[boesing]

Well, especially for the mentioned error above, a PR does exist since August 2022.
But instead of going the way, as the other implementations, @Nyholm suggested to throw errors when "invalid" characters are passed. Due to the interoperability idea of this discovery component (correct me if I'm wrong, because if that is not the idea, I might not get it at all), I still expect most if not all discovered implementations behave the same.

Especially in my recent case, everything worked well due to the fact that the discovery seemed to detect guzzle PSR-7 implementation. Due to a transitive dependency, both php-http/discovery and nyholm/psr7 (first required by elasticsearch, second by another 3rd-party library we recently updated) are installed in our project and thus, we were not even aware that the discovery magic ever happened. However, since nyholm/psr7 has the 2nd highest priority in this discovery, elasticsearch stopped working for us due to the production password contains #.

Only thing changed was composer.json and composer.lock. That was really surprising and I can't believe that this is how its meant to work.

---

I wonder if it would work to actually have a config file in the project to enforce this component to always detect the same implementations regardless of what is installed. I am aware of the fact that I can modify the discovery strategies at runtime but I'd prefer my code (besides a config file) to not interact with this component at all (as we do not directly depend on it but via transitive dependencies).

[nicolas-grekas]

I see several next steps:

• continue this discussion with nyholm/psr7
• make elasticsearch + that other dep use discovery instead of them hardcoding a dep
• I like your idea to pin a preference. It'd do it with an entry in composer.json, read by the composer plugin, which would then generate some file to configure the factory on bootstrap.

Would you like to help on these? I contacted @Nyholm to talk about item 1.

[boesing]

continue this discussion with nyholm/psr7

I will, but it seems to be a one-way discussion since I somehow understand the standpoint of "if PSR does not say so, we do not implement it" but then the interoperability is not given 🤷🏼‍♂️ The suggested way forward with throwing InvalidArgumentException in nyholm/psr7 is not what I'd expected.

make elasticsearch + that other dep use discovery instead of them hardcoding a dep

Elasticsearch is already using the discovery component. Regarding that other dep, I am already on it. Its some company internal thingy and thus I have (limited) ways to pursue this problem. 👍🏼

Would you like to help on these?

Definitely. Not sure if a composer plugin is a good idea tho. Was more thinking of some kind of PHP file where a proper mapping could happen.

Something like:

php-http-discovery.config.php

<?php
return [
    'static-mapping' => [
        \Psr\Http\Client\ClientInterface::class => \GuzzleHttp\Client::class,
        // PSR-17 below, [...]
    ],
];

This way, IDE, static-analysis, etc. can already show if stuff is available or not without having to run any runtime checks. When adding stuff to composer.extra for example, having class-names stored in JSON is usually not that painfree while it also needs proper CI setup with plugins allowed to run, etc.
The PHP file would also be opcachable and thus the amount of additional I/O won't be that much.

[nicolas-grekas]

This package is already a composer plugin so it would be super natural to use the extra entry in composer.json for this.

[boesing]

Ah, didn't realized that tbh. Seems that it was added with 1.15 but we are still on 1.14. Well, then it would be doable ofc.
So what would you expect the plugin to do? Not sure if I understand the goal.

So I can add in composer.extra.http-discovery a dependency?
For example:

{
    "extra": {
        "http-discovery": {
            "psr/http-client-implementation": "guzzlehttp/guzzle",
            "psr/http-factory-implementation": "guzzlehttp/psr7",
            "...": "..."
        }
    }
}

So that it can be extracted by the plugin, which then can verify if the added implementation is part of the PROVIDE_RULES AND the package is installed in the required version (defined in PROVIDE_RULES as well as far as I can see).
But the main question is, where to put that information so that it will actually gets picked-up by the discovery classes.

I am thinking of a PinnedByProjectStrategy which is generated by the composer plugin and prepended to the factories.
Or do you have something else in mind?

[Nyholm]

I think there are multiple definitions of "interoperability" floating around. nyholm/psr7 has always been about following the PSR and RFC specifications. It does not add any other features.

I do agree with you that there is something missing here from PSR-7 and that sucks.

I think the issue here is that you rely on features in Guzzle and expect them to be implemented everywhere. Instead you should rely on features in PSR-7.

---

I understand your frustration, I understand it is not easy. nyholm/psr7 is not opinionated, it is strict

---

I do agree with you that we should not silently fail. We definitely need Nyholm/psr7#202

[boesing]

What should not happen at all is that just because a composer depenency is being added or removed, the whole way of how stuff is interacting with PSR changes.

I do not rely on Guzzle features but on features which are supported by guzzle, laminas, slim and phalcon (so literally by ALL other implementations supported by this discovery package). So you can ofc say "well, I still don't care" and thats absolutely okay, but then please move the priority of your package to the very end of the discoverable implementations.

[Nyholm]

I do understand what you mean. I do understand your frustration. I do think something should be changed. But you are still wrong =)

If you write code using an interface. The contract ("api") is with that interface. It does not matter what all/most implementations are doing. You can never trust the implementation.

I agree that it is strange that the interface does not specify the super common feature you are asking for.

Anyhow, it does not matter if you see it the same way that I do or not. We do need change as you suggest.

@nicolas-grekas can you please make a decision what the best way forward is and take action?

[nicolas-grekas]

My reco is to go with Nyholm/psr7#213

[dbu]

thanks for the constructive discussion, i appreciate it!

in terms of interoperability, indeed relying on the urlencoding seems to be the "mistake", as it is not specified in PSR-7. in an ideal world, PSR-7 would be more precise, or even come with a comprehensive test suite that guarantees consistent behaviour of all compliant implementations.

i like nicolas' suggestion to adjust nyholm/psr7 to work closer to the other implementations, and wonder if we can propose to upgrade psr-7 to be specific about this behaviour for future versions of psr7?

the auto-installation that @nicolas-grekas contributed to this discovery package already chooses the matching psr7 package, e.g. in a project with laminias the laminas packages or with guzzle the guzzle psr 7.

i wonder if we should instead of a strict order of preference say that if one of the major psr18 client is available, we use the psr7 implementation of that client. in theory, all psr7 implementations should work with all psr18 clients, but in practice the operation of laminas with its psr7 or guzzle with its psr7 are the cases that are the most well tested. wdyt?

[dbu]

i created php-fig/fig-standards#1298, lets see

[nicolas-grekas]

I'm closing here because I think we resolved the topic. I opened #223 to keep track of the idea of pinning.