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

Fix parameter validation when generating relative URLs to routes with domain parameters #755

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

bakerkretzmar
Copy link
Collaborator

Laravel requires that domain parameters always be passed to the route() helper, even if the user is generating a relative URL and the domain parameters will not appear in the final output. Omitting the parameter results in an error. Depending how the parameters are passed the error will be different, but the underlying problem is the same:

Route::domain('{team}.ziggy.dev')->get('users/{user}', fn () => '')->name('users.index');

// Desired output: "/users/1"

route('users.index', ['team' => 'tighten', 'user' => 1], absolute: false);
// Produces desired output

route('users.index', ['user' => 1], absolute: false);
// Error, missing required route parameter 'team' - only one param value was passed, and it was specifically assigned to the `user` param

route('users.index', ['tighten', 1], absolute: false);
// Produces desired output

route('users.index', [1], absolute: false);
// Error, missing required route parameter 'user' - because param values were passed in order, `1` was handled as the `team` parameter

Ziggy currently behaves exactly the opposite way—it effectively prohibits domain parameters from being passed to the route() helper and handled as such, if the user is generating a relative URL:

// Desired output: "/users/1"

route('users.index', { user: 1 }, false);
// Produces desired output, unlike Laravel, which errors in this case

route('users.index', { team: 'tighten', user: 1 }, false);
// Produces "/users/1?team=tighten", incorrectly adding the `tighten` domain parameter to the query

route('users.index', [1], false);
// Produces desired output, unlike Laravel, which errors

route('users.index', ['tighten', 1], false);
// Produces "/users/tighten?1", incorrectly handling `'tighten'` as the `user` parameter and adding `1` to the query

So Laravel requires that all required parameters be provided, and Ziggy requires that only the required parameters that will appear in the output be provided.

This was almost certainly unintentional, and a side effect of how we replace parameters in the URL. Ziggy generates a template of the final output of the route() helper, which, if the relative option was passed, only contains the route path, and then replaces all the parameters in that template. Laravel generates a template of the entire URL, replaces all the parameters in that template, and then returns only the path if the relative option was passed.

I'm going to propose that we update Ziggy to match Laravel's behaviour, which is a large change, but justified in my opinion since it fixes what I believe is a large bug.

For now this PR includes a failing test demonstrating the issue.

Base automatically changed from fix-stripping-slashes-751 to 2.x May 16, 2024 23:25
@bakerkretzmar bakerkretzmar force-pushed the fix-domain-relative branch from 24a0724 to cd3d1b7 Compare May 16, 2024 23:29
@bakerkretzmar bakerkretzmar force-pushed the fix-domain-relative branch from cd3d1b7 to 7ec767f Compare May 16, 2024 23:32
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.

1 participant