do we ignorePsr\Http\Message\UriInterface deliberately?

Yes, it's not on the radar of comprehensive URI modelling.

do we ignorePsr\Http\Message\UriInterface deliberately?

We have a bridge for that. ✌️

Hi!

Before going forward with this PR, I feel like there is something we have to discuss first. I think it's good to do an open discussion on the prior art in the PHP community, to be transparent on why a new component would be necessary in the first place.

One library that is my mind immediately is the URI library provided by the PHP league:https://uri.thephpleague.com/ This one has also been brought up in the previous PR proposing a DNS component (#36999), and it looks like the goal and feature set of this proposed new component is very similar to this library.

Has there been any research in whether it is possible to use this library in Symfony instead? Both from a technical perspective as from a maintainer perspective? Have we talked with@nyamsprod about this?
At least from a maintainer perspective, the PHP league's standards already looks very similar to Symfony's and they are a proven organization for building and maintaining robust libraries. Maybe they are willing, with our help, to make the last steps to make Symfony able to use their library? (if there are any steps left?)

Thanks for bringing@nyamsprod in the discussion. I met him in Brussels a few months back and told him about this initiative. We could even borrow code, because the code made in the league package is impressive, hopefully with@nyamsprod blessings.

From a "package principles" pov, the League is mostly a bunch of talented individuals that don't work much together, so this doesn't fit for Symfony needs in terms of maintainership. But I'd be super happy to accompany anyone get into the Symfony community!

This new component has some pretty strong biases that run counter to what PHP currently offers (typically, support for “_”, “.” and “+” in query strings). It seems complicated to me to bring these biases intothephpleague/uri? In any case, I could understand if the maintainer didn't want to make these changes, which could be some hard breaking changes.

The original idea behind this component is the fact that DSN parsing within Symfony is duplicated code in many places. The first motivation for this component is to be able to support characters encoded in the user and pass, something that is handled individually in the parsing of each place where it's needed in the code. The idea is to factorize all this. Doesn'tthephpleague/uri seem to offer this feature at first glance? I could be wrong, I haven't looked at all the code in detail. 😄

From a "package principles" pov, the League is mostly a bunch of talented individuals that don't work much together, so this doesn't fit for Symfony needs in terms of maintainership. But I'd be super happy to accompany anyone get into the Symfony community!

Yet, we already use the library as a core dependency of one of our components:

Of course, we need to discuss this, but some core team members already maintain packages in the PHP league. So we've even solved a part of "what if this project becomes abandoned", as core team members within PHP league can take it over more easily than if Symfony had to fork the project (of course, if this happens we need to discuss and ask for permissions for this then).

So I think it's unfair to reject the idea solely based on this argument. Let's investigate the technical side as well (and check with the@nyamsprod about his opinion/idea of the matter).

to@alexandre-daubois,@wouterj and to everyone hi from Brussels! Since I have been mentioned I will quickly clarify some things.

I have not check the code but the premise for this component do align with those that guide me when developing league/uri. The league/uri ships with its own query parser and url parser for the same reason you are developing this package as far as I understood. This is illustrated in the first example on the documentation homepage for the package 😉 .
The goal of the library is to be as interop as possible and to follow as much as possible established RFC and standards. The packages, there are 3 of them, do a lot more but that's not the point of this response to elaborate on that.

In all fairness, I would have hope and prefer that the contribution could be made directly to The league/uri package but as@nicolas-grekas stated maybe the philosophy and the requirements between your organisation and my way of maintaining the package do not 100% aligned. That being said I believe they are parts of Symfony which do use my package already and I have already merged contributions from people from the Symfony in league/uri so to improve the interop between us. I even recall telling to@tgalopin about the incoming release of v7 so that he can prepare his component about the changes I was making. In other words, there are precedent for mutual participation/coordination in improving league/uri and Symfony.

