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

Add query parameters to named routes. #85

Closed
thinkverse opened this issue Aug 15, 2023 · 3 comments · Fixed by #88
Closed

Add query parameters to named routes. #85

thinkverse opened this issue Aug 15, 2023 · 3 comments · Fixed by #88
Assignees

Comments

@thinkverse
Copy link

thinkverse commented Aug 15, 2023

Currently, named routes don't support passing in query parameters to route, which doesn't provide 100% feature parity with native named routes. I noticed this while updating my test app tangerine to support named routes.

I also confirmed this by updating the feature parity test and appending a query parameter.

test('feature parity', function () {
    Route::get('/posts/{lowerCase}/{UpperCase}/{podcast}/{user:email}/show', function (
        string $id,
        string $upperCase,
        Podcast $podcast,
        User $user) {
        //
    })->name('users.regular');

    $parameters = [
        'lowerCase' => 'lowerCaseValue',
        'upperCase' => 'UpperCaseValue',
        'podcast' => Podcast::first(),
        'user' => User::first(),
        'query' => 'string',
    ];

    $expectedRoute = '/posts/lowerCaseValue/UpperCaseValue/1/test-email-1@laravel.com/show?query=string';

    $route = route('users.regular', [
        'lowerCase' => 'lowerCaseValue',
        'UpperCase' => 'UpperCaseValue',
        'podcast' => Podcast::first(),
        'user' => User::first(),
        'query' => 'string',
    ], false);

    expect($route)->toBe($expectedRoute);

    $route = route('posts.show', $parameters, false);

    expect($route)->toBe($expectedRoute);
});

This fails the expectation for Folio's named routes, but not for the native named routes:

 FAILED  Tests\Feature\NameTest > feature parity                                                                                             
  Failed asserting that two strings are identical.
  -'/posts/lowerCaseValue/UpperCaseValue/1/test-email-1@laravel.com/show?query=string'
  +'/posts/lowerCaseValue/UpperCaseValue/1/test-email-1@laravel.com/show'
  

  at tests/Feature/NameTest.php:103
     99▕     expect($route)->toBe($expectedRoute);
    100▕ 
    101▕     $route = route('posts.show', $parameters, false);
    102▕ 
  ➜ 103▕     expect($route)->toBe($expectedRoute);
    104▕ });
    105▕ 
    106▕ test('model route binding wrong column', function () {
    107▕     $parameters = [

  1   tests/Feature/NameTest.php:103


  Tests:    1 failed (2 assertions)
  Duration: 0.08s

This is used internally by Laravel, say password reset when generating the URL for the notification, which adds the users' email as a query parameter. Using a regular named route adds the query parameter, while Folio doesn't.

Would be great if Folio also supported adding query parameters when generating a URL with route.

@driesvints
Copy link
Member

@nunomaduro this is the same request as #81 I think

@thinkverse
Copy link
Author

thinkverse commented Aug 16, 2023

this is the same request as #81 I think

Don't think so, sounds like #81 wants to be able to pre-defined queries in the Folio file, maybe? IDK, query strings are already working in Folio, and Folio with Volt. They're accessible via request() or $_GET as always. Or with state()->url() in Volt.

What I'm asking is to be able to generate a URL with route and include query parameters in that. Figure a signup page with a referrer for instance:

route('register', ['referrer' => 'dries@laravel.com']);
// Currently with Folio named route:
// http://localhost/register

route('register', ['referrer' => 'dries@laravel.com']);
// Currently with a regular named route:
// http://localhost/register?referrer=dries%40laravel.com

Generating a URL with a named Folio route doesn't include the query parameter, as a regular named route would.

@nunomaduro
Copy link
Member

Fix in progress: #88.

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

Successfully merging a pull request may close this issue.

3 participants