request->getPath() includes "index.php"

[patrickvuarnoz]

I think there is a bug in the getPath() method. It is composed as static::getScriptName() . static::getPathInfo()). And getScriptName() returns the contents of $_SERVER['SCRIPT_NAME']. In my environment (Apache) this will always include index.php.

If I want to compose a URL (to use in a response) with the methods from Request it would be something like request()->getUrl() . request()->getPath() which leads to https://leafphp.dev/index.php/foo/bar which is not a valid URL anymore (with the given .htaccess file from LeafsPHP).

I don't know why this getScriptName() exists in the code base but it is not the physical path as mentioned in the function description and documentation. Also getPathInfo() is not the virtual path part alone (as documented), it is the full path. The correct values would be the following:

$physicalPath = dirname($_SERVER['SCRIPT_NAME']);
$virtualPath  = substr(parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH), $physicalPath == '/' ? 0 : strlen($physicalPath));
$fullPath     = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH);
$query        = parse_url($_SERVER['REQUEST_URI'], PHP_URL_QUERY);

Following your documentation you might want to create methods like getRootUri() (=$physicalPath) and getResourceUri() (=$virtualPath) which would then be totally clear what they deliver. Also a getUri() or getFullUri() might be helpful to get rootUri + resourceUri.

What's also a bit unclear to me is why the implementation of getPath() and getPathInfo() currently return the query parameters as well. This might be a problem if you're just interested in the path. It would be better if those paths were without query parameters and if there was another method like getQueryString() with which you could get the query string separately (in my project I need this to manipulate/replace parts of it for URLs in a response).

Maybe a bit confusing is also the method getUrl(). Naturally I would expect an URL to be the full thing, i.e. https://leafphp.dev/foo/bar?a=b () and not just scheme + host + port.

[mychidarko]

This makes sense @patrickvuarnoz, I'm on board with these changes. Do you have time to make a PR with the changes you suggested?

[patrickvuarnoz]

@mychidarko I'm sorry, but I'm not too familiar with doing PRs. Also I think it should be your decision what you want to change, what to keep and how it all should be documented. This is best done by the core team.

[ibnsultan]

@mychidarko what is the state of this?

[mychidarko]

I have made some updates which I'm testing. Will push in a bit and update the docs as well @ibnsultan