Last but not least, I indeed met@nicolas-grekas in Brussels and he did ask me if I was OK if people were to use my code elsewhere. Back then I thought he was referring to another package of mine (https://github.com/bakame-php/http-structured-fields) as we discuss about it more in depth but to me it is all the same so I re-iterate the fact that I am ok with what suit you the best. I have no desire to impose anything on anybody, the choice is all yours.

I hope this lengthy response does answer or clarify any doubt from anyone. We are here for the better of PHP anyway so let's just do that one way or another!

peace out!

On another note you wrote. I just re-read the bullet points:

parse_str() overwrites any duplicate field in the query parameter (e.g. ?foo=bar&foo=baz will return ['foo' => 'baz']). foo should be an array instead with the two values. More info and examples can be found atStackoverflow.

I used this approach in an EOL version of league/uri and it is the wrong approach you should look into theLiving Standard. There you will see that the correct way to represents query string parameters is a collection of KeyValue Pair as those will guarantee that building and parsing query strings will always return the same value in the same order (might be crucial for URL signing if I understood correctly what you want to achieve).

league/uri-components has a PHP implementation of the Living standard you may use as a template or reference.

Refs#57190

Thank you very much for your message Ignace and for these very complete explanations! I think this component makes sense within Symfony, the APIs between your packages and this one are quite different. The purpose of this component is to respond to problems we've encountered in the Symfony codebase (the decoding of user and password being the first ones we wanted to solve) while remaining fairly minimal. When we wrote the specs for the component, we thought about what we wanted in it and also what we didn't want, to keep the scope fairly restricted. This is just my opinion, let's see other people's opinions of course! 😃

In any case, thank you very much for raising the query string point and avoiding making a mistake. I've updated to respect the URL Living Standard document. Also thanks for theleague/uri-components link, which helped me understand what it's all about!

what about templated URIs? was it considered?

@ro0NL yes! We considered it while writing the specs. I did not implement it in this first iteration, but it definitely has its place here in my opinion

In advance: I'm sorry to still be pushing back here. This is nothing against the work and thoughts that has been put in this PR.

I'm not yet convinced that we need a Uri component at all. Rather than building a slightly smaller component that borrows code and ideas from an existing library, I think we should always favor using the existing library if there are no major compelling reasons to not do so (and given it's already a dependency of Symfony, I guess we've already established in the past there aren't).

As an example, I've written a fully compatible version of theRedisTrait using the league libraries. As you can see, the difference in code with the patch in the PR description is almost negligible.

