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

Make slash "not encoding" less restrictive #661

Merged
merged 4 commits into from Aug 18, 2023

Conversation

aguingand
Copy link
Contributor

@aguingand aguingand commented Aug 17, 2023

The problem

Following up on #500. This feature is great but too restrictive:

  • The docs state the "Encoded forward slashes are only supported within the last route segment." but in reality Laravel route generator doesn't encode slashes by default for all params (cf. RouteUrlGenerator)
  • The where regex can be different than .*

Example:

Route::get('/{breadcrumb}/page/{page}')->where('breadcrumb', '.+');

Proposed solution

I move the / replacement after asserting the current param matches the given wheres. At this point if slashes are replaced it's because the RegExp allowed so.

@bakerkretzmar
Copy link
Collaborator

@aguingand thanks a lot for this. Great catch about the Regex, you're of course right it might not always be .*.

The slash encoding thing I'm going to dig into more... I see what you mean about the code, but the docs are correct in the sense that routing only works with slashes if they're in the last parameter, encoded or not. See laravel/framework#22125 and https://symfony.com/doc/current/routing.html#slash-characters-in-route-parameters. I'm not certain whether that's relevant or matters here though since that all happens after the URL is generated. I'm going to try to add some tests here to see.

@aguingand
Copy link
Contributor Author

I have tested in my project, it works well with a single starting param containing slash (both laravel + ziggy). But some js tests may be relevant here.

@bakerkretzmar bakerkretzmar merged commit 3573e2a into tighten:main Aug 18, 2023
48 checks passed
@bakerkretzmar
Copy link
Collaborator

Thanks! Gonna come back to the encoding thing separately asap.

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.

None yet

2 participants