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

strtok the resques url before right trimming the / #52

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ClementGre
Copy link

@ClementGre ClementGre commented Feb 11, 2023

In the original code :

$request_url = rtrim($request_url, '/');
$request_url = strtok($request_url, '?');

The trim was done before the split. But this leads to issues when dealing with paths like this one :
https://example.com/pricing/?type=0
Indeed, the first line will trim an unexisting / at the end of the request url.
And the second will remove the GET query parameters.
Resulting in this :
https://example.com/pricing/
This left a remaining right / that will create an element at the end of the exploded array.

Then, the condition:

if( count($route_parts) != count($request_url_parts) ){ return; }

will be evaluated as true and the router will return while it should not.

The request_url should firstly be split to remove everything after the first ?:

$request_url = strtok($request_url, '?');

And only then, the $request_url should be right trimmed to remove the last /:

$request_url = rtrim($request_url, '/');

Then, I just inverted these two lines.

The request_url should firstly be split to remove everything after the first ``?``:
>  $request_url = strtok($request_url, '?');
And only then, the $request_url should be right trimmed to remove the last ``/``:
>  $request_url = rtrim($request_url, '/');
Theses two lines were inverted.
@santiagodonoso
Copy link

Hi @ClementGre, very good observation and excellent way to explain it. The router is there to avoid query strings. So in your example "https://example.com/pricing/?type=0" the route should be "pricing/$type".

Am I missing something?

@ClementGre
Copy link
Author

You are right, then it is not possible to manage url with GET arguments in PHP Router.

Using friendly URLs is often the best choice, but in some cases you would need to use GET arguments, when working with GET forms or just sending links in mails that you want to be formatted with GET arguments.

Adding the support for GET arguments might be a good idea.

@armikbd
Copy link

armikbd commented Oct 24, 2023

Line 50-51: Following code need to rearrange again.

$request_url = rtrim($request_url, '/');
$request_url = strtok($request_url, '?');

@indev-at
Copy link

indev-at commented Feb 6, 2024

Any update on this?
cause i don't need the GET-Arguments support. but I also want to avoid if somebody types in an"/" at the end as in: www.website.tld/projects/ instead of www.website.tld/projects which will result in some error.
is there a way to implement a catch if the last / leads nowhere so it discards the element?

@jameswilson
Copy link

I agree that the order of operations should be reversed.

Or better yet, Iin order to get the "path" part of the request URI (which your code is referring to as the "route") it might be more appropriate to consider using parse_url() to break down the request into its individual components, such as the scheme, host, port, path, query, and fragment.

  $route_parts = explode('/', $route);
  array_shift($route_parts);

  $request_url = filter_var($_SERVER['REQUEST_URI'], FILTER_SANITIZE_URL);
  $request_path = parse_url($request_url, PHP_URL_PATH));
  $request_path = rtrim($request_url, '/');
  $request_path_parts = explode('/', $request_path);
  array_shift($request_url_parts);

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.

5 participants