diff --git a/src/Symfony/Component/Cache/Traits/RedisTrait.php b/src/Symfony/Component/Cache/Traits/RedisTrait.phpindex 8183f92935..9e2163d73e 100644--- a/src/Symfony/Component/Cache/Traits/RedisTrait.php+++ b/src/Symfony/Component/Cache/Traits/RedisTrait.php@@ -11,6 +11,9 @@  namespace Symfony\Component\Cache\Traits;+use League\Uri\Components\Query;+use League\Uri\Components\UserInfo;+use League\Uri\Uri; use Predis\Command\Redis\UNLINK; use Predis\Connection\Aggregate\ClusterInterface; use Predis\Connection\Aggregate\RedisCluster;@@ -86,11 +89,8 @@ trait RedisTrait      */     public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): \Redis|\RedisArray|\RedisCluster|\Predis\ClientInterface|Relay     {-        if (str_starts_with($dsn, 'redis:')) {-            $scheme = 'redis';-        } elseif (str_starts_with($dsn, 'rediss:')) {-            $scheme = 'rediss';-        } else {+        $dsn = Uri::new($dsn);+        if (!\in_array($dsn->getScheme(), ['redis', 'rediss'], true)) {             throw new InvalidArgumentException('Invalid Redis DSN: it does not start with "redis[s]:".');         }@@ -98,68 +98,54 @@ trait RedisTrait             throw new CacheException('Cannot find the "redis" extension nor the "predis/predis" package.');         }-        $params = preg_replace_callback('#^'.$scheme.':(//)?(?:(?:(?<user>[^:@]*+):)?(?<password>[^@]*+)@)?#', function ($m) use (&$auth) {-            if (isset($m['password'])) {-                if (\in_array($m['user'], ['', 'default'], true)) {-                    $auth = rawurldecode($m['password']);-                } else {-                    $auth = [rawurldecode($m['user']), rawurldecode($m['password'])];-                }--                if ('' === $auth) {-                    $auth = null;-                }-            }--            return 'file:'.($m[1] ?? '');-        }, $dsn);--        if (false === $params = parse_url($params)) {-            throw new InvalidArgumentException('Invalid Redis DSN.');-        }--        $query = $hosts = [];--        $tls = 'rediss' === $scheme;+        $hosts = [];+        $query = Query::new($dsn->getQuery());+        $tls = 'rediss' === $dsn->getScheme();         $tcpScheme = $tls ? 'tls' : 'tcp';+        $userInfo = UserInfo::new($dsn->getUserInfo());+        $auth = null;+        if ($userInfo->getPass()) {+            $auth = [$userInfo->getPass()];+        }-        if (isset($params['query'])) {-            parse_str($params['query'], $query);+        if ($userInfo->getUser()) {+            $auth ??= [];+            array_unshift($auth, $userInfo->getUser());+        }-            if (isset($query['host'])) {-                if (!\is_array($hosts = $query['host'])) {-                    throw new InvalidArgumentException('Invalid Redis DSN: query parameter "host" must be an array.');+        if ($query->hasParameter('host')) {+            $hosts = $query->parameter('host');+            foreach ($hosts as $host => $parameters) {+                if (\is_string($parameters)) {+                    $parameters = Query::new($parameters);                 }-                foreach ($hosts as $host => $parameters) {-                    if (\is_string($parameters)) {-                        parse_str($parameters, $parameters);-                    }-                    if (false === $i = strrpos($host, ':')) {-                        $hosts[$host] = ['scheme' => $tcpScheme, 'host' => $host, 'port' => 6379] + $parameters;-                    } elseif ($port = (int) substr($host, 1 + $i)) {-                        $hosts[$host] = ['scheme' => $tcpScheme, 'host' => substr($host, 0, $i), 'port' => $port] + $parameters;-                    } else {-                        $hosts[$host] = ['scheme' => 'unix', 'path' => substr($host, 0, $i)] + $parameters;-                    }+                if (false === $i = strrpos($host, ':')) {+                    $hosts[$host] = ['scheme' => $tcpScheme, 'host' => $host, 'port' => 6379] + $parameters->parameters();+                } elseif ($port = (int) substr($host, 1 + $i)) {+                    $hosts[$host] = ['scheme' => $tcpScheme, 'host' => substr($host, 0, $i), 'port' => $port] + $parameters->parameters();+                } else {+                    $hosts[$host] = ['scheme' => 'unix', 'path' => substr($host, 0, $i)] + $parameters->parameters();                 }-                $hosts = array_values($hosts);             }+            $hosts = array_values($hosts);         }-        if (isset($params['host']) || isset($params['path'])) {-            if (!isset($params['dbindex']) && isset($params['path'])) {-                if (preg_match('#/(\d+)?$#', $params['path'], $m)) {-                    $params['dbindex'] = $m[1] ?? '0';-                    $params['path'] = substr($params['path'], 0, -\strlen($m[0]));-                } elseif (isset($params['host'])) {+        $host = $dsn->getHost();+        $path = $dsn->getPath();+        if ($host || $path) {+            if (!$query->hasParameter('dbindex') && $path) {+                if (preg_match('#/(\d+)?$#', $path, $m)) {+                    $query = $query->appendTo('dbindex', $m[1] ?? '0');+                    $path = substr($path, 0, -\strlen($m[0]));+                } elseif ($host) {                     throw new InvalidArgumentException('Invalid Redis DSN: query parameter "dbindex" must be a number.');                 }             }-            if (isset($params['host'])) {-                array_unshift($hosts, ['scheme' => $tcpScheme, 'host' => $params['host'], 'port' => $params['port'] ?? 6379]);+            if ($host) {+                array_unshift($hosts, ['scheme' => $tcpScheme, 'host' => $host, 'port' => $dsn->getPort() ?? 6379]);             } else {-                array_unshift($hosts, ['scheme' => 'unix', 'path' => $params['path']]);+                array_unshift($hosts, ['scheme' => 'unix', 'path' => $path]);             }         }@@ -167,7 +153,7 @@ trait RedisTrait             throw new InvalidArgumentException('Invalid Redis DSN: missing host.');         }-        $params += $query + $options + self::$defaultConnectionOptions;+        $params = $query->parameters() + $options + self::$defaultConnectionOptions;          if (isset($params['redis_sentinel']) && isset($params['sentinel_master'])) {             throw new InvalidArgumentException('Cannot use both "redis_sentinel" and "sentinel_master" at the same time.');

Another good reason to useleague/uri instead imho would be that we give the users aUri class that implements RFC-3986 and the URL Living Standard, rather than aUri class which is scoped specifically for internal usage of Symfony's DSN env vars. The latter can cause for issues when users decide to start using the Uri class for their own non-DSN use-cases.
And having only 29 small to medium sized classes inleague/uri andleague/uri-components together doesn't sound "bloated" enough to warrant a Symfony component with a smaller feature-set imho.

(and thanks@nyamsprod for your lengthy response!)

One thing that bothers me about the example you give for the Redis cache adapter is that it requires the addition of two external dependencies (uri anduri-components) instead of just one internal one. I think it's not ideal to have to add2 external packages to achieve the same result.

Even if this dependency already exists within the framework, do we want to further increase the coupling to this package? Again, to achieve the same result, the two dependencies will be needed in the different components to properly parse DSNs. Indeed,uri is required to parse the URL, anduri-components is required to parse the user info and do the decoding stuff. Seems heavy to me.

The latter can cause for issues when users decide to start using the Uri class for their own non-DSN use-cases.

I guess it is the same problem with every class in the framework, we cannot limit their usage if a user tries to use it for something else. But I may be missing something, did you have a particular example in mind?

Actually, in terms of deps-hell, there is a show stopper: this requires PSR-7. We went this way for HtmlSanitizer because it's a minor component, but that's nontheless still a problem to me. I certainly do not want to have to require PSR-7 and its myriad of other deps when using HttpClient, Cache, HttpFoundation (or even HtmlSanitizer).

To build on what Nicolas is saying. This PR suggest to introduce 6 new classes. We cannot compare that to the 75 classes added if you docomposer require league/uri-components

vendor/league/uri-components/IPv4Normalizer.phpvendor/league/uri-components/UriModifier.phpvendor/league/uri-components/Components/Domain.phpvendor/league/uri-components/Components/Port.phpvendor/league/uri-components/Components/Host.phpvendor/league/uri-components/Components/Authority.phpvendor/league/uri-components/Components/HierarchicalPath.phpvendor/league/uri-components/Components/Path.phpvendor/league/uri-components/Components/UserInfo.phpvendor/league/uri-components/Components/DataPath.phpvendor/league/uri-components/Components/Fragment.phpvendor/league/uri-components/Components/Component.phpvendor/league/uri-components/Components/Query.phpvendor/league/uri-components/Components/URLSearchParams.phpvendor/league/uri-components/Components/Scheme.phpvendor/league/uri-components/Modifier.phpvendor/league/uri-interfaces/FeatureDetection.phpvendor/league/uri-interfaces/Encoder.phpvendor/league/uri-interfaces/Contracts/UriInterface.phpvendor/league/uri-interfaces/Contracts/UriComponentInterface.phpvendor/league/uri-interfaces/Contracts/FragmentInterface.phpvendor/league/uri-interfaces/Contracts/IpHostInterface.phpvendor/league/uri-interfaces/Contracts/UserInfoInterface.phpvendor/league/uri-interfaces/Contracts/AuthorityInterface.phpvendor/league/uri-interfaces/Contracts/HostInterface.phpvendor/league/uri-interfaces/Contracts/PathInterface.phpvendor/league/uri-interfaces/Contracts/DomainHostInterface.phpvendor/league/uri-interfaces/Contracts/PortInterface.phpvendor/league/uri-interfaces/Contracts/UriException.phpvendor/league/uri-interfaces/Contracts/DataPathInterface.phpvendor/league/uri-interfaces/Contracts/QueryInterface.phpvendor/league/uri-interfaces/Contracts/UriAccess.phpvendor/league/uri-interfaces/Contracts/SegmentedPathInterface.phpvendor/league/uri-interfaces/KeyValuePair/Converter.phpvendor/league/uri-interfaces/UriString.phpvendor/league/uri-interfaces/Idna/Option.phpvendor/league/uri-interfaces/Idna/Result.phpvendor/league/uri-interfaces/Idna/Error.phpvendor/league/uri-interfaces/Idna/Converter.phpvendor/league/uri-interfaces/Exceptions/OffsetOutOfBounds.phpvendor/league/uri-interfaces/Exceptions/MissingFeature.phpvendor/league/uri-interfaces/Exceptions/ConversionFailed.phpvendor/league/uri-interfaces/Exceptions/SyntaxError.phpvendor/league/uri-interfaces/IPv4/GMPCalculator.phpvendor/league/uri-interfaces/IPv4/Calculator.phpvendor/league/uri-interfaces/IPv4/Converter.phpvendor/league/uri-interfaces/IPv4/BCMathCalculator.phpvendor/league/uri-interfaces/IPv4/NativeCalculator.phpvendor/league/uri-interfaces/QueryString.phpvendor/league/uri/UriTemplate.phpvendor/league/uri/Http.phpvendor/league/uri/HttpFactory.phpvendor/league/uri/Uri.phpvendor/league/uri/UriInfo.phpvendor/league/uri/BaseUri.phpvendor/league/uri/UriResolver.phpvendor/league/uri/UriTemplate/Expression.phpvendor/league/uri/UriTemplate/TemplateCanNotBeExpanded.phpvendor/league/uri/UriTemplate/Operator.phpvendor/league/uri/UriTemplate/VarSpecifier.phpvendor/league/uri/UriTemplate/Template.phpvendor/league/uri/UriTemplate/VariableBag.phpvendor/psr/http-message/src/ServerRequestInterface.phpvendor/psr/http-message/src/UriInterface.phpvendor/psr/http-message/src/StreamInterface.phpvendor/psr/http-message/src/UploadedFileInterface.phpvendor/psr/http-message/src/RequestInterface.phpvendor/psr/http-message/src/ResponseInterface.phpvendor/psr/http-message/src/MessageInterface.phpvendor/psr/http-factory/src/ResponseFactoryInterface.phpvendor/psr/http-factory/src/StreamFactoryInterface.phpvendor/psr/http-factory/src/UploadedFileFactoryInterface.phpvendor/psr/http-factory/src/UriFactoryInterface.phpvendor/psr/http-factory/src/ServerRequestFactoryInterface.phpvendor/psr/http-factory/src/RequestFactoryInterface.php

@nicolas-grekas I understand your hesitation about PSR-7 for the record I never used the PSR-7 aspect of the library. And the library predate PSR-7 UriInterface and UriFactoryInterface (which are in fact the only 2 interfaces used, since they are related to URI).

I only added PSR-7 so that the library could provide better interop with the PHP community (which is an underlying goal of the library). So if that was the one show stopper I believe then there's a case of moving that require dependency to an suggestion. But then again this would require a discussion before acting on that. As I stated

In other words, there are precedent for mutual participation/coordination in improving league/uri and Symfony.

So even opening a ticket to discuss this matter on the package would have IMHO clarify things or given a better context I supposed in your own decision making process.

But then again I want to completely stress out that I totally inderstand and respect your position/decision but I wanted to clarify this point.

we already use the library as a core dependency

noted, i tend to agree we should try to avoid using 2 URI libraries in core

perhaps we should invert the discussion a bit;

will symfony/uri be useful to league/uri? respectively being low-level and high-level URI libraries.

either way, i also tend to believe once symfony/uri is a thing, PSR-7 support will be requested sooner or later. which sounds fair.

Thinking a bit more about this and reading the new comments here and inthephpleague/uri-src#137, I feel more and more like the proposed component is, at the moment, badly named and this makes it hard to discuss the matter.

If we want something that is compatible with URI specs and implements them correctly, I think we're at fault if we try to reinvent the wheel ourselves and should collaborate withleague/uri on this. Either us using that package directly instead of a new component, or Symfony adoptingleague/uri as a component - as suggested. I can't see how learning 2 specs from scratch and writing PHP code to comply with them is better than collaborating with people having the code and years of experience already.

However, if our goal is only to ease and standardize internal parsing of DSNs and not necessarily comply with the full URI specs, we should probably not useleague/uri but instead write a very simple class that only deals with "our DSN spec" (e.g. removeFragmentText and keep the:// simplification) and get back to calling the component Dsn. This clearly signifies "this is a mostly internal component to parse DSNs, and if you need to parse URIs in your application, please use another library".

@wouterj I understand where you are coming from, but "we just need DSNs" seems like avoidance to me. I would like to see this be a point of collaboration instead of fragmentation.

@shadowhand yes, the first suggestion (2nd paragraph) is what I prefer as well.
But I have the feeling the second suggestion (3rd paragraph) is what is actually proposed in the diff of this PR. I hope highlighting this makes it easier to reason about it (as this discussion kept comparing complexity in this PR vs league/uri, which is an unfair comparison).

Btw, this is also the conclusion of a previous discussion about this when discussing a Dsn component:#36999

@wouterj I read through the various PRs that you referenced. It seems that:

1. There is no consensus on what a DSN is in Symfony.
2. An existing solution exists in the URI RFC, which will work formost existing DSNs.

Thus far there have been at least 3 distinct efforts to create a DSN/URI component and all have failed on this same point about whether or not DSN should be considered a URI (in the RFC sense).

There's undeniably a need for such a component, as the DSN parsing logic is present in multiple other components in the code and it comes with a lot of duplicated code with different support levels of features. This is why@wouterj's comment:

but instead write a very simple class that only deals with "our DSN spec" (e.g. remove FragmentText and keep the :// simplification) and get back to calling the component Dsn

is relevant to me. Let's remember that the main aim is to factorize DSN parsing logic into the Symfony codebase (with the auth decode thing), and that support for more general URI parsing is a "consequence" of this need. I'm curious to know what maintainers think about this?

I know that some components are already focused (initially I mean) to bring some features "internally", like PropertyInfo and PropertyAccess are for Serializer.

FTR, DSN parsing in each component as is done currently is fine to me. I'm not looking for a way to factorize DSN parsing, as this is not a domain on its own. Having some consistency in the way DSN are designed is nice, but isn't a requirement. DSNs are not the point here to me.

But I know we need a way to parse URLs that's free from PHP's oddities (listed in the description of this PR), and free from issues likephp/php-src#12703.

About the discussion related to league/uri, I'm listening carefully to the presented arguments. Still thinking about the topic.

Alright, thanks for clarifying. Right now, I'm able to fix the oddities that have been brought up without bringing complexity to the code. I think it worth challenging technically this PR to confirm if we can keep its simplicity while fixing what's been listed in the description. I still think that this approach, here, is worth it.

as this discussion kept comparing complexity in this PR vs league/uri, which is an unfair comparison

I don't agree with this point, I feel it is an important point both for maintainers and end-users. I personally think it is great to have the choice between multiple implementations as the end-user: some prefer to customize everything, some prefer more straightforward ways. I don't see this component and league/uri as competitors but rather as complementary, where you can choose which approach suits you best. This is why I think that comparing the complexity of those two packages is relevant.

Mate posted a proposal to add a native compliant URI parser to the engine!
https://externals.io/message/123997

This might close the topic in the best possible way :)

Definitely, let's see where it goes!

How difficult would it be to turn the code of this PR into a polyfill for the proposed new URL extension?

@derrabus a polyfill is not needed there's already a great implementation in PHP viahttps://github.com/TRowbotham/URL-Parser . But I would be really cautious about using that package by default or any package that just follow the WHATWG spec. I posted my remarks onhttps://externals.io/message/123997#124013

If we want to leverage that new extension, we will need a polyfill. Otherwise the extension will be unavailable to use until Symfony 9 possibly.

e.g. ?foo=bar&foo=baz will return ['foo' => 'baz']). foo should be an array instead with the two values.

foo[]=bar&foo[]=baz should return an array but for mefoo=bar&foo=baz returning['foo' => 'baz'] is correct

Let me close here, since we now have a plan: rely on php-src adding URI parsing. Let's try to have a polyfill instead.
Thanks@alexandre-daubois for all the work that happened here, and to@nyamsprod and everybody else involved in the discussion.