[HttpFoundation] Add `Url` and `UrlParser` #53346

alexandre-daubois · 2024-01-02T12:46:48Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

This PR #53320 raised a problem in the codebase. Each place in the code where a DSN is parsed has its own implemented logic.

This led to various consistency issues: variable names differing from one place to another but having the same meaning, different behavior (some places decode HTML characters in URLs while others don't), work done "twice"... And of course, if a bug is detected somewhere, you have to check that it's not present everywhere (as must have been done with the PR mentioned above).

@OskarStark and @nicolas-grekas raised that there could be some place for something that unifies the work of DSN parsing which is of course always the same: parse the URL, decode the auth part with special chars, check a scheme is present, check the host is optionally present also.

Here is a first take on this. I created the logic in HttpFoundation, because why not, but I don't think this is the best place. However, we don't have any idea on where to put this as you can see in the comments of the linked PR 😄 I'm even sure that this is not the right place because, to me, add HttpFoundation as a dependency for this DSN parser feels weird.

A new component might be the solution, but it seems it was already discussed in the past and it was rejected (note that I couldn't find info about this, so if you were part of the discussion, feel free to link some issue or PR, thanks!).

This PR will give you an idea of what could be possible. I already leveraged this new mechanism in DI, Notifier, Mailer and Translation so you can see this in action. Of course, if this get accepted, the next step will be to leverage this in the Cache component mainly.

Things to discuss:

What are your thoughts on the API?
Where should we put this (new component, in an existing component, something else)?
Which additional feature should be bundled in Parser or Parameters?

OskarStark · 2024-01-02T13:02:20Z

src/Symfony/Component/HttpFoundation/Dsn/Parameters.php

+/**
+ * @author Alexandre Daubois <alex.daubois@gmail.com>
+ */
+final class Parameters implements \Stringable


I would still name this àI would name this class Dsn`

OskarStark · 2024-01-02T13:03:38Z

src/Symfony/Component/Mailer/Tests/Transport/DsnTest.php

@@ -92,17 +92,17 @@ public static function invalidDsnProvider(): iterable
    {
        yield [
            'some://',
-            'The mailer DSN is invalid.',
+            'The "some://" DSN is invalid.',


Exposing the DSN here means it could leak sensitive data to a log file

xabbuh · 2024-01-02T13:06:11Z

see #32546 for a related discussion

Nyholm · 2024-01-02T13:07:50Z

Here is a link to a PR I made to try introduce a DSN component.
#36999

Symfony is all about following official standards. There is no standard for DSN (believe it or not). That is why it might be hard to push for a dedicated DSN component.

nicolas-grekas · 2024-01-02T13:10:06Z

💯 That's why I wrote we might need a URL parser, not a DSN parser ;)

alexandre-daubois · 2024-01-02T13:12:40Z

Alright I see the distinction. A bit of renaming is needed here. An URL parser could belong to HttpFoundation, as it already has some "UrlHelper".

OskarStark · 2024-01-02T13:26:46Z

Symfony is all about following official standards. There is no standard for DSN (believe it or not). That is why it might be hard to push for a dedicated DSN component.

I understand, but we created our own standard of DSN in the Symfony world, which is spread across different components

nicolas-grekas · 2024-01-02T13:31:17Z

we created our own standard of DSN in the Symfony world

That's not the case, each DSN in Symfony is potentially adhoc for the components that need one. There are already differences that are hard to abstract.

Nyholm · 2024-01-02T13:33:29Z

Symfony is all about following official standards. There is no standard for DSN (believe it or not). That is why it might be hard to push for a dedicated DSN component.

I understand, but we created our own standard of DSN in the Symfony world, which is spread across different components

Yes, that work has been implementation driven. I looked at how Symfony is using DSN and I created a specification for it. But making it a dedicated component says more "This is how Symfony think DSN should work". If we are just implementing DSN in various other components means we can eat the cake and have it too (ie don't agree on a standard but use DSN as we like).

Im obviously not against a DSN component or let a official DSN parser be part of HTTPFoundation, a new URI component. Im just trying to give some background or a TLDR for #36999 and #32546.

alexandre-daubois · 2024-01-02T13:34:53Z

There are already differences that are hard to abstract.

Would you have an example? From what I saw during the rawurldecode URL, "everything" seems pretty aligned. Isn't the differences something that could be solved with some good polymorphism?

Im just trying to give some background or a TLDR

Thank you for that! 🙏

Nyholm · 2024-01-02T13:36:17Z

Would you have an example?

It is mostly the "function" part. Some components supports DSN like:

failover(dummy://a dummy://a)
failover(dummy://a,dummy://a)
failover:(dummy://a,dummy://a)
roundrobin(dummy://a failover(dummy://b dummy://a) dummy://b)

alexandre-daubois · 2024-01-02T13:38:41Z

Just a quick thought but shouldn't we just use Tobias' package in the framework?

smnandre · 2024-01-02T19:03:07Z

src/Symfony/Component/HttpFoundation/Exception/Parser/InvalidUrlException.php

+
+class InvalidUrlException extends \InvalidArgumentException
+{
+    public function __construct(string $dsn)


string $url ?

parameter can be removed IMHO

smnandre · 2024-01-02T19:11:16Z

src/Symfony/Component/HttpFoundation/UrlParser/Url.php

+        $dsn = $this->scheme.'://';
+
+        if ($this->isAuthenticated()) {
+            $dsn .= rawurlencode($this->user).':'.rawurlencode($this->pass).'@';


Will throw if one of this->user or this->pass is null, as rawurlencode() requires a string

Good catch 👍

smnandre · 2024-01-02T19:13:21Z

src/Symfony/Component/HttpFoundation/UrlParser/Url.php

+    public function __construct(
+        public string $scheme,
+        public ?string $user = null,
+        public ?string $pass = null,


Could we use password here ?

We could, but this would make impossible to do the following:

$url = new Url(...parse_url($url));

This is why I kept pass here

the goal of the new code should be to completely replace using parse_url, so this argument is void sorry. (parse_url is broken)

Oh yes, of course 🤦 Updated accordingly

welcoMattic · 2024-01-08T09:05:23Z

Is HttpFoundation the best place for such a parser? It will be used on Notifier, Translation, etc, in not necessarily HTTP request related context. I'm just back from holidays, maybe the topic has already been discussed elsewhere, but why not creating a small component named UrlParser?

alexandre-daubois · 2024-09-18T09:15:31Z

Closed in favor of symfony/polyfill#498

alexandre-daubois requested review from welcoMattic and OskarStark as code owners January 2, 2024 12:46

carsonbot added Status: Needs Review Feature HttpFoundation labels Jan 2, 2024

carsonbot added this to the 7.1 milestone Jan 2, 2024

alexandre-daubois force-pushed the dsn-parser branch 5 times, most recently from 7adb171 to 1407a55 Compare January 2, 2024 12:59

OskarStark requested a review from Nyholm January 2, 2024 13:00

OskarStark reviewed Jan 2, 2024

View reviewed changes

alexandre-daubois force-pushed the dsn-parser branch from 1407a55 to 303a8c9 Compare January 2, 2024 13:32

alexandre-daubois changed the title ~~[HttpFoundation] Add Dsn\Parser and Dsn\Parameters~~ [HttpFoundation] Add Url and UrlParser Jan 2, 2024

smnandre reviewed Jan 2, 2024

View reviewed changes

alexandre-daubois force-pushed the dsn-parser branch from 303a8c9 to db7c57e Compare January 3, 2024 07:26

[HttpFoundation] Add UrlParser and Url

31232ee

alexandre-daubois force-pushed the dsn-parser branch from db7c57e to 31232ee Compare January 3, 2024 07:31

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

alexandre-daubois closed this Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Add `Url` and `UrlParser` #53346

[HttpFoundation] Add `Url` and `UrlParser` #53346

alexandre-daubois commented Jan 2, 2024 •

edited

Loading

OskarStark Jan 2, 2024

OskarStark Jan 2, 2024

xabbuh commented Jan 2, 2024

Nyholm commented Jan 2, 2024

nicolas-grekas commented Jan 2, 2024

alexandre-daubois commented Jan 2, 2024

OskarStark commented Jan 2, 2024

nicolas-grekas commented Jan 2, 2024

Nyholm commented Jan 2, 2024

alexandre-daubois commented Jan 2, 2024 •

edited

Loading

Nyholm commented Jan 2, 2024

alexandre-daubois commented Jan 2, 2024

smnandre Jan 2, 2024

OskarStark Jan 2, 2024

smnandre Jan 2, 2024

alexandre-daubois Jan 3, 2024

smnandre Jan 2, 2024

alexandre-daubois Jan 3, 2024

nicolas-grekas Jan 3, 2024

alexandre-daubois Jan 3, 2024

welcoMattic commented Jan 8, 2024

alexandre-daubois commented Sep 18, 2024

[HttpFoundation] Add Url and UrlParser #53346

[HttpFoundation] Add Url and UrlParser #53346

Conversation

alexandre-daubois commented Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Jan 2, 2024

Nyholm commented Jan 2, 2024

nicolas-grekas commented Jan 2, 2024

alexandre-daubois commented Jan 2, 2024

OskarStark commented Jan 2, 2024

nicolas-grekas commented Jan 2, 2024

Nyholm commented Jan 2, 2024

alexandre-daubois commented Jan 2, 2024 • edited Loading

Nyholm commented Jan 2, 2024

alexandre-daubois commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

welcoMattic commented Jan 8, 2024

alexandre-daubois commented Sep 18, 2024

[HttpFoundation] Add `Url` and `UrlParser` #53346

[HttpFoundation] Add `Url` and `UrlParser` #53346

alexandre-daubois commented Jan 2, 2024 •

edited

Loading

alexandre-daubois commented Jan 2, 2024 •

edited

Loading