slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.98k stars 1.95k forks source link

Routing bug? #1679

Closed feryardiant closed 8 years ago

feryardiant commented 8 years ago

Here I have setup my routes.php

$app->get('/[{name}]', 'App\Actions\HomeAction:index')->setName('home-page');

and here my index method in action class

public function index($req, $res, $args)
{
    if (isset($args['name'])) {
        return $this->view->render('hello', [
            'name' => $args['name'],
            'desc' => 'Welcome to world',
        ]);
    }
    return $this->view->render('home');
}

and sure it works as expected

Request URI Response
example.com[/] render home template
example.com/foobar render hallo template, with $args['name'] = 'foobar'
example.com/foo/bar return not found

but I don't expect it could work like this.

Request URI Response
example.com// render home template
example.com//foo render home template
example.com//foo/bar render hallo template, with $args['name'] = 'bar'
example.com//foo/bar/bas return not found
example.com///foo render home template
example.com///foo/bar render home template
example.com///foo/bar/bas render home template

Is it a bug?

EDIT: I'm using local ubuntu server with nginx

geggleto commented 8 years ago

Well Given that we use https://github.com/nikic/FastRoute ... I am not 100% sure.

silentworks commented 8 years ago

This is not a bug. I somewhat think its a browser/web server behaviour. I just tried this on some websites and it worked, also note some websites do redirect to the url with only 1 slash.

https://github.com///slimphp/Slim/issues/1679 https://www.google.com///doodles

feryardiant commented 8 years ago

@silentworks yes, you're right. But, can you see the difference between common behaviour and the way slim does?

I've also expect that slim route should work that way in my setup.

Request URI Response
example.com// render home template
example.com//foo render hallo template, with $args['name'] = 'foo'
example.com//foo/bar return not found
example.com//foo/bar/bas return not found
example.com///foo render hallo template, with $args['name'] = 'foo'
example.com///foo/bar return not found
example.com///foo/bar/bas return not found
geggleto commented 8 years ago

Not to be a stickler.... but difference between common behaviour and the way slim does ... we are using a 3rd party router that is very popular ... See my last comment.

feryardiant commented 8 years ago

So, anyone have test this case with other framework that use same 3rd party router that is very popular??

akrabat commented 8 years ago

This feels like a bug to me.

akrabat commented 8 years ago

I'm looking at the //foo/bar case.

This comes into Slim as $_SERVER['REQUEST_URI'] which is set to //foo/bar as you'd expect.

However, Uri::createFromEnvironment extracts it using this line:

$requestUri = parse_url($env->get('REQUEST_URI'), PHP_URL_PATH);

which results in $requestUri set to /bar

This isn't what I would expect!

akrabat commented 8 years ago

I'm pretty sure that using parse_url is wrong in this case and this line should be:

$requestUri = $env->get('REQUEST_URI');

Thoughts @codeguy, @silentworks?

geggleto commented 8 years ago

This is not a bug and is documented in FastRoute.

From the README.md of FastRoute

// Matches /user/foobar, but not /user/foo/bar
$r->addRoute('GET', '/user/{name}', 'handler');

// Matches /user/foo/bar as well
$r->addRoute('GET', '/user/{name:.+}', 'handler');

The correct pattern would be... $app->get('/[{name:.+}]', 'App\Actions\HomeAction:index')->setName('home-page');

example.com// ==> home example.com//foo ==> hallo example.com//foo/bar => matches because of the .+ example.com//foo/bar/bas => matches because of the .+ example.com///foo => hallo example.com///foo/bar => matches because of the .+ example.com///foo/bar/bas => matches because of the .+

I am not a regex wizard. But there would be a regex possible I think to do exactly what he wants.

geggleto commented 8 years ago

Also as a follow-up @akrabat

From FastRoute's documentation...

// Fetch method and URI from somewhere
$httpMethod = $_SERVER['REQUEST_METHOD'];
$uri = rawurldecode(parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH));
akrabat commented 8 years ago

This has nothing to do with FastRoute.

For the URL http://example.com//foo/bar we send /bar to FastRoute. i.e. we've removed the //foo/ from the URL before we set it into the Uri instance.

akrabat commented 8 years ago

That documentation snippet explains where we got the idea to use parse_url()!

However, the documentation for parse_url says:

Note: This function doesn't work with relative URLs.

geggleto commented 8 years ago

haha...

This function is intended specifically for the purpose of parsing URLs and not URIs.

akrabat commented 8 years ago

How about solving like this:

$requestUri = parse_url('http://example.com' . $env->get('REQUEST_URI'), PHP_URL_PATH);

As we only want the URI, the actual value of the scheme and domain are unimportant, but it will make parse_uri work as we would expect.

geggleto commented 8 years ago

Reference: https://github.com/slimphp/Slim/blob/3.x/Slim/Http/Uri.php#L194

geggleto commented 8 years ago

That solution looks fine to me.

I was wondering if the output of requestURI was being based into the constructor of the URI but it is not.

:+1: