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

URI #53731

Merged
merged 15 commits into from
Dec 9, 2024
Merged

URI #53731

merged 15 commits into from
Dec 9, 2024

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Dec 2, 2024

URI parsing and mutation powered by league/uri.

$uri = Uri::of('https://laravel.com')
    ->withQuery(['name' => 'Taylor'])
    ->withPath('/docs/installation')
    ->withFragment('hello-world');

$uri->scheme();
$uri->host();
$uri->user();
$uri->password();
$uri->path();
$uri->port();
$uri->query()->all();
$uri->query()->has('name');
$uri->query()->missing('name');
$uri->query()->decode();

// And more...

return (string) $uri;

URI instances can be retrieved from the current request, as well as created from URLs and names routes for the current application:

$uri = $request->uri();

$uri = Uri::to('/something')->withQuery(...);

$uri = Uri::route('named-route', ['user' => $user]);

URIs can even be returned directly from routes to generate a redirect to that location:

return Uri::route('named-route', ['user' => $user])->withQuery(['name' => 'Taylor']);

@taylorotwell taylorotwell marked this pull request as draft December 2, 2024 21:28
@stevebauman
Copy link
Contributor

This looks awesome! 🎉

@shaedrich
Copy link
Contributor

Awesome! Thanks to @stevebauman for your pioneer work in #53370 🎉 👏🏻

@tontonsb
Copy link
Contributor

tontonsb commented Dec 3, 2024

Tbh the API seems really smooth and making it Responsable feels natural now that I see it.

Love that you've chosen to go the immutable path using withers. Maybe it's worth adding the readonly keyword to class fields? Or maybe even mark both classes as readonly to communicate that the instances never change their state?

One more thing that I'd like to bring up — parse_str is older than the web standards (RFCs) and its behaviour on some edge cases is different from what people might expect to be the "correct" parsing. Have you considered using League\Uri\QueryString::parse() instead?

src/Illuminate/Support/Uri.php Outdated Show resolved Hide resolved
src/Illuminate/Support/Uri.php Show resolved Hide resolved
src/Illuminate/Support/Uri.php Show resolved Hide resolved
taylorotwell and others added 2 commits December 3, 2024 09:30
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: Hafez Divandari <hafezdivandari@gmail.com>
@korkoshko
Copy link
Contributor

@taylorotwell

Query parameters can include dots in their names, which is perfectly valid. But when using data_set with parse_str, unexpected behavior can happen:

$uri = Uri::of('https://dot.test/?foo.bar=baz');

dump([
    (string) $uri->withQuery(['foo.bar' => 'zab']),
    (string) $uri->replaceQuery(['foo.bar' => 'zab']),
]);

Output:

^ array:2 [
  0 => "https://dot.test/?foo_bar=baz&foo%5Bbar%5D=zab"
  1 => "https://dot.test/?foo.bar=zab"
]

What's happening:

  1. data_set: It treats foo.bar as a nested array and converts it into ['foo' => ['bar' => 'zab']].
  2. parse_str: It replaces dots with underscores (e.g., foo.bar becomes foo_bar) because PHP doesn’t allow dots in variable names.

So in the first case, foo.bar turns into both foo_bar and foo[bar], which isn’t what you’d expect.

P.S The League Query component avoids using parse_str entirely to prevent these kinds of issues.

* enhance URI class

* Update Uri.php

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
@taylorotwell taylorotwell marked this pull request as ready for review December 9, 2024 15:41
@taylorotwell
Copy link
Member Author

@korkoshko I've made withQuery and replaceQuery more consistent in how they handle dot notation.

@taylorotwell taylorotwell merged commit c9bdfed into 11.x Dec 9, 2024
39 checks passed
@taylorotwell taylorotwell deleted the uri branch December 9, 2024 16:03
@a21ns1g4ts
Copy link

You seem to be reading my thoughts

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

Successfully merging this pull request may close these issues.

9 participants