Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] AddUrl andUrlParser#53346
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
7adb171 to1407a55Compare| /** | ||
| * @author Alexandre Daubois <alex.daubois@gmail.com> | ||
| */ | ||
| finalclass Parametersimplements \Stringable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would still name thisàI would name this classDsn`
| yield [ | ||
| 'some://', | ||
| 'Themailer DSN is invalid.', | ||
| 'The"some://" DSN is invalid.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Exposing the DSN here means it could leak sensitive data to a log file
xabbuh commentedJan 2, 2024
see#32546 for a related discussion |
Nyholm commentedJan 2, 2024
Here is a link to a PR I made to try introduce a DSN component. 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 commentedJan 2, 2024
💯 That's why I wrote we might need a URL parser, not a DSN parser ;) |
alexandre-daubois commentedJan 2, 2024
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 commentedJan 2, 2024
I understand, but we created our own standard of DSN in the Symfony world, which is spread across different components |
nicolas-grekas commentedJan 2, 2024
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. |
1407a55 to303a8c9CompareNyholm commentedJan 2, 2024
Yes, that work has beenimplementation driven. I looked at how Symfony is using DSN and I created aspecification 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 commentedJan 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Would you have an example? From what I saw during the
Thank you for that! 🙏 |
Nyholm commentedJan 2, 2024
It is mostly the "function" part. Some components supports DSN like: |
alexandre-daubois commentedJan 2, 2024
Just a quick thought but shouldn't we just use Tobias' package in the framework? |
Dsn\Parser andDsn\ParametersUrl andUrlParser| class InvalidUrlExceptionextends \InvalidArgumentException | ||
| { | ||
| publicfunction__construct(string$dsn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
string $url ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
parameter can be removed IMHO
| $dsn =$this->scheme.'://'; | ||
| if ($this->isAuthenticated()) { | ||
| $dsn .=rawurlencode($this->user).':'.rawurlencode($this->pass).'@'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will throw if one ofthis->user orthis->pass is null, asrawurlencode() requires a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch 👍
| publicfunction__construct( | ||
| publicstring$scheme, | ||
| public ?string$user =null, | ||
| public ?string$pass =null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could we usepassword here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could, but this would make impossible to do the following:
$url =newUrl(...parse_url($url));
This is why I keptpass here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the goal of the new code should be to completely replace using parse_url, so this argument is void sorry. (parse_url is broken)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yes, of course 🤦 Updated accordingly
303a8c9 todb7c57eComparedb7c57e to31232eeComparewelcoMattic commentedJan 8, 2024
Is |
alexandre-daubois commentedSep 18, 2024
Closed in favor ofsymfony/polyfill#498 |
Uh oh!
There was an error while loading.Please reload this page.
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 afirst 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: